aminghadersohi commented on PR #42659: URL: https://github.com/apache/superset/pull/42659#issuecomment-5372677408
@gabotorresruiz thanks for the approval, and for the two catches that got it here — both were real and both were reproducible exactly as you described. Recapping how each landed, since the branch was rebased after your first review and the line numbers have moved: **1. UUID `dashboardId` (`permalink.py`)** — you were right that `CreateDashboardPermalinkCommand` stores `str(dashboard.uuid)`, so the old `int(...)` comparison failed for essentially every real permalink. Implemented both options you offered rather than one: `get_matching_dashboard_permalink_state` now compares the reference against id, uuid **and** slug, and `DashboardLookupResult.resolved_from_permalink` marks the permalink-only path so it skips re-verification entirely. Slug is included so pre-3.1 permalinks keep working. Tests use a real UUID-string `dashboardId` and assert `filter_state` is present for both tools — `test_dashboard_tools.py:652` and `test_get_dashboard_layout.py:355` — plus a slug case at `test_dashboard_tools.py:679`. **2. Swallowed not-found error (`get_dashboard_info.py`)** — also confirmed. Both tools now keep the identifier's own error whenever `result is not None` and reserve the permalink wording for permalink-only requests (`get_dashboard_layout.py:104` has the matching guard you flagged). Locked in by `test_get_dashboard_info_unknown_slug_keeps_not_found_error` and `test_get_dashboard_layout_unknown_slug_keeps_not_found_error`, both asserting the typo string appears in the message. **Heads-up: I pushed one commit after your approval.** `2ada7dc8a3` is tests-only — it adds direct coverage for `lookup_dashboard_reference`, which a bot review correctly pointed out had no direct tests despite four branching paths. No source files changed in that commit. GitHub kept the approval, but flagging it explicitly so it is your call rather than mine — happy to revert it into a follow-up PR if you would rather approve exactly what you reviewed. Also worth noting for the record: this branch was rebased onto master after `#43202` landed, which removed `sanitize_for_llm_context` and rewrote `_apply_permalink_state`. Reconciled both rather than taking either side — details in the comment above. -- 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]
