Skip to content

Feature/add url identifier routing - #63

Merged
imenattatra merged 4 commits into
metr-mainfrom
feature/add-url-identifier-routing
Mar 16, 2026
Merged

Feature/add url identifier routing#63
imenattatra merged 4 commits into
metr-mainfrom
feature/add-url-identifier-routing

Conversation

@imenattatra

Copy link
Copy Markdown

Closes https://github.com/orgs/metr-systems/projects/16?pane=issue&itemId=156558975&issue=metr-systems%7Cbacklog%7C4602

AI Disclaimer: Yes , i did use it mainly with tests

Summary

This PR closes the url identifier topic. It gives us the opportunity to be redirected to the dashboard we aim for, starting from a link that uses the url identifier.

For example : from https://dashboard.staging.metr.systems/staging/dashboards/by_url_identifier/sdasdasda you get redirected to https://dashboard.staging.metr.systems/staging/dashboards/71-test-t12

Code Strategy

We initially follow the code used for getting a dashboard by slug , so we add dashboard_by_url_identifier in statics file, later inside this function we make sure everything is handled there (on the backend side) we do nothing from frontend and we don't touch ressources.

A fix is added to the url identifier container in order to show the correct link that we can copy from the dashboard edit page and use directly.

Exception handling in dashboard_by_url_identifier is generic, in purpose. If the dashboard exists and is accessible we redirect to it otherwise we show 404, just to make things simple because anyway these cases are not supposed to happen. What do you think please?

QA

unit tests + on staging (you can test it there)

@imenattatra
imenattatra requested review from a team and helenalebreton March 10, 2026 15:26
@imenattatra imenattatra self-assigned this Mar 10, 2026

@thiagogds thiagogds left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to test in this new view for group access or

Comment thread redash/handlers/static.py Outdated
Comment thread redash/handlers/static.py Outdated
Comment on lines +46 to +47
except Exception:
raise NotFound()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you remove the require_dashboard_group_access, the only thing that could go wrong is finding the dashboard, then you can use a more specific exception.

Or, change the get_by_url_identifier_and_org to already raise a NotFound. Maybe call it get_by_url_identifier_and_org_or_404 to be closer to how we have it in Django.

My blocker here is using the except Exception, you can decide on the best solution

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, i updated it in the lates commit , can you please check it?

@helenalebreton

Copy link
Copy Markdown

@imenattatra it will seem like a bit of a daft question but ovbiously, it redirects to the dashobard 'tagged' with the url identifer within the same organisation, right ?

@imenattatra

Copy link
Copy Markdown
Author

@imenattatra it will seem like a bit of a daft question but ovbiously, it redirects to the dashobard 'tagged' with the url identifer within the same organisation, right ?

yes of course

@imenattatra
imenattatra requested a review from thiagogds March 11, 2026 15:40
@imenattatra
imenattatra merged commit cde4576 into metr-main Mar 16, 2026
1 check passed
@imenattatra
imenattatra deleted the feature/add-url-identifier-routing branch March 16, 2026 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants