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

Fix JSON export for reports (#175) #180

Merged
merged 2 commits into from
Jun 4, 2022
Merged

Fix JSON export for reports (#175) #180

merged 2 commits into from
Jun 4, 2022

Conversation

mehaase
Copy link
Contributor

@mehaase mehaase commented May 5, 2022

JSON exports were being rendered through an HTML template due to
content negotiation in the browser (e.g. Accept:text/html). This commit
forces the renderer to use JSON regardless of content negotiation and
also cleans up the json and docx export code.

@mehaase mehaase requested review from nschwane and m3mike May 5, 2022 16:38
Copy link
Contributor

@nschwane nschwane left a comment

Choose a reason for hiding this comment

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

I don't feel like this is the ideal way to do this with DRF since you can get JSON with built-in behavior by changing the original URL from /api/report-export/1/?type=json to /api/report-export/1/?format=json, but at least this removes a ViewSet and is using the JSONRenderer. For consistency, it would be ideal to have a docx renderer, but I don't see that being strictly necessary until it is needed in more than one place.

Also, the unit tests need to be updated.

@mehaase mehaase marked this pull request as draft May 19, 2022 20:14
JSON exports were being rendered through an HTML template due to
content negotiation in the browser (e.g. Accept:text/html). This commit
forces the renderer to use JSON regardless of content negotiation and
also cleans up the json and docx export code.
@mehaase mehaase force-pushed the 175-fix-json-export branch 2 times, most recently from 955a3ed to efe82ac Compare May 20, 2022 12:02
Based on Nick's feedback, reverted to using two ViewSets, moved the
docx code into a custom renderer, and use Django's built in format
parameter instead of type. This makes for a much cleaner PR.
@mehaase mehaase force-pushed the 175-fix-json-export branch from efe82ac to d4e968e Compare May 20, 2022 12:03
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mehaase mehaase marked this pull request as ready for review May 20, 2022 12:04
@codecov
Copy link

codecov bot commented May 20, 2022

Codecov Report

Merging #180 (d4e968e) into main (5e07e7d) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #180      +/-   ##
==========================================
- Coverage   95.76%   95.75%   -0.01%     
==========================================
  Files           9       10       +1     
  Lines         874      872       -2     
==========================================
- Hits          837      835       -2     
  Misses         37       37              
Impacted Files Coverage Δ
src/tram/renderers.py 100.00% <100.00%> (ø)
src/tram/urls.py 100.00% <100.00%> (ø)
src/tram/views.py 95.03% <100.00%> (-0.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f74039e...d4e968e. Read the comment docs.

@mehaase mehaase requested a review from nschwane May 20, 2022 12:20
Copy link
Contributor

@nschwane nschwane left a comment

Choose a reason for hiding this comment

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

I definitely like this approach much more than the original. I confirmed that both JSON and DOCX exports work as expected.

@mehaase mehaase merged commit b50bfd1 into main Jun 4, 2022
@mehaase mehaase deleted the 175-fix-json-export branch June 4, 2022 00:58
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.

2 participants