feat: Shows user charts by default when editing a dashboard #23547

michael-s-molina · 2023-03-31T19:51:33Z

SUMMARY

#20684 introduced a change to allow a user to add charts owned/created by others to their dashboards. While this is a reasonable use case, the way it was implemented was to always display all charts accessible to the user which resulted in problems for organizations that contain many charts. From this comment, we can see some of the user feedback:

To me the experience is degraded, because almost all charts I add are created by me, and there’s a clear candidate for the next chart to add, i.e., it’s the chart most recently created by me.

Of course it’s good to be able to add charts created by others, but I would think that maybe this could be visible through a toggle (default is to show charts created by me, with toggle in “on” stage for “filter to my charts”).

This change of removing the filter might be an improvement in a small company, but in a big place it’s very unlikely that charts by others are useful to me, there’s just too many.

This PR modifies the dashboard editor to show by default only the charts owned or created by the user and gives the user an option to show other charts that are accessible to them. The reason for showing user charts by default is because it's more likely they will interact with their own charts and also for performance reasons, as we don't need to load all the metadata associated with a big number of charts upfront.

This PR also:

Rewrites actions/sliceEntities using Typescript to improve type checking and also simplify its logic
Removes items loaded in the Redux store after the user leaves editing mode

While developing, it was clear that we should also rewrite components/SliceAdder to simplify its logic and get rid of the react-virtualized-auto-sizer and react-search-input dependencies but I'll leave this to a follow up to simplify the review process.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2023-03-31.at.16.28.08.mov

TESTING INSTRUCTIONS

1 - Edit a dashboard
2 - Make sure it displays by default only charts owned or created by the user
3 - Select the option to view other charts
4 - Make sure it displays all charts accessible to the user

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

michael-s-molina · 2023-03-31T19:53:50Z

@kasiazjc Can you please review the proposed control? My thinking was to treat it as another filter, hence the checkbox. Any improvements are welcome 🙂

villebro

I agree with the proposed change - having this checkbox will give us the best of both worlds. Curious to hear thoughts on the wording here, as I think it's important to be as precise as possible.

superset-frontend/src/dashboard/components/SliceAdder.jsx

kasiazjc · 2023-04-03T08:26:07Z

@kasiazjc Can you please review the proposed control? My thinking was to treat it as another filter, hence the checkbox. Any improvements are welcome 🙂

Thanks @michael-s-molina ❤️

I agree this is a good change! In general I think that for now the checkbox makes sense - I was thinking that at some point we could have full on filters that would be hidden in a button (like "more filters" in horizontal navbar), so that you could filter by owner, dataset etc. But it's a long way to go.

In terms of styling:

let's align the checkbox to the left
(I think) let's bump the spacings between search and checkbox and thumbnails to 16px to give it more breathing space

In terms of behavior:
I am wondering if having a control that would say "Show my charts only" wouldn't be a better /more intuitive approach as we would start with the big list with all the possibilities, that you just narrow with the filter checkbox. We could also add a tooltip with information about this whole thing "By default, the chart list displays all charts that you have access to in the workspace. You can filter the list to show only charts that you own."

Also - is the selection you made in the checkbox saved for the future or you have to click each time?

Curious to hear your thoughts @michael-s-molina @villebro 😌

michael-s-molina · 2023-04-03T11:56:09Z

In terms of styling:

let's align the checkbox to the left

(I think) let's bump the spacings between search and checkbox and thumbnails to 16px to give it more breathing space

Will do!

In terms of behavior: I am wondering if having a control that would say "Show my charts only" wouldn't be a better /more intuitive approach as we would start with the big list with all the possibilities, that you just narrow with the filter checkbox.

I think "Show my charts only" is an interesting suggestion and maybe even easier to understand than "Show all charts I have access to". @john-bodley did a similar suggestion but in his scenario, "Show only my charts" would be selected by default and the user could uncheck it. The advantage is this point:

The reason for showing user charts by default is because it's more likely they will interact with their own charts and also for performance reasons, as we don't need to load all the metadata associated with a big number of charts upfront.

We could also add a tooltip with information about this whole thing "By default, the chart list displays all charts that you have access to in the workspace. You can filter the list to show only charts that you own."

Great idea. The tooltip will be helpful.

Also - is the selection you made in the checkbox saved for the future or you have to click each time?

