fitzee commented on PR #43634: URL: https://github.com/apache/superset/pull/43634#issuecomment-5490861788
Addressed the review findings in `8f9ce164d6` and updated the PR description. **Changed** - Replaced the display-name map with a locale-sorted, bounded `viz_type_order` slug array; SQL orders by integer `CASE` ranks, then unknown slug. This removes registered-name collation drift and cuts request size/bind count. - Made the parameter an explicit Chart-list API contract via a chart-specific Rison schema, `get_list` override, OpenAPI documentation, and generated-spec regression test. Limits are 256 unique slugs and the model-aligned 250 characters per slug. - Truncate UI-generated arrays at 256 so plugin-heavy registries degrade through the unknown-slug fallback rather than 400. - Removed `columns` from the integration fixture to exercise the production outer-query branch; added asc/desc cross-page, unknown, empty, omitted, and non-Type-sort cases. - Added schema boundary coverage, frontend non-Type omission/cap coverage, refresh replay coverage, and reserved extra-query-key protection. ChartList also preserves any future non-reserved extras. - Clarified the inner-query primary-key tie breaker versus FAB's outer ordering in the PR body. **Declined after code inspection** - `refreshData` does not bypass the wrapper's result: the hook caches the already-enriched config (including extras) and replays it through its internal fetch. This is now pinned by a two-request test. - Hoisting the order to module initialization would freeze the registry before runtime plugin registration; it remains computed per Type request. - The logging concern does not apply to this route: the inherited FAB `ModelRestApi.get_list` has no `log_this_with_context`, and the chart-specific override intentionally preserves that behavior, so `collect_request_payload` is not invoked for Chart list requests. The `fix` title remains intentional because this is a corrective user-visible sorting repair; the checked API box documents the supporting query-contract addition rather than a standalone feature. Validation: 56 focused frontend tests, 37 schema tests, 3 focused Chart API integration tests, targeted frontend/Python lint, and the full staged pre-commit suite all pass. The added tests also address the earlier Codecov missing-branch report. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
