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

Feature/80 export students data #113

Merged
merged 23 commits into from
Mar 25, 2024
Merged

Conversation

pedlopruz
Copy link
Contributor

I have implemented the export of students in both pdf and csv format.
You can access the urls :
http://127.0.0.1:8000/export/csv/students and http://127.0.0.1:8000/export/pdf/students to try it out.
Any suggestions for changes let me know in the comments.

@pedlopruz pedlopruz added the enhancement New feature or request label Mar 20, 2024
@pedlopruz pedlopruz added this to the Sprint 2 milestone Mar 20, 2024
@pedlopruz pedlopruz self-assigned this Mar 20, 2024
@pedlopruz pedlopruz changed the base branch from main to develop March 20, 2024 14:18
FelixoGudiel
FelixoGudiel previously approved these changes Mar 20, 2024
Copy link
Contributor

@FelixoGudiel FelixoGudiel left a comment

Choose a reason for hiding this comment

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

I thought that it didn't work, but I have found that the URLs that you have given are incorrect. I will add the correct ones in this comment, so that other reviewers can try them out directly.

http://127.0.0.1:8000/api/export/pdf/students
http://127.0.0.1:8000/api/export/csv/students

Apart from that, works as expected. Good job!

manortbla
manortbla previously approved these changes Mar 20, 2024
Copy link
Contributor

@manortbla manortbla left a comment

Choose a reason for hiding this comment

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

Thanks @FelixoGudiel for the correct urls. Both the excel and the pdf download correctly. Well done.
Remember to update from develop.

Copy link
Contributor

@pabpercab1 pabpercab1 left a comment

Choose a reason for hiding this comment

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

Thank you for submitting your feature so early, but can you please add the Excel export, and query parameters, as well as in the donations task?

Sorry about that, It was implemented in last week's issue, but I forgot to add it in the ones of this week.

@pedlopruz
Copy link
Contributor Author

Thanks for the comments, first of all, sorry for the urls I do not know how it could have happened and secondly I do the new without any problem the only thing is that in donations the parameters were around two dates in this case that filtering would be the most appropriate

@pabpercab1
Copy link
Contributor

Thanks for the comments, first of all, sorry for the urls I do not know how it could have happened and secondly I do the new without any problem the only thing is that in donations the parameters were around two dates in this case that filtering would be the most appropriate

Of course! In this case, it would be great to have a filter based on the name, surname, family, nationality, if it's a morning student or even the lesson on which the student is enrolled if you think it's possible.

@pedlopruz pedlopruz dismissed stale reviews from manortbla and FelixoGudiel via 2835f7a March 20, 2024 19:55
@pedlopruz
Copy link
Contributor Author

I have finished implementing the export in Pdf and excel of students with the filters. You can filter to test by adding the fields name, surname, nationality, family and is a moning student.
These are some of the urls that you can test although you can make about 30 different combinations specifying one or the other:
http://127.0.0.1:8000/api/export/pdf/students?name=Felipe&nationality=España
http://127.0.0.1:8000/api/export/csv/students?surname=Moreno&family=Familia Moreno
If you want me to change some of the attributes for others, let me know, although I tell you that it is already excessive so many filters because you have to combine all with all.

Copy link
Contributor

@pabpercab1 pabpercab1 left a comment

Choose a reason for hiding this comment

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

Well done! I think that the feature is much better now!

However, can you consider also adding a filter considering the current education year and the status? Because I believe that these are important filters when in the future the app contains a lot of data.

Also, I tried this query http://127.0.0.1:8000/api/export/pdf/students?isamorningstudent=True and seems it is not working for me.

Finally, Can you try to add the query to the document name as the feature of last week?

Thank you so much!

@pedlopruz
Copy link
Contributor Author

Thank you for your comment, I will immediately add the two you have told me to filter in the query. The url of is a moning student does not work because I have put it only morning because I consider the name too long for a query. I also implement the name in the title without any problem.

@pedlopruz
Copy link
Contributor Author

I have just implemented the 2 new filters with their correct filenames. These are an example to test, but you can try all the options:
http://127.0.0.1:8000/api/export/pdf/students?morning=False&status=PENDIENTE
http://127.0.0.1:8000/api/export/pdf/students?education_year=CUARTO PRIMARIA

@FelixoGudiel
Copy link
Contributor

Seems like the second link does not work. I would think about changing all the enums to a pattern like CUARTO_PRIMARIA instead of CUARTO PRIMARIA so that it can be understood as a single URL.

@pedlopruz
Copy link
Contributor Author

The problem is with GitHub because it interprets it as outside the URL, but if you copy it and paste it in the browser there is no problem.

@FelixoGudiel FelixoGudiel self-requested a review March 24, 2024 18:46
FelixoGudiel
FelixoGudiel previously approved these changes Mar 24, 2024
Copy link
Contributor

@FelixoGudiel FelixoGudiel left a comment

Choose a reason for hiding this comment

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

You are right. Everythong seems in order then!

manortbla
manortbla previously approved these changes Mar 25, 2024
pabpercab1
pabpercab1 previously approved these changes Mar 25, 2024
Copy link
Contributor

@pabpercab1 pabpercab1 left a comment

Choose a reason for hiding this comment

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

The feature works like a charm. Good work!

@pedlopruz pedlopruz merged commit c5de5f5 into develop Mar 25, 2024
1 check passed
@pedlopruz pedlopruz deleted the feature/80-export-students-data branch March 25, 2024 16:07
@pabpercab1 pabpercab1 restored the feature/80-export-students-data branch March 26, 2024 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants