Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dashboard URL does not show new name when dashboard name is updated #1009

Open
wants to merge 10 commits into
base: master
from

Conversation

@rauchy
Copy link
Contributor

rauchy commented Aug 15, 2019

Before dashboard title change:
before_title_change

After dashboard title change:
after_title_change

@arikfr

This comment has been minimized.

Copy link
Member

arikfr commented Apr 23, 2016

This is sort of intentional to make sure the URL doesn't break. I see two possible solutions here:

  1. Stop using slugs for dashboard names and switch to ids as we do with queries, alerts, etc.
  2. If we want to keep the slugs, we should add the id of the dashboard to the name and do the lookup by the id and not the slug. I.e. if the dashboard id is 7, then the url will be /dashboard/7-cohort-analysis. Because the slug is meaningless for lookup we can keep it at sync with the actual title. This has other benefits such as removing the need to make sure the slug is unique (the id will guarantee this).
@arikfr arikfr added the Backend label Apr 23, 2016
@ChiragKParmar

This comment has been minimized.

Copy link
Contributor Author

ChiragKParmar commented May 4, 2016

Frankly, I am not sure which one would be better! I think second option is better in terms of giving ability to user to guess what the dashboard is for (from the slugs).

@arikfr

This comment has been minimized.

Copy link
Member

arikfr commented May 4, 2016

I agree. And because we have the "technology" it's not a big cost to keep slugs, while improving their implementation.

@arikfr arikfr added this to the Next milestone Mar 18, 2018
@susodapop

This comment has been minimized.

Copy link
Contributor

susodapop commented Jan 17, 2019

Related to this: the CSS title needs to update when the dashboard name is changed. I think fixing the title is independent of the URL slug. It should be fixed regardless what is done about the slugs.

Notice in the attached screenshot that the tab title doesn't match the dashboard title.

screen shot 2019-01-17 at 07 19 54

@arikfr

This comment has been minimized.

Copy link
Member

arikfr commented Jan 17, 2019

@susodapop worth opening a separate issue for this then. Probably an easy fix.

Copy link
Member

arikfr left a comment

Worth taking a moment and clearing the code a bit -- there are some places where we expect a dashboard ID, but call it slug in the code 😖

dashboard = get_object_or_404(models.Dashboard.get_by_slug_and_org, dashboard_slug, self.current_org)
if dashboard_slug.split('-')[0].isdigit():
identifier = int(dashboard_slug.split('-')[0])
fn = models.Dashboard.get_by_id_and_org

This comment has been minimized.

Copy link
@arikfr

arikfr Aug 15, 2019

Member

We have 185 dashboards in our database that match this pattern already ("28 Days Growth Metrics"), so old URLs will stop working. We need a fallback here.

Some ideas:

  • Try with get_by_slug_and_org if get_by_id_and_org fails.
  • Test if the slug matches the dashboards slug.

This comment has been minimized.

Copy link
@rauchy

rauchy Aug 17, 2019

Author Contributor

Preferred and implemented the first, but then realized that it breaks when you have a dashboard called "28 Days Growth Metrics" (available through /dashboards/28-days-growth-metrics) and then you create another dashboard called "I like trains" which gets assigned with id 28 (available through /dashboards/28-i-like-trains).

In that case, get_by_id_and_org wouldn't fail, and your previous links would break.

This comment has been minimized.

Copy link
@arikfr

arikfr Aug 18, 2019

Member

Why?

When you request 28-days... it will work with get_by_slug_and_org, and when you request 28-i-like... it will fail get_by_slug_and_org and then try get_by_id_and_org which will work.

This comment has been minimized.

Copy link
@rauchy

rauchy Aug 18, 2019

Author Contributor

Hehe, that's the opposite direction of what you suggested in the first option :)
Yeah, that might work, but it would add an extra query for most dashboard loads. I went ahead and implemented the second option which seems to work fine.

This comment has been minimized.

Copy link
@rauchy

rauchy Aug 18, 2019

Author Contributor

Rethinking it, there's no possible way I can think that we can remain agnostic to the slug if we don't do get_by_slug_and_org and fallback to get_by_id_and_org (i.e. if we don't go down this path, renaming dashboards would always break existing links). aba6e3b

This comment has been minimized.

Copy link
@arikfr

arikfr Aug 19, 2019

Member

Hehe, that's the opposite direction of what you suggested in the first option :)

😳 when I first read your reply I thought that's what happened, read my suggestion again but still parsed it in reverse order. 🤦‍♂

Yeah, that might work, but it would add an extra query for most dashboard loads. I went ahead and implemented the second option which seems to work fine.

🤔 how about the following:

Currently the dashboard URLs are /dashboard/<slug>. But they should actually be /dashboards/<id>-<whatever>. So what if as part of this change we will change the URLs and keep the old ones for backward compatibility.

This way if we get a request for /dashboard/... we know it's an old URL and we should pass a slug but if it's /dashboards/... we use the new logic?

This comment has been minimized.

Copy link
@rauchy

rauchy Aug 19, 2019

Author Contributor

👍 a851402 😈

.replace(/[^\w-]+/g, '')
.replace(/--+/g, '-')
.replace(/^-+/, '')
.replace(/-+$/, '')}`;

This comment has been minimized.

Copy link
@arikfr

arikfr Aug 15, 2019

Member

We should still create the slug in the backend, it will just update when you update the dashboard name.

This comment has been minimized.

Copy link
@rauchy

rauchy Aug 15, 2019

Author Contributor

Yeah, but that might break existing links, if anyone kept them.

My intention was to have existing slugs continue to work, while new links would be dashboards/123-pretty-much-the-name-of-the-dashboard-here-but-could-be-anything-youd-like-cause-we-only-care-about-the-id-from-now.

This comment has been minimized.

Copy link
@arikfr

arikfr Aug 15, 2019

Member

It doesn't have to:

  1. We will keep the current slug value in the database as is, so we can do lookups for existing dashboards.
  2. We will no longer update it, but compute it when returning the API response based on the current name.

This comment has been minimized.

Copy link
@rauchy

rauchy Aug 18, 2019

Author Contributor

name_as_slug 😬😬😬

@arikfr

This comment has been minimized.

Copy link
Member

arikfr commented on redash/models/__init__.py in aba6e3b Aug 19, 2019

  1. You should probably move this into redash.utils.slugify.
  2. What does it do differently?
  3. How does it handle unicode?
  4. Tests?
@rauchy

This comment has been minimized.

Copy link
Contributor

rauchy commented Aug 19, 2019

  1. You should probably move this into redash.utils.slugify.
  2. What does it do differently?
  3. How does it handle unicode?
  4. Tests?

I realized this after a few minutes 🤦‍♂

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.