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

Implement exporting of all company interest data to a csv file #3231

Merged
merged 2 commits into from Mar 6, 2023

Conversation

Arashfa0301
Copy link
Contributor

@Arashfa0301 Arashfa0301 commented Mar 4, 2023

Implement exporting of all company interest data to a csv file as requested by Intercom. The query will specify which answers to export. The answers are exported by the semester they are registered in and the type of event they are related to.


image


@Arashfa0301 Arashfa0301 added new-feature Pull requests that introduce or issues that suggest a new feature priority:high Pull requests that have high priority, and should therefore be prioritized do-not-merge/hold Pull request is on hold (e.g. waiting for something else to be merged), and should not be merged labels Mar 4, 2023
@Arashfa0301 Arashfa0301 self-assigned this Mar 4, 2023
@ivarnakken ivarnakken added review-needed Pull requests that need review and removed priority:high Pull requests that have high priority, and should therefore be prioritized labels Mar 5, 2023
@Arashfa0301 Arashfa0301 force-pushed the companyInterest-export-to-csv branch from 5f5e80e to bbd0302 Compare March 5, 2023 15:17
Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

Cool 💯

Would be nice with a test checking the permissions of the user requesting, although it looks fine.

lego/apps/companies/views.py Outdated Show resolved Hide resolved
lego/apps/companies/views.py Outdated Show resolved Hide resolved
lego/apps/companies/views.py Outdated Show resolved Hide resolved
lego/apps/companies/views.py Outdated Show resolved Hide resolved
@Arashfa0301 Arashfa0301 force-pushed the companyInterest-export-to-csv branch 2 times, most recently from 3778e13 to 19d4089 Compare March 6, 2023 01:02
lego/apps/companies/views.py Outdated Show resolved Hide resolved
lego/apps/companies/views.py Show resolved Hide resolved
_get_export_company_interest()
)
self.assertEqual(
company_interest_export_responce.status_code, status.HTTP_403_FORBIDDEN
Copy link
Member

Choose a reason for hiding this comment

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

You should test for both cases, so a 200 response as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't manage to find out how make a request in the reverse() function that includes the query parameters "year" and "semester". Could you give me some tips.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think you can make a new reverse function for that, but you shouldn't need to. The reverse function is based on what is defined in the urls, but to send a valid response you only need to manipulate the query params (from the view).

If you take the reverse url you have already used and append "?year=2023&semester=whatnot", from a Bedkom-user you should get a valid response.
You have some discussions of different ways to do it here https://stackoverflow.com/questions/4995279/including-a-querystring-in-a-django-core-urlresolvers-reverse-call

@ivarnakken
Copy link
Member

Also, use multi line commit messages where you describe your change if you're gonna do a "small fixes".

@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Patch coverage: 96.66% and project coverage change: +0.02 🎉

Comparison is base (3462a11) 88.31% compared to head (800b71c) 88.34%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3231      +/-   ##
==========================================
+ Coverage   88.31%   88.34%   +0.02%     
==========================================
  Files         659      659              
  Lines       20770    20829      +59     
==========================================
+ Hits        18344    18401      +57     
- Misses       2426     2428       +2     
Impacted Files Coverage Δ
lego/apps/companies/views.py 95.28% <95.12%> (-0.18%) ⬇️
lego/apps/companies/constants.py 100.00% <100.00%> (ø)
lego/apps/companies/tests/test_company_interest.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@norbye norbye left a comment

Choose a reason for hiding this comment

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

Some small knitpicking

lego/apps/companies/views.py Outdated Show resolved Hide resolved
lego/apps/companies/views.py Outdated Show resolved Hide resolved
lego/apps/companies/views.py Outdated Show resolved Hide resolved
_get_export_company_interest()
)
self.assertEqual(
company_interest_export_responce.status_code, status.HTTP_403_FORBIDDEN
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you can make a new reverse function for that, but you shouldn't need to. The reverse function is based on what is defined in the urls, but to send a valid response you only need to manipulate the query params (from the view).

If you take the reverse url you have already used and append "?year=2023&semester=whatnot", from a Bedkom-user you should get a valid response.
You have some discussions of different ways to do it here https://stackoverflow.com/questions/4995279/including-a-querystring-in-a-django-core-urlresolvers-reverse-call

@Arashfa0301 Arashfa0301 force-pushed the companyInterest-export-to-csv branch 6 times, most recently from 84a9524 to fa352db Compare March 6, 2023 13:04
Wrote tests that check the permissions of the user requesting and other small fixes as requested in the pr conversations
@Arashfa0301 Arashfa0301 force-pushed the companyInterest-export-to-csv branch from fa352db to 800b71c Compare March 6, 2023 13:31
@Arashfa0301 Arashfa0301 removed the do-not-merge/hold Pull request is on hold (e.g. waiting for something else to be merged), and should not be merged label Mar 6, 2023
@Arashfa0301 Arashfa0301 merged commit efff81b into master Mar 6, 2023
@Arashfa0301 Arashfa0301 deleted the companyInterest-export-to-csv branch March 6, 2023 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature Pull requests that introduce or issues that suggest a new feature review-needed Pull requests that need review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants