Skip to content

Conversation

@ZeRego
Copy link
Collaborator

@ZeRego ZeRego commented Jul 18, 2023

Relates to: #6133
Closes: #6227

Description:

Changes:

  • amend knex table typing
  • update chart table to have a dashboard uuid column and space id column be nullable

Table before:

create table saved_queries
(
    saved_query_id                    integer generated always as identity
        primary key,
    saved_query_uuid                  uuid         default uuid_generate_v4() not null
        constraint saved_queries_saved_query_uuid_unique
            unique,
    created_at                        timestamp    default CURRENT_TIMESTAMP  not null,
    name                              text                                    not null,
    space_id                          integer                                 not null
        constraint saved_queries_space_id_foreign
            references spaces
            on delete cascade,
    description                       text,
    last_version_chart_kind           varchar(255) default 'vertical_bar'::character varying,
    last_version_updated_at           timestamp    default CURRENT_TIMESTAMP,
    last_version_updated_by_user_uuid uuid
        constraint saved_queries_last_version_updated_by_user_uuid_foreign
            references users (user_uuid)
            on delete set null
);

Table now:

create table saved_queries
(
    saved_query_id                    integer generated always as identity
        primary key,
    saved_query_uuid                  uuid         default uuid_generate_v4() not null
        constraint saved_queries_saved_query_uuid_unique
            unique,
    created_at                        timestamp    default CURRENT_TIMESTAMP  not null,
    name                              text                                    not null,
    space_id                          integer
        constraint saved_queries_space_id_foreign
            references spaces
            on delete cascade,
    description                       text,
    last_version_chart_kind           varchar(255) default 'vertical_bar'::character varying,
    last_version_updated_at           timestamp    default CURRENT_TIMESTAMP,
    last_version_updated_by_user_uuid uuid
        constraint saved_queries_last_version_updated_by_user_uuid_foreign
            references users (user_uuid)
            on delete set null,
    dashboard_uuid                    uuid
        constraint saved_queries_dashboard_uuid_foreign
            references dashboards (dashboard_uuid)
            on delete cascade
);

E2e test passing:

Screenshot 2023-07-18 at 15 43 46

Preview:

Dashboards.-.Lightdash.-.18.July.2023.mp4

@ZeRego ZeRego self-assigned this Jul 18, 2023
@netlify
Copy link

netlify bot commented Jul 18, 2023

Deploy Preview for peaceful-bassi-cbf284 canceled.

Name Link
🔨 Latest commit 5a74a33
🔍 Latest deploy log https://app.netlify.com/sites/peaceful-bassi-cbf284/deploys/64ba4f1a022c340008e9af0e

saved_query_id: undefined,
saved_query_uuid: undefined,
space_id: getNewSpace(d.space_id),
dashboard_uuid: null,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are going to support preview content in a following PR

@ZeRego ZeRego requested a review from a team July 18, 2023 14:57
@rephus
Copy link
Collaborator

rephus commented Jul 19, 2023

This branch has not been deployed

Can we get this in render ?

@ZeRego ZeRego force-pushed the feat/save-chart-in-dashboard branch from d5adabe to 4a1fe55 Compare July 20, 2023 09:51
"migrate-production": "knex migrate:latest --knexfile dist/database/knexfile.js",
"seed": "knex seed:run --knexfile src/database/knexfile.ts",
"seed-production": "knex seed:run --knexfile dist/database/knexfile.js",
"rollback-last": "knex migrate:rollback --knexfile src/database/knexfile.ts",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

knex migrate:rollback is to rollback the last batch of migrations. eg: If you migrated everything in 1 go, it would rollback everything.

We just want to rollback the last migration script.

@ZeRego ZeRego closed this Jul 20, 2023
@ZeRego ZeRego reopened this Jul 20, 2023
@owlas owlas requested a deployment to feat/save-chart-in-dashboard-duplicate - jaffle_db PR #6427 July 20, 2023 10:39 — with Render Abandoned
@owlas owlas deployed to feat/save-chart-in-dashboard-duplicate - headless-browser PR #6427 July 20, 2023 10:39 — with Render Active
@ZeRego ZeRego requested a review from rephus July 20, 2023 11:08
@rephus
Copy link
Collaborator

rephus commented Jul 20, 2023

Do we have this API dashboard twice because we run e2e tests twice ?

image

perhaps we want to delete it "at the beggining" of a new e2e test

@rephus
Copy link
Collaborator

rephus commented Jul 20, 2023

I can't edit the title or the content of a chart in dashboard

Screenshot from 2023-07-20 15-33-23

@rephus
Copy link
Collaborator

rephus commented Jul 20, 2023

