Conversation
d25e67f to
22f55b4
Compare
| wagtail-inventory>=3.1 | ||
| unittest-xml-reporting | ||
| whitenoise | ||
| google-api-python-client |
There was a problem hiding this comment.
From the google-api-python-client README:
The maintainers of this repository recommend using Cloud Client Libraries for Python, where possible, for new code development due to the following reasons:
Was that considered? Would the Cloud Client Libraries be suitable here?
| unittest-xml-reporting | ||
| whitenoise | ||
| google-api-python-client | ||
| google-auth-httplib2 |
There was a problem hiding this comment.
- Is this used? I don't see it in the code.
- Google no longer recommends using this library:
For any new usages please see provided transport layers by google-auth library.
There was a problem hiding this comment.
I think you're right, we can remove this requirement. Although it looks like google-auth-httplib2 is still a requirement of google-api-python-client.
6b9eaf1 to
2166c09
Compare
| default='other_incident', | ||
| help_text='Please check the styleguide to associate the icons with their name' | ||
| ) | ||
| google_sheets_name = models.TextField( |
There was a problem hiding this comment.
| google_sheets_name = models.TextField( | |
| google_sheets_category_name = models.TextField( |
| FieldPanel('plural_name'), | ||
| FieldPanel('page_symbol'), | ||
| FieldPanel( | ||
| 'google_sheets_name', |
There was a problem hiding this comment.
| 'google_sheets_name', | |
| 'google_sheets_category_name', |
| spreadsheetId=SPREADSHEET_ID, | ||
| range="A2:Z5299", |
There was a problem hiding this comment.
question: can you get the whole sheet without a range? Hard-coding this range seems clunky. There's already code to skip rows in sync_prepubs().
| class ShortcutsPanel(Component): | ||
| # This is an ordering number that is the minimum multiple of 10 to place | ||
| # this panel underneath the built-in Wagtail panels. | ||
| order = 100 |
There was a problem hiding this comment.
question: is there a constant we can use, such as WAGTAIL_MAX_ORDERING + 10?
| scopes=["https://www.googleapis.com/auth/spreadsheets.readonly"], | ||
| ) | ||
|
|
||
| return build("sheets", "v4", credentials=creds) |
There was a problem hiding this comment.
suggestion: put v4 in a constant
harrislapiroff
left a comment
There was a problem hiding this comment.
Just a few comments—will leave final review to others.
| class PrepublicationIncidentSync(models.Model): | ||
| class Status(models.TextChoices): | ||
| SUCCESS = 'SUCCESS' | ||
| INVALID_DATA = 'INVALID_DATA' | ||
| FAILED = 'FAILED' | ||
|
|
||
| status = models.CharField(max_length=255, choices=Status.choices) | ||
| completed_at = models.DateTimeField(auto_now=True) | ||
| message = models.TextField(default='') | ||
|
|
||
| class Meta: | ||
| ordering = ["-completed_at"] |
There was a problem hiding this comment.
Might be worth noting in a comment that this is intended to be a singleton
| def handle(self, *args, **options): | ||
| try: | ||
| status, message = sync_prepubs() | ||
| except Exception as e: | ||
| message = f'Prepub sync failed: {e}' | ||
| status = PrepublicationIncidentSync.Status.FAILED | ||
| self.stdout.write(f'Prepub sync failed: {e}') | ||
| raise |
There was a problem hiding this comment.
Ideally if sync fails, we should ensure the failure gets logged to kibana and marked as an error in some way. Then we can work with infra on what level of notification we want on these sort of failures.
| ) | ||
|
|
||
|
|
||
| SPREADSHEET_ID = "1PeMPpol5d0MrF0KH36ZviN7Z4PipK6ZeSDh9AlJ3-eA" |
There was a problem hiding this comment.
As discussed in today's meeting, we should figure out what to do about this ID. If our auth credentials are tied to the specific spreadsheet, it's possible we should put them in the same place (both env vars?)
Alternatively, if it's possible to update the permissions for our auth credentials dynamically, it might make sense to make this configurable in the wagtail admin as a site setting.
Maybe discuss with infra before settling on a decision?
| else: | ||
| sync.message = message | ||
| sync.status = status | ||
| sync.save() |
There was a problem hiding this comment.
Should we also log here-ish the results of the sync if it's not SUCCESS? That way we'll capture INVALID_DATA in the logs too.
2166c09 to
410343d
Compare
The styles here are very rudimentary, and will probably need some fixing up before we'd want this to go live.
410343d to
031efa9
Compare
Description
Fixes #2074
This pull request:
sync_prepubs. Conceptually, this command is pretty simple: it connects to a specific Google sheet, fetches the rows, selects the ones that refer to pre-published incidents, and saves the data as a Django model:PrepublicationIncident. We're interested in 3 pieces of data: a date, a location, and one or more categories.date.GeoNamefor that location. If there isn't aGeoNamefor that location, the row will be marked as invalid. There is a special case of "Washington, D.C." that we have to handle separately.CategoryPageobjects these names correspond to. The names do not necessarily match the titles of any existing category pages, so I added a field on that model (accessible from the Settings tab) to store the exact string that is used in the Google sheet. These have to be updated manually. The choices for these names are found in the Categories tab on the Google Sheet.sync_prepubscommand analyzes the rows, and if it finds invalid data it will not create or delete any Django objects. All data must be valid before the sync process will actually sync.PrepublicationIncidentobjects in the admin or anywhere on the site. I recommend using the./manage.py shellor viewing the database directly to see the data in there./adminhome page, to display the outcome of the most recent sync. If there are errors, it will put the actual error message in a<details>element. This part of the site could use some nicer design, I think, but I have run out of time for that.Type of change
Testing
but if you have any better ideas for this, let me know.
Geonames fixtures. The command won't be able to look up the location names unless the geonames fixtures are loaded with
./manage.py loaddata cities5000-us-only.json.xzOnce you have the credentials set up, test this command with
./manage.py sync_prepubs. Ideally there should be no output. The results of the command should be visible at/admin. You can check the actual data in the./manage.py shellor in the DB.Pre-deployment actions
The env var
GOOGLE_SHEETS_CREDSneeds to be populated.Post-deployment actions
google_sheets_namefilled correctlysync_prepubscommand should be added to a recurring task.Checklist
General checks
If you made changes to API flow:
If you made changes to incident model metadata
If you made changes to blog
If you made changes to shared templates (e.g. card design, lead media, etc.)
If you made changes to email signup flow
If you made changes to "Submit an Incident" form
If it's a major change
If you made any frontend change
If the PR involves some visual changes in the frontend, it is recommended to add a screenshot of the new visual.