Refactor editing dashboard's cards #29641

qnkhuat · 2023-03-29T09:26:22Z

Milestone 0 of #29502

Motivation

Currently, the APIs we used to create and delete dashcards are not bulked, which means if you edit a dashboard and add 5 cards to it, FE will calls POST /dashboard/cards 5 times.

That's not cool and we want to do bulk operations instead. So to do that we'll Remove

DELETE /api/dashboard/:id/cards
POST /api/dashboard/:id/cards

Update PUT /api/dashboard/:id/cards to handle bulks create/update/delete at the same time.
it takes a payload with keys cards

to create cards, include a card with a negative id
to delete cards, don't include that card in the payload
to update cards, update card information in the cards payload

FE PR (already merged onto this branch): #29762

Demo

Screen.Recording.2023-04-11.at.11.14.28.AM.mov

Adding and removing cards and confirming revision history is correct.

This change is

deploysentinel · 2023-03-29T09:53:47Z

No failed tests 🎉

codecov · 2023-03-29T15:50:52Z

Codecov Report

Patch coverage: 84.73% and project coverage change: +0.30 🎉

Comparison is base (2e4472c) 69.17% compared to head (e553c2f) 69.47%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #29641      +/-   ##
==========================================
+ Coverage   69.17%   69.47%   +0.30%     
==========================================
  Files        2806     2847      +41     
  Lines       97346    98946    +1600     
  Branches    12444    12539      +95     
==========================================
+ Hits        67336    68740    +1404     
- Misses      24669    24853     +184     
- Partials     5341     5353      +12

