Skip to content
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
467a128
on dashboard api calls - take the id from the beginning of the slug, …
Aug 15, 2019
1e818f6
add dashboard id when showing links to dashboards
Aug 15, 2019
90ef7cc
change path to include new name when renaming dashboards
Aug 15, 2019
8392126
Merge branch 'master' into id-and-slug-for-dashboards
Aug 17, 2019
aba6e3b
move slug generation to backend
Aug 18, 2019
0ae13b0
redirect to new name after changing (this time with a proper promise)
Aug 18, 2019
0783351
oh right, we already have a slug function
Aug 18, 2019
1aedc44
add spec that makes sure that renamed dashboards are redirected to the
Aug 18, 2019
b7a7a55
use id-slug in all Cypress specs
Aug 19, 2019
a851402
move dashboards from /dashboard/:slug to /dashboards/:id-:name_as_slug
Aug 19, 2019
75b71d7
Merge branch 'master' into id-and-slug-for-dashboards
gabrieldutra Jun 26, 2020
6ea5b97
Update dashboard url as its name changes
gabrieldutra Jun 26, 2020
8e5f491
Update separator to be "/"
gabrieldutra Jun 26, 2020
3f47fb8
Update missing dashboard urls
gabrieldutra Jun 26, 2020
2649292
Update api not to depend on int id
gabrieldutra Jun 26, 2020
59a9143
Use '-' instead of '/' as separator and update Dashboard.get calls
gabrieldutra Jun 30, 2020
169200e
slug -> name_as_slug
gabrieldutra Jun 30, 2020
1c9bbbe
Keep slug urls on cypress
gabrieldutra Jun 30, 2020
2cf590b
Update route path
gabrieldutra Jul 1, 2020
49482d9
Use legacy attr for GET
gabrieldutra Jul 2, 2020
52041a3
Use getter for urlForDashboard
gabrieldutra Jul 2, 2020
2ccc4c1
Update dashboard url when loaded by slug
gabrieldutra Jul 2, 2020
08c14de
Update Dashboard routes to use id instead of slug
gabrieldutra Jul 2, 2020
d7728d1
Update Dashboard handler tests
gabrieldutra Jul 2, 2020
d166069
Update Cypress tests
gabrieldutra Jul 3, 2020
210a755
Merge branch 'master' into id-and-slug-for-dashboards
gabrieldutra Jul 3, 2020
7a8cb0e
Fix create new dashboard spec
gabrieldutra Jul 3, 2020
0957129
Merge branch 'master' into id-and-slug-for-dashboards
gabrieldutra Jul 6, 2020
eb82583
Use axios { params }
gabrieldutra Jul 6, 2020
fd26a9f
Drop Ternary operator
gabrieldutra Jul 13, 2020
6e68b91
Send updated slug directly in 'slug' attr
gabrieldutra Jul 13, 2020
2de74b3
Merge branch 'master' into id-and-slug-for-dashboards
gabrieldutra Jul 13, 2020
cd867a7
Update multiple urls Dashboard test name
gabrieldutra Jul 13, 2020
212d4b4
Update route names
gabrieldutra Jul 13, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion client/app/components/dashboards/CreateDashboardDialog.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { wrap as wrapDialog, DialogPropType } from "@/components/DialogWrapper";
import navigateTo from "@/components/ApplicationArea/navigateTo";
import recordEvent from "@/services/recordEvent";
import { policy } from "@/services/policy";
import { urlForDashboard } from "@/services/dashboard";
Copy link
Member

Choose a reason for hiding this comment

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

How about we transform the response we receive and add a URL field to it, so we don't have to import (or remember to use) urlForDashboard everywhere?


