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

Cache GIDD exports using S3 #628

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft

Cache GIDD exports using S3 #628

wants to merge 2 commits into from

Conversation

thenav56
Copy link
Member

Addresses

Changes

  • Save files to s3

This PR doesn't introduce any:

  • temporary files, auto-generated files or secret keys
  • n+1 queries
  • flake8 issues
  • print
  • typos
  • unwanted comments

This PR contains valid:

  • tests
  • permission checks (tests here too)
  • translations

Copy link

sentry-io bot commented May 14, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: apps/gidd/views.py

Function Unhandled Issue
export_disaggregated AttributeError: 'NoneType' object has no attribute 'strftime' /external-api/gidd/disaggregations/disaggreg...
Event Count: 4
export AttributeError: 'NoneType' object has no attribute 'strftime' /external-api/gidd/disasters/dis...
Event Count: 2

Did you find this useful? React with a 👍 or 👎

@thenav56 thenav56 force-pushed the feature/cache-export branch 3 times, most recently from c1a3ef9 to b332143 Compare May 14, 2024 06:28
DISAGGREGATION_EXPORT = 'disaggregation-export'
DISAGGREGATION_EXPORT_GEOJSON = 'disaggregation-export-geojson'
DISASTER_EXPORT = 'disaster-export'
DISPLACEMENT_EXPORT = 'displacement-export'
Copy link
Contributor

Choose a reason for hiding this comment

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

I also saw conflict-export in the gidd export.
We need to see if we also add them.

Copy link
Member Author

@thenav56 thenav56 May 15, 2024

Choose a reason for hiding this comment

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

Conflict-export is generated using filter by another export.

Comment on lines 45 to 51
return (
re.sub(
' +',
' ',
(StatusLog.last_release_date() or '').replace(',', ' ')
)
).replace(',', ' ').strip().replace(' ', '-')
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we converting datetime to date?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

GiddExportCache.Key.DISAGGREGATION_EXPORT_GEOJSON,
lambda: self._export_disaggregated_geojson(filename, qs),
s3_parameters={
'ResponseContentDisposition': f'attachment; filename={filename}.geojson',
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need the response content type here? or is json default content type?

Copy link
Member Author

@thenav56 thenav56 May 15, 2024

Choose a reason for hiding this comment

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

Done
NOTE: This is automatically handed by S3 as well

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