Feature/refactor allowed widgets - #68
Merged
Merged
Conversation
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.
Closes https://github.com/metr-systems/backlog/issues/4778
AI Disclaimer: Yes. Claude Code was used to plan and assist
Summary
Dashboards can now reference the query that has their allowed-widgets mapping by a stable, org-unique identifier instead of relying on the naming convention
allowed_widgets_{dashboard_id}.Previously, the only way a dashboard could declare which widgets are allowed per parameter value was to create a query literally named
allowed_widgets_<dashboard_id>. This would limit us when working with global redash since we need to be able to link the dynamic widgets query to many sub-dashboard of the final composed dashbaord.This PR introduces:
MetrQuerytable (mirroring the existingMetrDashboardpattern) carrying aquery_identifierstring that is unique per org.allowed_widget_query_identifiercolumn onMetrDashboard.MetrDashboard.allowed_widget_query_identifier→MetrQuery.query_identifier(org-scoped) → underlyingQuery→ itslatest_query_data, and builds the same{parameter_value: [widget_ids]}mapping as before.allowed_widgets_{dashboard_id}name lookup, so existing dashboards keep working with no migration.The Redash
QueryandDashboardschemas are left untouched — all new state lives on theMetr*tables, so upstream merges shouldn't conflict.It also fixes a pre-existing bug: requesting a dashboard via its legacy slug URL (
?legacy) used to pass a string into an integer-column filter and 500 withinvalid input syntax for type integer. The decorator now resolves the integer id from the serialized result.Code Strategy
query_identifierlives in newMetrQuerytable rather than inQueryitself, exactly mirroring howMetrDashboard.url_identifierworks. This keeps upstream models clean and reuses an established pattern reviewers already know.get_allowed_widgets_info()was modified to have the new resolution strategy.MetrQuerylookup is filtered byorg_idpreventing any cross-org leakage of widget mappings.allowed_widget_query_identifieris selected from a dropdown populated by a new org-scopedGET /api/queries/query_identifiersendpoint, so there's no server-side existence check in the main POST — mirroring howurl_identifieris handled (dedicated/validateendpoint for the query side, main POST trusts client-picked values). Stale/missing identifiers degrade gracefully via the resolver fallback.MetrQuery.queryis a relationship attribute, so it shadows the default SQLAlchemy model .query proxy. Because of that, we usedb.session.query(MetrQuery)for lookups to make it explicit that we are querying theMetrQuerymodel, not accessing the relationship field.Extra notes:
@add_allowed_widgets_infodecorator is still not applied toPublicDashboardResource; public dashboards are out of the scope.query_identifiercan be assigned. The identifier field is hidden entirely for unsaved queries and appears only once the query has been saved. Additionally, the identifier is write-once from the UI: once set it renders read-only (cleared/changed only via the API).QA
How did you test it? Keep your future self in mind to help debug things later.