Currently, it's not persisted but I think it's a great idea because it would allow us to default to user charts but give them the option to change this default.

@villebro @kasiazjc @john-bodley In summary, are you all ok with changing the checkbox to "Show only my charts" , which is checked by default, has a tooltip, and persists its state in the local storage so users can change it?

kasiazjc · 2023-04-03T12:00:40Z

In terms of styling:

let's align the checkbox to the left

(I think) let's bump the spacings between search and checkbox and thumbnails to 16px to give it more breathing space

Will do!

In terms of behavior: I am wondering if having a control that would say "Show my charts only" wouldn't be a better /more intuitive approach as we would start with the big list with all the possibilities, that you just narrow with the filter checkbox.

I think "Show my charts only" is an interesting suggestion and maybe even easier to understand than "Show all charts I have access to". @john-bodley did a similar suggestion but in his scenario, "Show my charts only" would be selected by default and the user could uncheck it. The advantage is this point:

The reason for showing user charts by default is because it's more likely they will interact with their own charts and also for performance reasons, as we don't need to load all the metadata associated with a big number of charts upfront.

We could also add a tooltip with information about this whole thing "By default, the chart list displays all charts that you have access to in the workspace. You can filter the list to show only charts that you own."

Great idea. The tooltip will be helpful.

Also - is the selection you made in the checkbox saved for the future or you have to click each time?

Currently, it's not persisted but I think it's a great idea because it would allow us to default to user charts but give them the option to change this default.

@villebro @kasiazjc @john-bodley In summary, are you all ok with changing the checkbox to "Show my charts only" , which is checked by default, has a tooltip, and persists its state in the local storage so users can change it?

I think it makes sense! Thank you @michael-s-molina 😌

codecov · 2023-04-03T14:52:47Z

Codecov Report

Merging #23547 (08058cb) into master (bc2ec04) will increase coverage by 0.00%.
The diff coverage is 65.12%.

❗ Current head 08058cb differs from pull request most recent head bd7741d. Consider uploading reports for the commit bd7741d to get more accurate results

@@           Coverage Diff            @@
##           master   #23547    +/-   ##
========================================
  Coverage   67.68%   67.69%            
========================================
  Files        1914     1916     +2     
  Lines       73958    74061   +103     
  Branches     8029     8052    +23     
========================================
+ Hits        50060    50132    +72     
- Misses      21852    21878    +26     
- Partials     2046     2051     +5

