[v0/v1 migration] Replace /v1/bulk/info/variable#6113
[v0/v1 migration] Replace /v1/bulk/info/variable#6113nick-nlb merged 5 commits intodatacommonsorg:masterfrom
Conversation
…ontend to remove fields that are no longer available.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the frontend to consume the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the variable_info API call from v1 to v2 and adjusts the corresponding endpoint configuration. It also involves a significant refactoring of the Stat Var Explorer UI, specifically removing the "Places" section and related components, and streamlining the "Provenance" section by removing "Release frequency" and "Place Types" information, along with associated CSS adjustments. There is no feedback to provide.
There was a problem hiding this comment.
Code Review
This pull request updates the /bulk/info/variable API endpoint from v1 to v2 and refactors the Stat Var Explorer UI. The 'Places' component and its related functionality have been removed, and the 'Provenance' component has been simplified by removing the display of releaseFrequency and 'Place Types'. CSS classes were also updated to reflect these UI changes. The review suggests removing releaseFrequency and placeTypeSummary from the ProvenanceSummary interface in static/js/shared/types.ts to maintain consistency and improve maintainability, recommending verification of their usage across the application before removal.
I am having trouble creating individual review comments. Click here to see my feedback.
static/js/tools/stat_var/provenance.tsx (66-68)
Since releaseFrequency is being removed from the UI, it would be good practice to also remove it from the ProvenanceSummary interface in static/js/shared/types.ts to keep the type definition in sync with the API response. The same applies to placeTypeSummary, which is also no longer used. However, please verify their usage across the entire application before removal, as they might be used in other features not directly related to the current changes. This can be done in a follow-up change to improve maintainability.
References
- Before suggesting to remove a function as dead code, verify its usage across the entire application, as it might be used in other features not directly related to the current changes (e.g., disaster maps, place search).
juliawu
left a comment
juliawu
left a comment
There was a problem hiding this comment.
LGTM, approved from ENG standpoint
n-h-diaz
left a comment
n-h-diaz
left a comment
There was a problem hiding this comment.
Thanks!
Not needed for this PR, but just wanted to call out
- I had removed release_frequency from the V2 response since it was tricky to fetch via BQ federation and possible to get these directly from V2 node instead. If we still want to display these, they can be fetched via "sourceReleaseFrequency" property, something like
https://api.datacommons.org/v2/node?key=<API_KEY>&nodes=dc/base/Argentina_Census&property=-%3EsourceReleaseFrequency (would this be okay?) - Let me know if we still want the per facet place types (it looks like we're dropping these from the display)
We definitely want the per facet place types! They're dropped from the display because the V2 endpoint clears them out (we talk about this a bit in the chat with @miss-o-soup). I've added this as a topic for when we meet later today. |
## Issue [b/459855424](https://buganizer.corp.google.com/issues/459855424) ## Related PR [6113](#6113) ## Description This PR does the following: * Restores v1 functionality in Flask, gating the v2 call under the migration feature flag * Restores v1 functionality on the frontend, likewise gating the v2 updates under the feature flag * As part of the frontend updates, it consolidates various disparate calls to the same endpoint into the same utility function (making it easier to apply the sending of flag overrides back to the backend). * Fixes the tooltip issue in V2 * Sets the migration flag to true in local, autopush and dev ## Notes Although the diff might look sizable, a lot of this functionality simply restores frontend V1 code removed in the linked PR. This will leave this PR functionally as a noop against prod. This code will be removed only once the functionality is fully deployed and the flags have been dropped. The actual new lines of code introduced here are few: * `static/js/tools/stat_var/explorer.tsx` Most changes here are just restoration of v1 frontend * `static/js/tools/stat_var/places.tsx` This whole file is just a restoration of v1 frontend * ## Testing [Stat Var Explorer](http://localhost:8080/tools/statvar#sv=Count_Person) Locally, by default, the v2 flag is set to be true. The above page should show the v2 version (i.e., with no overall place table at the top of the page). Additionally, the v2 endpoint should be called by flask (this can be verified by either reviewing the trimmed down payload that comes into the front end, or by adding logs to the Flask control flow. To verify that the flag override process works: * Set your local `use_v2_api` to false. * Restart your Flask server and refresh the page * You should now see the V1 version (where the place summary appears at the top and the tooltips show place types). * Add the flag override to the URL: [Stat Var Explorer with Flag Override](http://localhost:8080/tools/statvar?enable_feature=use_v2_api#sv=Count_Person) * You should now see the v2 version without the place overview section at the top and without the place types in the tooltip. Other pages that have been touched include the other tools (map, scatter, timeline), as these also have stat var hierarchies. These should show the correct tooltips in their respective feature flag mode as above. ## Screenshots See the related PR above for screenshots of what the v1 version and v2 version will look like. ## Autoreview Bot Gemini's comments all related to code that was not really new to this PR (but was restored with v1 functionality pending the migration feature flags being turned down). The code will be removed again later, and we should not do anything other than restore the working code as it was. I ignored the comments.
3 participants
Issue
b/459855424
Related PR
Mixer: 1818
🚨 The mixer PR must be merged in before this PR.
Description
This PR converts the usage of
/v1/bulk/info/variableto/v2/bulk/info/variable.Notes
With the conversion to
v2, certain fields (notably place related fields) have been dropped from the mixer response. Some of that functionality will be restored in a different way withv3; however, for thev2migration, we are just dropping those fields from the frontend.Once the
v2migration is complete and thev3endpoint is ready, we can bring in some of the information brought in by thev3endpoint. However, the priority for now is in completing thev1tov2migration.Testing
To test this PR, you must set your local mixer branch to the above related PR (and run the website against your local mixer).
Stat Var Explorer
Screenshots
Note the top place section is now removed, as well as the right-hand column in the series keys. WIth the later
v3migration, the right-hand column can be recreated (but will be different than whatv1provided).