Simplify how we put together polygons for alerts #1534
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eric-gade
approved these changes
Aug 5, 2024
There was a problem hiding this comment.
LGTM 👍
Can confirm that the caching is a help -- loads almost instantly now when reloaded in different browsers.
Only thing is I don't have the context about the parsing change.
Code Review Checklist
This is an automated comment on every pull request requiring a review. A checked item is either an assertion that I have tested this item or an indication that I have verified it does not apply to this pull request.
The Basics
- Checks are passing
- I read the code
- I ran the code
- (if applicable) Post deploy steps are run
- (if applicable) I validated the change on the deployed version in
Documentation
- changes to “how we do things” are documented in READMEs
- all new functions and methods are commented using plain language
- any new modules added documented in modules.md
Security
- security false positives are documented
- data from external sources is cleaned and clearly marked
Reliability
- error handling exists for unusual or missing values
- interactions with external systems are wrapped in try/except
- functionality is tested with unit or integration tests
- dependency updates in composer.json also got changed in composer-lock.json
Infrastructure
- all changes are auditable and documented via a script
- it is clear who can and should run the script
- (if applicable) diagrams have been updated or added in PlantUML
Accessibility
- New pages have been added to cypress-axe file so that they will be tested with our automated accessibility testing
- Meets WCAG 2.0 AA or 2.1 AA for Section 508 compliance
- Site is keyboard accessible. All interactions can be accessed with a keyboard
- Site is free of keyboard traps. The keyboard focus is never trapped in a loop
- All form inputs have explicit labels
- Form instructions are associated with inputs
- All relevant images use an img tag
- All images have appropriate alt attributes
- Multimedia is tagged. All multimedia has appropriate captioning and audio description
- Text has sufficient color contrast. All text has a contrast ratio of 4.5:1 with the background
- Site never loses focus. Focus is always visible when moving through the page with the keyboard
- Tab order is logical
- Tables are coded properly. Tables have proper headers and column attributes
- Headings are nested properly. Heading elements are nested in a logical way
- Language is set. The language for the page is set
- CSS is not required to use the page. The page makes sense with or without CSS
- Links are unique and contextual. All links can be understood taken alone, e.g., ‘Read more - about 508’
- Page titles are descriptive
Device Matrix
- firefox/gecko (renders correctly and user interactions work)
- chrome/chromium/edge (renders correctly and user interactions work)
- safari/webkit (renders correctly and user interactions work)
- web page is readable and usable
- at 480px (mobile)
- at 640px (tablet)
- at 1024px (desktop)
| $this->cache->set( | ||
| "alert-polygon-" . $alert->id, | ||
| $output->geometry, | ||
| time() + 3600, // keep it for an hour |
| @@ -954,7 +931,7 @@ public static function getPlacesFromAlertDescription($alert) | |||
| // | |||
| // So if we have a line that matches, we should try parsing it. | |||
| preg_match( | |||
| "/IN \S+ THIS \S+ INCLUDES \d+ COUNTIES$/sim", | |||
| "/IN \S+ (THIS|THE NEW) \S+ INCLUDES \d+ COUNTIES$/sim", | |||
There was a problem hiding this comment.
Oh, oops. Good catch! One of the alerts had place information in it but the heading said IN THE NEW WATCH instead of our expected IN THIS WATCH. It looks like that alert has been replaced now, but this was to fix that and I totally forgot to mention it in the PR body.
There was a problem hiding this comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do? 🛠️
Instead of unioning all zone or county polygons into a single polygon, we now "collect" them into a single geometry collection and then geographically simplify it. Creating a union of the polygons was taking entirely too much time in the case where an alert covers several zones with complex geometries (for example, coastal boundaries).
This PR also caches the collected and simplified polygon based on alert ID. If we already have the computed polygon in cache, we return that immediately rather than rebuild it for every single request.