From 467a1280f01c06324f0b74b539eec3ccc25b5643 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Thu, 15 Aug 2019 11:14:47 +0300 Subject: [PATCH 01/29] on dashboard api calls - take the id from the beginning of the slug, unless there is no number in it - in that case, take the entire slug as id --- redash/handlers/dashboards.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/redash/handlers/dashboards.py b/redash/handlers/dashboards.py index 954e9da4f7..6f11d01c8e 100644 --- a/redash/handlers/dashboards.py +++ b/redash/handlers/dashboards.py @@ -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 + 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) From 1e818f6202a00614fdf927e9c213b646b560afbd Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Thu, 15 Aug 2019 11:15:39 +0300 Subject: [PATCH 02/29] add dashboard id when showing links to dashboards --- client/app/pages/dashboards/DashboardList.jsx | 4 ++-- client/app/services/dashboard.js | 9 +++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/client/app/pages/dashboards/DashboardList.jsx b/client/app/pages/dashboards/DashboardList.jsx index bef99c2797..be4ef170e6 100644 --- a/client/app/pages/dashboards/DashboardList.jsx +++ b/client/app/pages/dashboards/DashboardList.jsx @@ -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) => ( - { item.name } + { item.name } `dashboard/${id}-${name.toString() + .trim() + .toLowerCase() + .replace(/\s+/g, '-') + .replace(/[^\w-]+/g, '') + .replace(/--+/g, '-') + .replace(/^-+/, '') + .replace(/-+$/, '')}`; + export function collectDashboardFilters(dashboard, queryResults, urlParams) { const filters = {}; _.each(queryResults, (queryResult) => { From 90ef7ccda0a904d75195554825f615ae6dc4a55b Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Thu, 15 Aug 2019 11:16:05 +0300 Subject: [PATCH 03/29] change path to include new name when renaming dashboards --- client/app/pages/dashboards/dashboard.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/client/app/pages/dashboards/dashboard.js b/client/app/pages/dashboards/dashboard.js index f283c3594b..227e46a171 100644 --- a/client/app/pages/dashboards/dashboard.js +++ b/client/app/pages/dashboards/dashboard.js @@ -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) => { From aba6e3beb5b38c6753a099f8d68fa0ffb842abd2 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Sun, 18 Aug 2019 23:19:22 +0300 Subject: [PATCH 04/29] move slug generation to backend --- client/app/services/dashboard.js | 9 +------ redash/handlers/dashboards.py | 9 +------ redash/models/__init__.py | 18 +++++++++++++- redash/serializers/__init__.py | 2 +- tests/models/test_dashboards.py | 40 ++++++++++++++++++++++++++++++++ 5 files changed, 60 insertions(+), 18 deletions(-) diff --git a/client/app/services/dashboard.js b/client/app/services/dashboard.js index 62c1df8dbd..82e9445a7f 100644 --- a/client/app/services/dashboard.js +++ b/client/app/services/dashboard.js @@ -4,14 +4,7 @@ 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(/-+$/, '')}`; +export const urlForDashboard = ({ id, slug }) => `dashboard/${id}-${slug}`; export function collectDashboardFilters(dashboard, queryResults, urlParams) { const filters = {}; diff --git a/redash/handlers/dashboards.py b/redash/handlers/dashboards.py index 6f11d01c8e..954e9da4f7 100644 --- a/redash/handlers/dashboards.py +++ b/redash/handlers/dashboards.py @@ -146,14 +146,7 @@ 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 """ - if dashboard_slug.split('-')[0].isdigit(): - identifier = int(dashboard_slug.split('-')[0]) - fn = models.Dashboard.get_by_id_and_org - else: - identifier = dashboard_slug - fn = models.Dashboard.get_by_slug_and_org - - dashboard = get_object_or_404(fn, identifier, self.current_org) + dashboard = get_object_or_404(models.Dashboard.get_by_slug_and_org, dashboard_slug, self.current_org) response = serialize_dashboard(dashboard, with_widgets=True, user=self.current_user) api_key = models.ApiKey.get_by_object(dashboard) diff --git a/redash/models/__init__.py b/redash/models/__init__.py index a4671e5824..3bc67aed82 100644 --- a/redash/models/__init__.py +++ b/redash/models/__init__.py @@ -3,6 +3,7 @@ import logging import time import pytz +from re import sub from six import python_2_unicode_compatible, text_type from sqlalchemy import distinct, or_, and_, UniqueConstraint @@ -867,6 +868,14 @@ class Dashboard(ChangeTrackingMixin, TimestampMixin, BelongsToOrgMixin, db.Model def __str__(self): return u"%s=%s" % (self.id, self.name) + @property + def name_as_slug(self): + return sub(r'-+$', '', + sub(r'^-+', '', + sub(r'--+', '-', + sub(r'[^\w-]+', '', + sub(r'\s+', '-', self.name.strip().lower()))))) + @classmethod def all(cls, org, group_ids, user_id): query = ( @@ -927,7 +936,14 @@ def favorites(cls, user, base_query=None): @classmethod def get_by_slug_and_org(cls, slug, org): - return cls.query.filter(cls.slug == slug, cls.org == org).one() + try: + return cls.query.filter(cls.slug == slug, cls.org == org).one() + except NoResultFound as e: + id = slug.split('-', 1)[0] if '-' in slug else slug + if id.isdigit(): + return super(Dashboard, cls).get_by_id_and_org(int(id), org) + else: + raise e @hybrid_property def lowercase_name(self): diff --git a/redash/serializers/__init__.py b/redash/serializers/__init__.py index 0dc8b6a9f9..84a549a147 100644 --- a/redash/serializers/__init__.py +++ b/redash/serializers/__init__.py @@ -207,7 +207,7 @@ def serialize_dashboard(obj, with_widgets=False, user=None, with_favorite_state= d = { 'id': obj.id, - 'slug': obj.slug, + 'slug': obj.name_as_slug, 'name': obj.name, 'user_id': obj.user_id, # TODO: we should properly load the users diff --git a/tests/models/test_dashboards.py b/tests/models/test_dashboards.py index cf9595a17d..82ecbdabbf 100644 --- a/tests/models/test_dashboards.py +++ b/tests/models/test_dashboards.py @@ -28,3 +28,43 @@ def test_all_tags(self): list(Dashboard.all_tags(self.factory.org, self.factory.user)), [(u'tag1', 3), (u'tag2', 2), (u'tag3', 1)] ) + + def test_gets_by_original_unhyphenated_slug(self): + expected = self.factory.create_dashboard(slug='hello') + actual = Dashboard.get_by_slug_and_org('hello', self.factory.org) + self.assertEqual(expected, actual) + + def test_gets_by_original_hyphenated_slug(self): + expected = self.factory.create_dashboard(slug='hello-world') + actual = Dashboard.get_by_slug_and_org('hello-world', self.factory.org) + self.assertEqual(expected, actual) + + def test_gets_by_number_name_as_slug(self): + expected = self.factory.create_dashboard(name='123') + actual = Dashboard.get_by_slug_and_org('123', self.factory.org) + self.assertEqual(expected, actual) + + def test_gets_by_id_and_single_word_name_as_slug(self): + expected = self.factory.create_dashboard(id=123, name='Hello') + actual = Dashboard.get_by_slug_and_org('123-hello', self.factory.org) + self.assertEqual(expected, actual) + + def test_gets_by_id_and_multi_word_name_as_slug(self): + expected = self.factory.create_dashboard(id=123, name='Hello World') + actual = Dashboard.get_by_slug_and_org('123-hello-world', self.factory.org) + self.assertEqual(expected, actual) + + def test_gets_by_original_slug_when_name_includes_a_number(self): + expected = self.factory.create_dashboard(id=123, name='28 Days Growth Metrics') + actual = Dashboard.get_by_slug_and_org('28-days-growth-metrics', self.factory.org) + self.assertEqual(expected, actual) + + def test_gets_by_id_when_name_includes_a_number(self): + expected = self.factory.create_dashboard(id=123, name='28 Days Growth Metrics') + actual = Dashboard.get_by_slug_and_org('123-28-days-growth-metrics', self.factory.org) + self.assertEqual(expected, actual) + + def test_gets_by_id_even_when_slug_doesnt_match_name(self): + expected = self.factory.create_dashboard(id=123, name='28 Days Growth Metrics') + actual = Dashboard.get_by_slug_and_org('123-i-like-trains', self.factory.org) + self.assertEqual(expected, actual) \ No newline at end of file From 0ae13b0fe976af655eca9f80a06d17d25a443624 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Sun, 18 Aug 2019 23:42:22 +0300 Subject: [PATCH 05/29] redirect to new name after changing (this time with a proper promise) --- client/app/pages/dashboards/dashboard.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/client/app/pages/dashboards/dashboard.js b/client/app/pages/dashboards/dashboard.js index 227e46a171..16aa3a9cce 100644 --- a/client/app/pages/dashboards/dashboard.js +++ b/client/app/pages/dashboards/dashboard.js @@ -277,7 +277,7 @@ function DashboardCtrl( this.loadTags = () => getTags('api/dashboards/tags').then(tags => _.map(tags, t => t.name)); - const updateDashboard = async (data) => { + const updateDashboard = (data) => { _.extend(this.dashboard, data); data = _.extend({}, data, { slug: this.dashboard.id, @@ -302,9 +302,10 @@ function DashboardCtrl( ); }; - this.saveName = async (name) => { - await updateDashboard({ name }); - $location.path(urlForDashboard(this.dashboard)); + this.saveName = (name) => { + updateDashboard({ name }).$promise.then(() => { + $location.path(urlForDashboard(this.dashboard)); + }); }; this.saveTags = (tags) => { From 07833515e339220ea59c9dd0e95b400e6becf481 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Mon, 19 Aug 2019 00:20:13 +0300 Subject: [PATCH 06/29] oh right, we already have a slug function --- redash/models/__init__.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/redash/models/__init__.py b/redash/models/__init__.py index 3bc67aed82..93209d59e5 100644 --- a/redash/models/__init__.py +++ b/redash/models/__init__.py @@ -3,7 +3,6 @@ import logging import time import pytz -from re import sub from six import python_2_unicode_compatible, text_type from sqlalchemy import distinct, or_, and_, UniqueConstraint @@ -870,11 +869,7 @@ def __str__(self): @property def name_as_slug(self): - return sub(r'-+$', '', - sub(r'^-+', '', - sub(r'--+', '-', - sub(r'[^\w-]+', '', - sub(r'\s+', '-', self.name.strip().lower()))))) + return utils.slugify(self.name) @classmethod def all(cls, org, group_ids, user_id): From 1aedc44098674efd2f853599b4c1312f19407598 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Mon, 19 Aug 2019 00:25:37 +0300 Subject: [PATCH 07/29] add spec that makes sure that renamed dashboards are redirected to the url which contains their new name --- client/app/pages/dashboards/dashboard.html | 3 +- .../integration/dashboard/dashboard_spec.js | 35 +++++++++++++++---- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/client/app/pages/dashboards/dashboard.html b/client/app/pages/dashboards/dashboard.html index cc89ae3e61..7d88e9d08c 100644 --- a/client/app/pages/dashboards/dashboard.html +++ b/client/app/pages/dashboards/dashboard.html @@ -3,7 +3,7 @@

- +

{{$ctrl.dashboard.user.name}} @@ -33,6 +33,7 @@

Saved