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
@@ -15,7 +15,7 @@ import ItemsTable, { Columns } from '@/components/items-list/components/ItemsTab

import Layout from '@/components/layouts/ContentWithSidebar';

import { Dashboard } from '@/services/dashboard';
import { Dashboard, urlForDashboard } from '@/services/dashboard';
import { routesToAngularRoutes } from '@/lib/utils';

import DashboardListEmptyState from './DashboardListEmptyState';
@@ -45,7 +45,7 @@ class DashboardList extends React.Component {
Columns.favorites({ className: 'p-r-0' }),
Columns.custom.sortable((text, item) => (
<React.Fragment>
<a className="table-main-title" href={'dashboard/' + item.slug} data-test={item.slug}>{ item.name }</a>
<a className="table-main-title" href={urlForDashboard(item)} data-test={item.slug}>{ item.name }</a>
<DashboardTagsControl
className="d-block"
tags={item.tags}
@@ -6,7 +6,7 @@ import {
editableMappingsToParameterMappings,
synchronizeWidgetTitles,
} from '@/components/ParameterMappingInput';
import { collectDashboardFilters } from '@/services/dashboard';
import { collectDashboardFilters, urlForDashboard } from '@/services/dashboard';
import { durationHumanize } from '@/filters';
import template from './dashboard.html';
import ShareDashboardDialog from './ShareDashboardDialog';
@@ -277,13 +277,13 @@ function DashboardCtrl(

this.loadTags = () => getTags('api/dashboards/tags').then(tags => _.map(tags, t => t.name));

const updateDashboard = (data) => {
const updateDashboard = async (data) => {
_.extend(this.dashboard, data);
data = _.extend({}, data, {
slug: this.dashboard.id,
version: this.dashboard.version,
});
Dashboard.save(
return Dashboard.save(
data,
(dashboard) => {
_.extend(this.dashboard, _.pick(dashboard, _.keys(data)));
@@ -302,8 +302,9 @@ function DashboardCtrl(
);
};

this.saveName = (name) => {
updateDashboard({ name });
this.saveName = async (name) => {
await updateDashboard({ name });
$location.path(urlForDashboard(this.dashboard));
};

this.saveTags = (tags) => {
@@ -4,6 +4,15 @@ import { Widget } from './widget';

export let Dashboard = null; // eslint-disable-line import/no-mutable-exports

export const urlForDashboard = ({ id, name }) => `dashboard/${id}-${name.toString()
.trim()
.toLowerCase()
.replace(/\s+/g, '-')
.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 😬😬😬


export function collectDashboardFilters(dashboard, queryResults, urlParams) {
const filters = {};
_.each(queryResults, (queryResult) => {
@@ -146,7 +146,14 @@ def get(self, dashboard_slug=None):
:>json string widget.created_at: ISO format timestamp for widget creation
:>json string widget.updated_at: ISO format timestamp for last widget modification
"""
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 😈

else:
identifier = dashboard_slug
fn = models.Dashboard.get_by_slug_and_org

dashboard = get_object_or_404(fn, identifier, self.current_org)
response = serialize_dashboard(dashboard, with_widgets=True, user=self.current_user)

api_key = models.ApiKey.get_by_object(dashboard)
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.