I haven't tested scheduled delivieres for these charts, but I can see they can be previewed on /minimal, so I assume they work

@rephus
Copy link
Collaborator

rephus commented Jul 20, 2023

Interesting behaviour on these chart in dashboard options:

image

  • save chart as: creates a new chat that doens't belong to a dashboard, so it is browsable (kind of expected)
  • add to dashboard: you can add this chart to another dashboard, but I guess this will be deleted as well if the original dashboard gets deleted.
  • move to space: errors (tries to update the chart, see previous comment)

Copy link
Collaborator

@rephus rephus left a comment

Choose a reason for hiding this comment

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

I think the only thing we need to fix is the ability to update chart in dashboards. Everything else seems ok, or not a blocker.

@ZeRego
Copy link
Collaborator Author

ZeRego commented Jul 21, 2023

@rephus

Do we have this API dashboard twice because we run e2e tests twice ?

This may happen when the test retries.

perhaps we want to delete it "at the beggining" of a new e2e test

We already delete these dashboards and charts at the beginning of the tests.

I can't edit the title or the content of a chart in dashboard

Fixed chart summary query and added test.

Screenshot 2023-07-21 at 10 22 18

Interesting behaviour on these chart in dashboard options:

We have tickets in the milestone to disable some of these options. Shouldn't be an issue while the feature is under a feature flag.

@ZeRego ZeRego requested a review from rephus July 21, 2023 09:29
@owlas owlas temporarily deployed to feat/save-chart-in-dashboard-duplicate - headless-browser PR #6427 July 21, 2023 10:07 — with Render Destroyed
@owlas owlas temporarily deployed to feat/save-chart-in-dashboard-duplicate - lightdash PR #6427 July 21, 2023 10:07 — with Render Destroyed
Copy link
Collaborator

@rephus rephus left a comment

Choose a reason for hiding this comment

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

Everything works now. I'm happy about your comments.

Just 1 minor bug, we can fix it on a separate PR, or even disable the move to space option int he UI branch

If I move a chart in dashboard into another space (which doesn't make sense anyway) , the chart becomes "visible"

image

@ZeRego
Copy link
Collaborator Author

ZeRego commented Jul 21, 2023

If I move a chart in dashboard into another space (which doesn't make sense anyway) , the chart becomes "visible"

We will have the option to move charts from dashboards to the space. The BE will set the dashboard to null in that case. In your case we set the space uuid but didn't set the dashboarduuid to null causing that problem.

@ZeRego ZeRego merged commit 7a93c39 into main Jul 21, 2023
@ZeRego ZeRego deleted the feat/save-chart-in-dashboard branch July 21, 2023 11:59
lightdash-bot pushed a commit that referenced this pull request Jul 21, 2023
# [0.685.0](0.684.0...0.685.0) (2023-07-21)

### Features

* save chart in dashboard ([#6382](#6382)) ([7a93c39](7a93c39))
@lightdash-bot
Copy link
Collaborator

🎉 This PR is included in version 0.685.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

jayanand05 pushed a commit to jayanand05/lightdash that referenced this pull request Jul 21, 2023
* feat: save chart in dashboard

* chore: amend mock

* chore: amend mock

* chore: amend rollback last command

* fix: delete all charts in dashboard on rollback

* fix: amend chart summary query and add more tests
jayanand05 pushed a commit to jayanand05/lightdash that referenced this pull request Jul 21, 2023
jayanand05 pushed a commit to jayanand05/lightdash that referenced this pull request Aug 10, 2023
* feat: save chart in dashboard

* chore: amend mock

* chore: amend mock

* chore: amend rollback last command

* fix: delete all charts in dashboard on rollback

* fix: amend chart summary query and add more tests
jayanand05 pushed a commit to jayanand05/lightdash that referenced this pull request Aug 10, 2023
jayanand05 pushed a commit to jayanand05/lightdash that referenced this pull request Aug 10, 2023
* feat: save chart in dashboard

* chore: amend mock

* chore: amend mock

* chore: amend rollback last command

* fix: delete all charts in dashboard on rollback

* fix: amend chart summary query and add more tests
jayanand05 pushed a commit to jayanand05/lightdash that referenced this pull request Aug 10, 2023
jayanand05 pushed a commit to jayanand05/lightdash that referenced this pull request Nov 29, 2023
* feat: save chart in dashboard

* chore: amend mock

* chore: amend mock

* chore: amend rollback last command

* fix: delete all charts in dashboard on rollback

* fix: amend chart summary query and add more tests
jayanand05 pushed a commit to jayanand05/lightdash that referenced this pull request Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Charts created from within a dashboard do not appear in any chart lists in the app.

5 participants