Flag	Coverage Δ
javascript	`53.89% <57.62%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
...ackages/superset-ui-core/src/query/types/Filter.ts	`100.00% <ø> (ø)`
...nd/plugins/plugin-chart-table/src/controlPanel.tsx	`53.03% <0.00%> (-0.82%)`	⬇️
...nd/src/components/Chart/MenuItemWithTruncation.tsx	`100.00% <ø> (ø)`
...rset-frontend/src/components/Chart/chartReducer.ts	`26.00% <0.00%> (+0.50%)`	⬆️
...d/components/DashboardBuilder/DashboardBuilder.tsx	`74.00% <ø> (-0.26%)`	⬇️
.../nativeFilters/FilterBar/CrossFilters/selectors.ts	`9.09% <0.00%> (-0.91%)`	⬇️
...rc/dashboard/components/nativeFilters/selectors.ts	`58.26% <ø> (+0.90%)`	⬆️
...t-frontend/src/dashboard/containers/SliceAdder.jsx	`0.00% <ø> (ø)`
...t-frontend/src/dashboard/reducers/nativeFilters.ts	`39.28% <0.00%> (-1.46%)`	⬇️
...t-frontend/src/dashboard/reducers/sliceEntities.js	`80.00% <0.00%> (-8.89%)`	⬇️
... and 41 more

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

michael-s-molina · 2023-04-03T16:06:06Z

/testenv up

github-actions · 2023-04-03T16:08:04Z

@michael-s-molina Ephemeral environment spinning up at http://34.220.124.153:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

michael-s-molina · 2023-04-03T16:10:19Z

@kasiazjc @villebro @john-bodley I implemented the suggestions and spun a test environment.

superset-frontend/src/dashboard/components/SliceAdder.jsx

kasiazjc · 2023-04-04T17:28:37Z

@kasiazjc @villebro @john-bodley I implemented the suggestions and spun a test environment.

Thanks! for some reason test env doesn't work for me, nor @yousoph, not sure why :( could you add a screenshot?

michael-s-molina · 2023-04-04T17:42:34Z

Thanks! for some reason test env doesn't work for me, nor @yousoph, not sure why :( could you add a screenshot?

I'm not sure why the test env is not working 😢 but here's a video showing the feature:

Screen.Recording.2023-04-04.at.14.38.26.mov

kgabryje · 2023-04-04T17:46:22Z

superset-frontend/cypress-base/cypress/integration/dashboard/editmode.test.ts

@@ -751,8 +751,8 @@ describe('Dashboard edit', () => {
      cy.getBySel('dashboard-component-chart-holder').should('have.length', 1);
    });

-    it('should remove added charts', () => {
-      dragComponent('Pivot Table');
+    it.only('should remove added charts', () => {


only 🙈

superset-frontend/src/dashboard/actions/sliceEntities.ts

justinpark · 2023-04-04T18:45:15Z

superset-frontend/src/dashboard/components/SliceAdder.jsx

+        showOnlyMyCharts,
+      ),
+    }));
+    setItem(


it won't store the value when showOnlyMyCharts is true

Did you mean, the first time? If that's the case, it's not a problem because the constructor defaults to true.

showOnlyMyCharts: getItem( LocalStorageKeys.dashboard__editor_show_only_my_charts, true, ),

oh my mistake. I understood this operation under if (!showOnlyMyCharts) { ... } but it's not. it looks good

michael-s-molina · 2023-04-05T19:19:54Z

@kasiazjc @kgabryje Is it ok to merge the PR?

kasiazjc · 2023-04-06T10:42:36Z

@kasiazjc @kgabryje Is it ok to merge the PR?

Thank you for the video 😌 I had one other change that I was sure I have posted, but haven't - sorry!

Because the filter will be persisted now I'm thinking about changing the copy to something like:

You can choose to display all charts that you have access to in the workspace or only the ones you own. Your filter selection will be saved and remain active until you choose to change it.

wdyt? otherwise I think it's good to go

michael-s-molina · 2023-04-06T11:39:47Z

Because the filter will be persisted now I'm thinking about changing the copy to something like:

You can choose to display all charts that you have access to in the workspace or only the ones you own. Your filter selection will be saved and remain active until you choose to change it.

I like it. I'll only remove the workspace term because it's a Preset thing 😉

michael-s-molina · 2023-04-06T11:47:26Z

@rusackas @eschutho @jinghua-qa @betodealmeida @geido Can any of you, Cypress owners, stamp it so I can merge the PR?

geido

E2E tests approved. Thanks!

github-actions · 2023-04-06T13:10:59Z

Ephemeral environment shutdown and build artifacts deleted.

…3547) (cherry picked from commit bccd267)

qlands · 2023-04-27T23:45:29Z

@michael-s-molina Would it be possible to apply this PR in Branch 2.1? I would really appreciate it.

michael-s-molina · 2023-04-28T11:41:03Z

@michael-s-molina Would it be possible to apply this PR in Branch 2.1? I would really appreciate it.

@qlands @rusackas Given that this is a feature, it should be added to 2.2 or 3.0 according to semantic versioning.

qlands · 2023-04-28T13:14:22Z

@michael-s-molina I understand, however, this feature is so important and missing in 2.X that it would help many deployments if merged sooner into 2.X. I will try to merge it on my own fork for now—many thanks for the great work.

eschutho · 2023-05-12T23:05:55Z

3.0 will be right on the heels of 2.1.1, so we'll be sure to get this into that major release.

feat: Shows user charts by default when editing a dashboard

785f26e

michael-s-molina requested review from rusackas, justinpark, john-bodley and kgabryje March 31, 2023 19:51

pull-request-size bot added the size/XL label Mar 31, 2023

michael-s-molina requested a review from villebro March 31, 2023 19:54

villebro reviewed Mar 31, 2023

View reviewed changes

superset-frontend/src/dashboard/components/SliceAdder.jsx Outdated Show resolved Hide resolved

Fixes tests

59f75c1

michael-s-molina requested review from jinghua-qa, geido, eschutho and betodealmeida as code owners April 3, 2023 13:33

Addresses comments

7b53fcc

justinpark reviewed Apr 4, 2023

View reviewed changes

superset-frontend/src/dashboard/components/SliceAdder.jsx Show resolved Hide resolved

Inverts input message

d22c8f9

kgabryje reviewed Apr 4, 2023

View reviewed changes

Removes only

fe4f17f

justinpark reviewed Apr 4, 2023

View reviewed changes

superset-frontend/src/dashboard/actions/sliceEntities.ts Outdated Show resolved Hide resolved

justinpark reviewed Apr 4, 2023

View reviewed changes

Uses reduce

92a5600

justinpark approved these changes Apr 5, 2023

View reviewed changes

Changes copy

bd7741d

geido approved these changes Apr 6, 2023

View reviewed changes

michael-s-molina merged commit bccd267 into apache:master Apr 6, 2023

michael-s-molina added a commit to airbnb/superset-fork that referenced this pull request Apr 7, 2023

feat: Shows user charts by default when editing a dashboard (apache#2…

ea505ae

…3547) (cherry picked from commit bccd267)

rusackas added the 2.1.1 label Apr 28, 2023

eschutho removed the 2.1.1 label May 12, 2023

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Shows user charts by default when editing a dashboard #23547

feat: Shows user charts by default when editing a dashboard #23547

michael-s-molina commented Mar 31, 2023

michael-s-molina commented Mar 31, 2023

villebro left a comment

kasiazjc commented Apr 3, 2023 •

edited

Loading

michael-s-molina commented Apr 3, 2023 •

edited

Loading

kasiazjc commented Apr 3, 2023

codecov bot commented Apr 3, 2023 •

edited

Loading

michael-s-molina commented Apr 3, 2023

github-actions bot commented Apr 3, 2023

michael-s-molina commented Apr 3, 2023

kasiazjc commented Apr 4, 2023

michael-s-molina commented Apr 4, 2023

kgabryje Apr 4, 2023

michael-s-molina Apr 4, 2023

justinpark Apr 4, 2023

michael-s-molina Apr 4, 2023

justinpark Apr 5, 2023

michael-s-molina commented Apr 5, 2023

kasiazjc commented Apr 6, 2023 •

edited

Loading

michael-s-molina commented Apr 6, 2023

michael-s-molina commented Apr 6, 2023

geido left a comment

github-actions bot commented Apr 6, 2023

qlands commented Apr 27, 2023

michael-s-molina commented Apr 28, 2023

qlands commented Apr 28, 2023

eschutho commented May 12, 2023

feat: Shows user charts by default when editing a dashboard #23547

feat: Shows user charts by default when editing a dashboard #23547

Conversation

michael-s-molina commented Mar 31, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

michael-s-molina commented Mar 31, 2023

villebro left a comment

Choose a reason for hiding this comment

kasiazjc commented Apr 3, 2023 • edited Loading

michael-s-molina commented Apr 3, 2023 • edited Loading

kasiazjc commented Apr 3, 2023

codecov bot commented Apr 3, 2023 • edited Loading

Codecov Report

michael-s-molina commented Apr 3, 2023

github-actions bot commented Apr 3, 2023

michael-s-molina commented Apr 3, 2023

kasiazjc commented Apr 4, 2023

michael-s-molina commented Apr 4, 2023

kgabryje Apr 4, 2023

Choose a reason for hiding this comment

michael-s-molina Apr 4, 2023

Choose a reason for hiding this comment

justinpark Apr 4, 2023

Choose a reason for hiding this comment

michael-s-molina Apr 4, 2023

Choose a reason for hiding this comment

justinpark Apr 5, 2023

Choose a reason for hiding this comment

michael-s-molina commented Apr 5, 2023

kasiazjc commented Apr 6, 2023 • edited Loading

michael-s-molina commented Apr 6, 2023

michael-s-molina commented Apr 6, 2023

geido left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 6, 2023

qlands commented Apr 27, 2023

michael-s-molina commented Apr 28, 2023

qlands commented Apr 28, 2023

eschutho commented May 12, 2023

kasiazjc commented Apr 3, 2023 •

edited

Loading

michael-s-molina commented Apr 3, 2023 •

edited

Loading

codecov bot commented Apr 3, 2023 •

edited

Loading

kasiazjc commented Apr 6, 2023 •

edited

Loading