function CreateDashboardDialog({ dialog }) {
const [name, setName] = useState("");
Expand All @@ -27,7 +28,7 @@ function CreateDashboardDialog({ dialog }) {

axios.post("api/dashboards", { name }).then(data => {
dialog.close();
navigateTo(`dashboard/${data.slug}?edit`);
navigateTo(`${urlForDashboard(data)}?edit`);
});
recordEvent("create", "dashboard");
}
Expand Down
6 changes: 3 additions & 3 deletions client/app/components/queries/AddToDashboardDialog.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import List from "antd/lib/list";
import Icon from "antd/lib/icon";
import { wrap as wrapDialog, DialogPropType } from "@/components/DialogWrapper";
import { QueryTagsControl } from "@/components/tags-control/TagsControl";
import { Dashboard } from "@/services/dashboard";
import { Dashboard, urlForDashboard } from "@/services/dashboard";
import notification from "@/services/notification";
import useSearchResults from "@/lib/hooks/useSearchResults";

Expand Down Expand Up @@ -38,7 +38,7 @@ function AddToDashboardDialog({ dialog, visualization }) {

function addWidgetToDashboard() {
// Load dashboard with all widgets
Dashboard.get({ slug: selectedDashboard.slug })
Dashboard.get(selectedDashboard)
.then(dashboard => {
dashboard.addWidget(visualization);
return dashboard;
Expand All @@ -51,7 +51,7 @@ function AddToDashboardDialog({ dialog, visualization }) {
notification.success(
"Widget added to dashboard",
<React.Fragment>
<a href={`dashboard/${dashboard.slug}`} onClick={() => notification.close(key)}>
<a href={`${urlForDashboard(dashboard)}`} onClick={() => notification.close(key)}>
{dashboard.name}
</a>
<QueryTagsControl isDraft={dashboard.is_draft} tags={dashboard.tags} />
Expand Down
4 changes: 2 additions & 2 deletions client/app/pages/dashboards/DashboardList.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import ItemsTable, { Columns } from "@/components/items-list/components/ItemsTab
import CreateDashboardDialog from "@/components/dashboards/CreateDashboardDialog";
import Layout from "@/components/layouts/ContentWithSidebar";

import { Dashboard } from "@/services/dashboard";
import { Dashboard, urlForDashboard } from "@/services/dashboard";
import { currentUser } from "@/services/auth";
import routes from "@/services/routes";

Expand Down Expand Up @@ -46,7 +46,7 @@ class DashboardList extends React.Component {
Columns.custom.sortable(
(text, item) => (
<React.Fragment>
<a className="table-main-title" href={"dashboard/" + item.slug} data-test={item.slug}>
<a className="table-main-title" href={urlForDashboard(item)} data-test={item.slug}>
{item.name}
</a>
<DashboardTagsControl
Expand Down
20 changes: 16 additions & 4 deletions client/app/pages/dashboards/DashboardPage.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -144,36 +144,48 @@ DashboardComponent.propTypes = {
dashboard: PropTypes.object.isRequired, // eslint-disable-line react/forbid-prop-types
};

function DashboardPage({ dashboardSlug, onError }) {
function DashboardPage({ dashboardSlug, dashboardId, onError }) {
const [dashboard, setDashboard] = useState(null);
const onErrorRef = useRef();
onErrorRef.current = onError;

useEffect(() => {
Dashboard.get({ slug: dashboardSlug })
Dashboard.get({ id: dashboardId, slug: dashboardSlug })
.then(dashboardData => {
recordEvent("view", "dashboard", dashboardData.id);
setDashboard(dashboardData);
})
.catch(error => onErrorRef.current(error));
}, [dashboardSlug]);
}, [dashboardSlug, dashboardId]);

return <div className="dashboard-page">{dashboard && <DashboardComponent dashboard={dashboard} />}</div>;
}

DashboardPage.propTypes = {
dashboardSlug: PropTypes.string.isRequired,
dashboardSlug: PropTypes.string,
dashboardId: PropTypes.string,
onError: PropTypes.func,
};

DashboardPage.defaultProps = {
dashboardSlug: null,
dashboardId: null,
onError: PropTypes.func,
};

// route kept for backward compatibility
routes.register(
"Dashboards.ViewOrEdit",
routeWithUserSession({
path: "/dashboard/:dashboardSlug",
render: pageProps => <DashboardPage {...pageProps} />,
})
);

routes.register(
"Dashboards.ViewOrEditWithId",
routeWithUserSession({
path: "/dashboards/:dashboardId-(.*)?",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If - is optional - probably it should be inside of brackets as well

Copy link
Member

Choose a reason for hiding this comment

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

I thought so, I tried it in many ways, but this is the only way I got it to work as I wanted 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔

Copy link
Member

Choose a reason for hiding this comment

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

Worth adding a Cypress test that tries to load dashboard with id, id-, id-slug, id-random string.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When you move - to brackets it decides that (-.*)? is a pattern for dashboardId. This should work: /dashboards/:dashboardId([^-]+)(-.*)?

Copy link
Member

Choose a reason for hiding this comment

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

Worth adding a Cypress test that tries to load dashboard with id, id-, id-slug, id-random string.

Yep, that's actually on my plans :)

Copy link
Member

Choose a reason for hiding this comment

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

When you move - to brackets it decides that (-.*)? is a pattern for dashboardId.

Oh, I see, it's the same pattern for both indicating I want ath and deciding the attr pattern 🤦‍♂️... Thanks! :)

render: pageProps => <DashboardPage {...pageProps} />,
})
);
12 changes: 8 additions & 4 deletions client/app/pages/dashboards/hooks/useDashboard.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { useState, useEffect, useMemo, useCallback, useRef } from "react";
import { isEmpty, includes, compact, map, has, pick, keys, extend, every, get } from "lodash";
import notification from "@/services/notification";
import location from "@/services/location";
import { Dashboard, collectDashboardFilters } from "@/services/dashboard";
import { Dashboard, collectDashboardFilters, urlForDashboard } from "@/services/dashboard";
import { currentUser } from "@/services/auth";
import recordEvent from "@/services/recordEvent";
import { QueryResultError } from "@/services/query";
Expand All @@ -14,6 +14,7 @@ import ShareDashboardDialog from "../components/ShareDashboardDialog";
import useFullscreenHandler from "../../../lib/hooks/useFullscreenHandler";
import useRefreshRateHandler from "./useRefreshRateHandler";
import useEditModeHandler from "./useEditModeHandler";
import navigateTo from "@/components/ApplicationArea/navigateTo";

export { DashboardStatusEnum } from "./useEditModeHandler";

Expand Down Expand Up @@ -69,9 +70,12 @@ function useDashboard(dashboardData) {
data = { ...data, version: dashboard.version };
}
return Dashboard.save(data)
.then(updatedDashboard =>
setDashboard(currentDashboard => extend({}, currentDashboard, pick(updatedDashboard, keys(data))))
)
.then(updatedDashboard => {
setDashboard(currentDashboard => extend({}, currentDashboard, pick(updatedDashboard, keys(data))));
if (has(data, "name")) {
navigateTo(urlForDashboard(updatedDashboard), true);
}
Copy link
Member

Choose a reason for hiding this comment

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

What's this about? This is to update the URL when the dashboard is renamed?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, correct

})
.catch(error => {
const status = get(error, "response.status");
if (status === 403) {
Expand Down
4 changes: 2 additions & 2 deletions client/app/pages/home/Home.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { axios } from "@/services/axios";
import recordEvent from "@/services/recordEvent";
import { messages } from "@/services/auth";
import notification from "@/services/notification";
import { Dashboard } from "@/services/dashboard";
import { Dashboard, urlForDashboard } from "@/services/dashboard";
import { Query } from "@/services/query";
import routes from "@/services/routes";

Expand Down Expand Up @@ -119,7 +119,7 @@ function DashboardAndQueryFavoritesList() {
<FavoriteList
title="Favorite Dashboards"
resource={Dashboard}
itemUrl={dashboard => `dashboard/${dashboard.slug}`}
itemUrl={dashboard => urlForDashboard(dashboard)}
emptyState={
<p>
<span className="btn-favourite m-r-5">
Expand Down
4 changes: 3 additions & 1 deletion client/app/services/dashboard.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { currentUser } from "@/services/auth";
import location from "@/services/location";
import { cloneParameter } from "@/services/parameters";

export const urlForDashboard = ({ id, name_as_slug }) => `dashboards/${id}-${name_as_slug}`;

export function collectDashboardFilters(dashboard, queryResults, urlParams) {
const filters = {};
_.each(queryResults, queryResult => {
Expand Down Expand Up @@ -149,7 +151,7 @@ function transformResponse(data) {

const saveOrCreateUrl = data => (data.slug ? `api/dashboards/${data.slug}` : "api/dashboards");
const DashboardService = {
get: ({ slug }) => axios.get(`api/dashboards/${slug}`).then(transformResponse),
get: ({ id, slug }) => axios.get(`api/dashboards/${id || slug}`).then(transformResponse),
getByToken: ({ token }) => axios.get(`api/dashboards/public/${token}`).then(transformResponse),
save: data => axios.post(saveOrCreateUrl(data), data).then(transformResponse),
delete: ({ slug }) => axios.delete(`api/dashboards/${slug}`).then(transformResponse),
Expand Down
2 changes: 1 addition & 1 deletion redash/handlers/dashboards.py
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ def get(self, dashboard_slug=None):
:>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
models.Dashboard.get_by_id_or_slug_and_org, dashboard_slug, self.current_org
Copy link
Member

Choose a reason for hiding this comment

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

On a second thought, it's actually safer to have a different route for this 🤔, it can cause problems with existing /dashboard/:slug urls.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, see comment.

)
response = DashboardSerializer(
dashboard, with_widgets=True, user=self.current_user
Expand Down
15 changes: 14 additions & 1 deletion redash/models/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import numbers
import pytz

from sqlalchemy import distinct, or_, and_, UniqueConstraint
from sqlalchemy import distinct, or_, and_, UniqueConstraint, cast
from sqlalchemy.dialects import postgresql
from sqlalchemy.event import listens_for
from sqlalchemy.ext.hybrid import hybrid_property
Expand Down Expand Up @@ -1097,6 +1097,10 @@ class Dashboard(ChangeTrackingMixin, TimestampMixin, BelongsToOrgMixin, db.Model
def __str__(self):
return "%s=%s" % (self.id, self.name)

@property
def name_as_slug(self):
return utils.slugify(self.name)

@classmethod
def all(cls, org, group_ids, user_id):
query = (
Expand Down Expand Up @@ -1168,6 +1172,15 @@ def favorites(cls, user, base_query=None):
def get_by_slug_and_org(cls, slug, org):
return cls.query.filter(cls.slug == slug, cls.org == org).one()

@classmethod
def get_by_id_or_slug_and_org(cls, id_or_slug, org):
return (
cls.query.filter(
cast(cls.id, db.String) == id_or_slug, cls.org == org
).one_or_none()
or cls.query.filter(cls.slug == id_or_slug, cls.org == org).one()
Copy link
Member

Choose a reason for hiding this comment

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

This is likely to mess up the index performance (because of the casting to string).

If we redirect from the old /dashboard/ URL to the new /dashboards/ ones, it means we only need to support a GET request by slug and we know when it's just a slug vs. id (let's add a flag to the GET request to note when it's a slug, maybe ?legacy?). And we can convert all the other API calls to use IDs.

Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

This looks good to me, but need to keep in mind that all existing api usage outside of our Frontend will be broken.

)

@hybrid_property
def lowercase_name(self):
"Optional property useful for sorting purposes."
Expand Down
1 change: 1 addition & 0 deletions redash/serializers/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ def serialize_dashboard(obj, with_widgets=False, user=None, with_favorite_state=
d = {
"id": obj.id,
"slug": obj.slug,
"name_as_slug": obj.name_as_slug,
Copy link
Member

Choose a reason for hiding this comment

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

Why not return it as the slug value?

Copy link
Member

Choose a reason for hiding this comment

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

Backward compatibility reasons + the other routes needed the slug. So the delete button wouldn't work if you had changed the dashboard url for example (considering route as /api/dashboards/:slug as the api would rely in the actual "slug".

Copy link
Member

Choose a reason for hiding this comment

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

Now that we migrated all API routes to use an id, we can drop this?

Copy link
Member

Choose a reason for hiding this comment

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

Done 👍

"name": obj.name,
"user_id": obj.user_id,
"user": {
Expand Down