Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Making export #385

Merged
merged 5 commits into from
Jun 13, 2023
Merged

Making export #385

merged 5 commits into from
Jun 13, 2023

Conversation

parteekcoder
Copy link

issue #366

@NikhilM98 @llaske
On clicking the export button which I added , based on filters it will download the result

@parteekcoder parteekcoder changed the base branch from master to dev March 7, 2023 07:23
@NikhilM98
Copy link
Collaborator

Don't you already have a PR open addressing this issue?

@parteekcoder
Copy link
Author

Don't you already have a PR open addressing this issue?

yeah in that PR you removed my confusion regarding the feature actually required , that's why I shifted to new

@parteekcoder
Copy link
Author

Hey @llaske @NikhilM98 please review this PR

What I have done here is:-
when you click on export assignment button it will download the CSV files based on filters

I would to like hear your suggestions and work upon them

@NikhilM98
Copy link
Collaborator

@parteekcoder there are some linting issues with this PR. Can you handle them?

@parteekcoder
Copy link
Author

@parteekcoder there are some linting issues with this PR. Can you handle them?

sure I will remove that , but there is also an other PR having the same code 396

@parteekcoder
Copy link
Author

@parteekcoder there are some linting issues with this PR. Can you handle them?

yeah @NikhilM98 , now I removed all the lint errors thanks for reminding

Now you can review

@parteekcoder
Copy link
Author

hey @NikhilM98 I removed all eslint error in my code

@NikhilM98
Copy link
Collaborator

sure I will remove that , but there is also an other PR having the same code 396

It's okay to have another PR for the same issue if it's intended for GSoC. The quality of PRs is an important factor for evaluating the understanding of a contributor about the project.

@parteekcoder
Copy link
Author

sure I will remove that , but there is also an other PR having the same code 396

It's okay to have another PR for the same issue if it's intended for GSoC. The quality of PRs is an important factor for evaluating the understanding of a contributor about the project.

@NikhilM98 If you would like anything needs to be improved in the PR , I will appreciate that and will update it

dashboard/views/deliveries.ejs Outdated Show resolved Hide resolved
dashboard/controller/assignments/exportAssignment.js Outdated Show resolved Hide resolved
dashboard/controller/assignments/exportAssignment.js Outdated Show resolved Hide resolved
@parteekcoder
Copy link
Author

parteekcoder commented Mar 18, 2023

@NikhilM98 done the changes you suggested , I can also update the fields which you would like to keep in .csv file

Thanks for your review

@parteekcoder parteekcoder requested a review from NikhilM98 March 18, 2023 17:22
@parteekcoder
Copy link
Author

hey @llaske @NikhilM98 @ricknjacky please can you review this pull request so that it can be merged. And we can the discuss the fields to be exported in the csv file

@llaske
Copy link
Owner

llaske commented Jun 10, 2023

Hi @parteekcoder ,
Thanks for this contribution.
Some information are missing: student name, delivery date and score.

image

@parteekcoder
Copy link
Author

Yeah sure @llaske i will add those , thanks for testing and your invaluable time.

If you have any suggestions I will be happy to work on them

@parteekcoder
Copy link
Author

Hi @llaske @NikhilM98 @ricknjacky ,
Thanks @llaske for testing this PR, I included the fields you told. And updated the PR

image

Please let me know if there are other changes to be made to improve this PR

Copy link
Owner

@llaske llaske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice. Please fix my comment.

Comment on lines 61 to 64
// console.log(response.body.deliveries[0]);
// console.log(response.body.deliveries[0].content);
// console.log(response.body.deliveries[0].content[0]);
// console.log(response.body.deliveries[0].content[0].metadata);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove debug message.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh!! yes , i forget to remove it. I will update the pr

@llaske llaske merged commit c3137c6 into llaske:dev Jun 13, 2023
@llaske
Copy link
Owner

llaske commented Jun 13, 2023

Thanks @parteekcoder , good job.

@parteekcoder
Copy link
Author

@llaske thanks for testing efforts and the suggestions you give to improve code quality

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants