AFD HTML-only endpoints and selection implementation #1587
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
Apparently this cannot be used as the name for a Controller's handler function
-- What We now trim the AFD references for each WFO (in this case OKX and TAE) down to just 10 entries. We also then add each of product data files that are referenced. This last step is necessary because otherwise tests or examples would reach the live API to get the product by ID. The problem is that after some time these product IDs can no longer be found by the API. These changes will help prevent that inconsistency
-- What Now all the /wx/ routes should be rendering correctly. Additionally, we have fixed the "transclusion" that occurred by using the page \#theme render array value for the individual afd render template. We now have separate structures and references.
This is disabled by default (not loaded onto the page)
eric-gade
force-pushed
the
eg-1528-afd-html-endpoint
branch
from
2c86523
to
f456ae8
Compare
Merged
greg-does-weather
approved these changes
Aug 26, 2024
Comment on lines
+213
to
+215
| - name: dump drupal logs | ||
| if: always() | ||
| run: docker compose logs drupal |
There was a problem hiding this comment.
Suggested change
| - name: dump drupal logs | |
| if: always() | |
| run: docker compose logs drupal |
This is just for debugging. Not sure we want it always?
| { | ||
| $afd = $this->dataLayer->getProduct($id); | ||
| if ($afd) { | ||
| $afd = $this->dataLayer->getProduct($id); |
There was a problem hiding this comment.
Curious why this is called again. 🤔
Comment on lines
291
to
298
| if (!$afd || !array_key_exists("productText", $afd)) { | ||
| return null; | ||
| } | ||
| $rx = "/AFD(?<wfo>[A-Z]{3})/"; | ||
| if (preg_match($rx, $afd["productText"], $matches)) { | ||
| return $matches["wfo"]; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
We should switch to using the issuingOffice field. If that begins with a K, we strip of the K and use the remaining 3 letters. Otherwise, we'll use a lookup table provided by @coreypieper in Slack
| $versions = $this->weatherData->getLatestAFDReferences($wfo_code); | ||
| $afd = $this->weatherData->getAFDById($afd_id); | ||
| if (!$afd) { | ||
| // This is where you 404 |
There was a problem hiding this comment.
Curious if there's a reason not to go ahead and add the 404 here. I'll leave this alone and we can discuss next week.
| * Get the AFD viewer page with the given | ||
| * AFD and its WFO already populated | ||
| */ | ||
| public function byId($afd_id) |
There was a problem hiding this comment.
This function isn't referenced in the routing yaml. Gonna leave it alone and we can discuss when you're back!
| <h1>Area Forecast Discussion (AFD)</h1> | ||
| {% if wfo is not null %} | ||
| <h2>latest for {{wfo}}</h2> | ||
| <h2>Issued by {{wfo}} at {{afd.issuanceTime}}</h2> |
There was a problem hiding this comment.
Suggested change
| <h2>Issued by {{wfo}} at {{afd.issuanceTime}}</h2> | |
| <h2>Issued by {{wfo | upper}} at {{afd.issuanceTime}}</h2> |
Comment on lines
+43
to
+70
| async handleWFOSelectionUpdated(event){ | ||
| this.disableInputs(); | ||
| const wfoCode = event.target.value; | ||
| const afdSelectElement = document.getElementById('version-selector'); | ||
| const url = `/wx/afd/locations/${wfoCode}`; | ||
| const response = await fetch(url); | ||
| if(response.ok){ | ||
| afdSelectElement.innerHTML = ""; | ||
| const markup = await response.text(); | ||
| afdSelectElement.innerHTML = markup; | ||
| } | ||
| this.enableInputs(); | ||
| } | ||
|
|
||
| async handleAFDSelectionUpdated(event){ | ||
| this.disableInputs(); | ||
| const id = event.target.value; | ||
| const afdContainer = this.querySelector('.afd-content'); | ||
| if(!afdContainer){ | ||
| return; | ||
| } | ||
| const response = await fetch(`/wx/afd/${id}`); | ||
| if(response.ok){ | ||
| const markup = await response.text(); | ||
| afdContainer.outerHTML = markup; | ||
| } | ||
| this.enableInputs(); | ||
| } |
There was a problem hiding this comment.
This is so slick. Love it.
3 tasks
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? 🛠️
This PR mainly implements #1528, which called for a custom endpoint for HTML-only retrieval of AFD markup (designed to be used by a frontend component).
In the course of working on this ticket, some other changes were made -- see the next section.
This PR also includes a partial implementation of #1529, though it is "switched off" here
What does the reviewer need to know? 🤔
New routes
In order to facilitate the independent AFD viewer page with WFO and version switching, we need to add several routes and Drupal render-array
#themeconfigurations. Note that we are now using the practice of starting routes with/wx/if they retrieve data that is designed for consumption by frontend components (and not the general public).Here is a table describing the various routes and what they are for:
/afdwfoquerystring param, will redirect to the most recent for the given WFO. If passed anidquerystring param, it will redirect to the specific page for that AFD and corresponding WFO./afd/{wfo}/afd/{wfo}/{afd_id}/wx/afd/{id}/wx/afd/locations/{wfo}<options>elements representing the available AFD versions for the given WFO. This is designed for use on the frontend, in order to repopulate the versions select menu when the WFO has changedNon-JS functionality
This implementation does not make use of any JS or frontend components for dynamically loading AFD information, even though the routes are present and tested.
Instead, it uses basic form submission with querystrings to the
/afdroute described above. We did this in order to formally wire up the logic we would need anyway on the backend. Because of this change, we have added a submit button that is not in the original design. In consultation with @partly-igor , we are considering keeping this button even when we add the JS component, so that the interaction is clear.Bundling updates
We discovered an issue whereby the bundle data for products was incomplete. When getting AFD references for either the whole country (
/products/types/AFD) or a given WFO (/products/types/AFD/locations/OKX), there will be references to product IDs that will eventually expire and no longer be available via the API after some time. To deal with this, we've decided to limit the number of references in each of these files to just 10, and then to add all the AFDs for each reference to the bundle. That way we have all the information in perpetuity, and tests will not call out to the real API looking for data that likely doesn't exist anymore.Screenshots (if appropriate): 📸