Flag	Coverage Δ
back-end	`86.28% <88.35%> (+0.08%)`	⬆️
front-end	`51.71% <72.72%> (+0.33%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...nd/src/metabase/dashboard/actions/data-fetching.js	`20.71% <0.00%> (ø)`
frontend/src/metabase/dashboard/actions/save.js	`36.11% <0.00%> (+6.56%)`	⬆️
frontend/src/metabase/dashboard/reducers.js	`39.75% <0.00%> (-0.49%)`	⬇️
frontend/src/metabase/services.js	`62.31% <ø> (+1.12%)`	⬆️
frontend/src/metabase/dashboard/actions/cards.js	`17.02% <50.00%> (+1.80%)`	⬆️
frontend/src/metabase/dashboard/actions/core.js	`86.20% <66.66%> (ø)`
src/metabase/models/dashboard.clj	`85.96% <76.92%> (+2.53%)`	⬆️
src/metabase/api/dashboard.clj	`86.83% <88.40%> (-0.69%)`	⬇️
frontend/src/metabase/lib/revisions/revisions.js	`97.77% <100.00%> (+0.27%)`	⬆️
src/metabase/models/dashboard_card.clj	`93.71% <100.00%> (-0.20%)`	⬇️
... and 1 more

... and 199 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

github-advanced-security · 2023-04-05T05:13:16Z

You have successfully added a new CodeQL configuration .github/workflows/codeql.yml:analyze. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

…tion end with exclaimation

…dashcards (#29762)

metamben

Reviewed 26 of 26 files at r13, 2 of 2 files at r14, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @alxnddr, @calherries, @EmmadUsmani, and @qnkhuat)

src/metabase/api/dashboard.clj line 521 at r5 (raw file):

Previously, calherries (Cal Herries) wrote…

How about diff-by-id, since it's basically doing a clojure.data.diff on the ids

I wasn't discussing the name of the function or its output. I was discussing the names current-change-ids and new-change-ids. Those sound like as if "change" were an entity of our domain, but I don't think they are, and so I have no intuition what they ought to mean.

src/metabase/api/dashboard.clj line 523 at r5 (raw file):

Previously, qnkhuat (Ngoc Khuat) wrote…

updated d894d8f

I would've used comp for to-create and to-update too.

qnkhuat · 2023-04-13T15:01:08Z

src/metabase/api/dashboard.clj line 541 at r14 (raw file):

Previously, calherries (Cal Herries) wrote…

Also would be nice to use dashcards instead of cards

updated all create/update/delete functions to have cards instead of dashcards now.

Code quote:

create-cards!

qnkhuat · 2023-04-13T15:04:16Z

test/metabase/api/dashboard_test.clj line 1198 at r14 (raw file):

Previously, calherries (Cal Herries) wrote…

TODO later? Or now?

later since it's not the scope of this PR. I think it's show up in the changes because I moved the fn.

qnkhuat · 2023-04-13T15:07:39Z

src/metabase/api/dashboard.clj line 521 at r5 (raw file):

Previously, metamben wrote…

I wasn't discussing the name of the function or its output. I was discussing the names current-change-ids and new-change-ids. Those sound like as if "change" were an entity of our domain, but I don't think they are, and so I have no intuition what they ought to mean.

I see, can you review the new changes? I did a /s/changes/items and use the comp + :id pattern now.

qnkhuat · 2023-04-13T15:07:48Z

src/metabase/api/dashboard.clj line 523 at r5 (raw file):

Previously, metamben wrote…

I would've used comp for to-create and to-update too.

updated in a67ad0e

qnkhuat · 2023-04-13T15:08:21Z

src/metabase/api/dashboard.clj line 535 at r14 (raw file):

Previously, calherries (Cal Herries) wrote…

Perhaps this is simpler to understand if you used clojure.data.diff on the :id.

e.g.
(let ...
[create-ids delete-ids update-ids] (diff new-change-ids current-change-ids)
...)

I thought about this, but I like the idea of having things return as a map automatically so the caller side can just destruct the map and apply operation on it.

qnkhuat · 2023-04-13T15:09:23Z

src/metabase/api/dashboard.clj line 540 at r14 (raw file):

Previously, calherries (Cal Herries) wrote…

How about renaming this to create-dashcards? I think it's passable to use cards instead of dashcards in a context when it's not ambiguous. But top-level functions lack context by themselves. If you have functions calling functions calling functions where everything is named card and not dashcard, I'd argue it slows down reading because you have to check whether a thing called card is really a card or a dashcard.

yes! thanks for reminding me. I did a /s/cards/dashcards for these create/update/delete methods a67ad0e

qnkhuat · 2023-04-13T15:09:27Z

src/metabase/api/dashboard.clj line 541 at r14 (raw file):

Previously, qnkhuat (Ngoc Khuat) wrote…

updated all create/update/delete functions to have cards instead of dashcards now.

a67ad0e

qnkhuat · 2023-04-13T15:09:33Z

src/metabase/api/dashboard.clj line 558 at r14 (raw file):

Previously, calherries (Cal Herries) wrote…

This had me confused for a second, thinking a card was somehow being transformed into a dashboardcard

a67ad0e

qnkhuat · 2023-04-13T15:09:40Z

src/metabase/api/dashboard.clj line 565 at r14 (raw file):

Previously, calherries (Cal Herries) wrote…

dashboard-cards will always be non-nil. You need to check if it's empty

yes a67ad0e

qnkhuat

Reviewable status: 48 of 50 files reviewed, 14 unresolved discussions (waiting on @alxnddr, @calherries, @EmmadUsmani, and @metamben)

src/metabase/util/malli/schema.clj line 53 at r14 (raw file):

(def NegativeInt
  "Schema representing an integer than must also be greater than zero."

Done.

metamben · 2023-04-13T20:44:46Z

src/metabase/api/dashboard.clj

@@ -539,24 +513,72 @@
                                         (assoc mapping :card-id (get dashcard-id->card-id dashcard-id)))]
    (check-parameter-mapping-permissions new-mappings)))

+(mu/defn ^:private classify-changes :- [:map
+                                        [:to-create [:sequential [:map [:id neg-int?]]]]


I'm so confused. We do have a nicer version of neg-int?, don't we?

metamben

Reviewed 1 of 3 files at r16, all commit messages.
Reviewable status: 48 of 50 files reviewed, 15 unresolved discussions (waiting on @alxnddr, @calherries, @EmmadUsmani, and @qnkhuat)

src/metabase/api/dashboard.clj line 521 at r5 (raw file):

Previously, qnkhuat (Ngoc Khuat) wrote…

I see, can you review the new changes? I did a /s/changes/items and use the comp + :id pattern now.

I'm seeing older versions, have some changes got lost? The changes you describe are great, but I can't see them in GH.

EmmadUsmani

Reviewable status: 48 of 50 files reviewed, 15 unresolved discussions (waiting on @alxnddr, @calherries, @EmmadUsmani, @metamben, and @qnkhuat)

src/metabase/api/dashboard.clj line 521 at r5 (raw file):

Previously, metamben wrote…

I'm seeing older versions, have some changes got lost? The changes you describe are great, but I can't see them in GH.

Oops I think that was my doing. I force pushed a new commit but I think I forgot to pull the updated branch with Ngoc's new changes before I did it. Ngoc should be able to just push that commit again on top of mine.

- update classify-changes to use items as name instead - update classify-changes to use remove + comp pattern - some docstring cleanup

qnkhuat · 2023-04-14T01:53:31Z

@metamben I've re-pushed the lost commits

metamben

Reviewable status: 48 of 50 files reviewed, 14 unresolved discussions (waiting on @alxnddr, @calherries, @EmmadUsmani, and @qnkhuat)

src/metabase/api/dashboard.clj line 523 at r5 (raw file):

Previously, qnkhuat (Ngoc Khuat) wrote…

updated in a67ad0e

Sorry, I wasn't clear. filter is better than remove unless you have to use not. That is

to-create          (filter (comp neg? :id) new-items)
to-update          (filter (comp pos? :id) new-items)

are better, IMHO.

BE: MVP to bulk create/delete APIs

eaed96a

qnkhuat added 2 commits March 29, 2023 17:50

tests fixes

8a3438c

test fixes

635ce33

qnkhuat added 6 commits March 29, 2023 22:58

use bulk cards insert everywhere

40fa70a

bulk everything -- add dashcards

2753b97

bulk everything for real now(previously only bulk for post/update cards)

470a9d8

test fixes

5656423

Overload POST/DELETE into PUT /dashboard/:id/cards

d0c7821

Merge branch 'master' into dashboard-bulk-create-delete

e79d7fc

qnkhuat changed the title ~~Bulk Create/Delete dashcards~~ Remove POST/DELETE dashcards endpoint and update PUT to handle CUD Mar 31, 2023

qnkhuat added 5 commits April 3, 2023 14:46

tests fixes

2deb9ed

rearrange tests

8cdab45

remove inline defs and fix kondo

32df5cf

fix can't prune cards

89eef9d

use malli for schema

4e422aa

qnkhuat added the .Team/Backend 🥞 label Apr 4, 2023

qnkhuat added 2 commits April 4, 2023 20:50

generalize the classify changes function

5f07941

Merge branch 'master' into dashboard-bulk-create-delete

36d83d1

qnkhuat changed the base branch from master to feature-dashboard-tabs April 5, 2023 05:08

optimize DB calls

d7bd103

qnkhuat force-pushed the dashboard-bulk-create-delete branch from a13541d to d7bd103 Compare April 5, 2023 15:14

qnkhuat changed the base branch from feature-dashboard-tabs to master April 5, 2023 15:14

qnkhuat added 2 commits April 6, 2023 19:47

makes sure we dissoc id when create cards and makes side-affects func…

ae3ee94

…tion end with exclaimation

makes sure we dissoc id when create cards and makes side-affects func…

5861ac5

…tion end with exclaimation

EmmadUsmani mentioned this pull request Apr 6, 2023

refactor saveDashboardAndCards thunk to use new PUT endpoint for dashcards #29762

Merged

1 task

refactor saveDashboardAndCards thunk to use new PUT endpoint for …

af798f3

…dashcards (#29762)

qnkhuat changed the title ~~Remove POST/DELETE dashcards endpoint and update PUT to handle CUD~~ Refactor editting dashboard's cards Apr 7, 2023

metamben approved these changes Apr 13, 2023

View reviewed changes

qnkhuat commented Apr 13, 2023

View reviewed changes

(hopefully) fix line_chart.cy.spec.js

e35cc07

EmmadUsmani force-pushed the dashboard-bulk-create-delete branch from a67ad0e to e35cc07 Compare April 13, 2023 17:26

metamben reviewed Apr 13, 2023

View reviewed changes

metamben requested changes Apr 13, 2023

View reviewed changes

EmmadUsmani reviewed Apr 13, 2023

View reviewed changes

- rename functions with cards -> dashcards

e553c2f

- update classify-changes to use items as name instead - update classify-changes to use remove + comp pattern - some docstring cleanup

qnkhuat requested a review from metamben April 14, 2023 04:42

qnkhuat enabled auto-merge (squash) April 14, 2023 05:22

metamben reviewed Apr 14, 2023

View reviewed changes

metamben approved these changes Apr 14, 2023

View reviewed changes

qnkhuat merged commit 2fcc70f into master Apr 14, 2023
92 of 93 checks passed

qnkhuat deleted the dashboard-bulk-create-delete branch April 14, 2023 14:27

qnkhuat mentioned this pull request Apr 21, 2023

Revision history erratically loads and removes historic changes #30030

Closed

alxnddr added .Team/PixelPolice 👮 labels May 15, 2023

qnkhuat mentioned this pull request May 23, 2023

Dashboard revision history revert bugs #6884

Closed

qnkhuat mentioned this pull request Sep 18, 2023

Missing one API #33918

Closed

nemanjaglumac mentioned this pull request Nov 23, 2023

[E2E] Re-enable previously quarantined pie chart null test #36077

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor editing dashboard's cards #29641

Refactor editing dashboard's cards #29641

qnkhuat commented Mar 29, 2023 •

edited by EmmadUsmani

deploysentinel bot commented Mar 29, 2023 •

edited

codecov bot commented Mar 29, 2023 •

edited

github-advanced-security bot commented Apr 5, 2023

metamben left a comment

qnkhuat commented Apr 13, 2023

qnkhuat commented Apr 13, 2023

qnkhuat commented Apr 13, 2023

qnkhuat commented Apr 13, 2023

qnkhuat commented Apr 13, 2023

qnkhuat commented Apr 13, 2023

qnkhuat commented Apr 13, 2023

qnkhuat commented Apr 13, 2023

qnkhuat commented Apr 13, 2023

qnkhuat left a comment

metamben Apr 13, 2023

metamben left a comment

EmmadUsmani left a comment

qnkhuat commented Apr 14, 2023

metamben left a comment

Refactor editing dashboard's cards #29641

Refactor editing dashboard's cards #29641

Conversation

qnkhuat commented Mar 29, 2023 • edited by EmmadUsmani

Motivation

Demo

deploysentinel bot commented Mar 29, 2023 • edited

codecov bot commented Mar 29, 2023 • edited

Codecov Report

github-advanced-security bot commented Apr 5, 2023

metamben left a comment

Choose a reason for hiding this comment

qnkhuat commented Apr 13, 2023

qnkhuat commented Apr 13, 2023

qnkhuat commented Apr 13, 2023

qnkhuat commented Apr 13, 2023

qnkhuat commented Apr 13, 2023

qnkhuat commented Apr 13, 2023

qnkhuat commented Apr 13, 2023

qnkhuat commented Apr 13, 2023

qnkhuat commented Apr 13, 2023

qnkhuat left a comment

Choose a reason for hiding this comment

metamben Apr 13, 2023

Choose a reason for hiding this comment

metamben left a comment

Choose a reason for hiding this comment

EmmadUsmani left a comment

Choose a reason for hiding this comment

qnkhuat commented Apr 14, 2023

metamben left a comment

Choose a reason for hiding this comment

qnkhuat commented Mar 29, 2023 •

edited by EmmadUsmani

deploysentinel bot commented Mar 29, 2023 •

edited

codecov bot commented Mar 29, 2023 •

edited