Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(charts list): do not trigger ListViewError exception for anonymous user #20171

Merged

Conversation

trepmag
Copy link
Contributor

@trepmag trepmag commented May 24, 2022

SUMMARY

This PR is a follow up for issue #18210 which fixes charts list, while the first PR #19409 fixes the dashboards list.

Note well: this PR is a copy/past of @dudasaron PR #19409 for dashboards list, for which, I must admit, I don't master each changes...

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

Selection_429

After

Selection_428

TESTING INSTRUCTIONS

Was tested on current master (b746e6f): visit the charts list as anonymous user...

ADDITIONAL INFORMATION

@codecov
Copy link

codecov bot commented May 24, 2022

Codecov Report

Merging #20171 (0bb05c4) into master (b746e6f) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #20171      +/-   ##
==========================================
- Coverage   66.46%   66.43%   -0.03%     
==========================================
  Files        1721     1723       +2     
  Lines       64521    64627     +106     
  Branches     6811     6829      +18     
==========================================
+ Hits        42882    42935      +53     
- Misses      19906    19960      +54     
+ Partials     1733     1732       -1     
Flag Coverage Δ
javascript 51.34% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
superset-frontend/src/views/CRUD/utils.tsx 64.80% <ø> (ø)
...perset-frontend/src/views/CRUD/chart/ChartCard.tsx 50.00% <100.00%> (+2.38%) ⬆️
...perset-frontend/src/views/CRUD/chart/ChartList.tsx 58.77% <100.00%> (+2.73%) ⬆️
...s/legacy-plugin-chart-sunburst/src/controlPanel.ts 20.00% <0.00%> (-80.00%) ⬇️
...et-frontend/src/components/EditableTitle/index.tsx 64.28% <0.00%> (-5.72%) ⬇️
.../components/Header/HeaderActionsDropdown/index.jsx 68.33% <0.00%> (-3.34%) ⬇️
...ins/plugin-chart-table/src/DataTable/DataTable.tsx 35.39% <0.00%> (-3.15%) ⬇️
.../src/explore/components/DataTableControl/index.tsx 69.23% <0.00%> (-2.57%) ⬇️
...ponents/ReportModal/HeaderReportDropdown/index.tsx 68.00% <0.00%> (-1.34%) ⬇️
...uperset-frontend/src/explore/exploreUtils/index.js 79.71% <0.00%> (-1.32%) ⬇️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b746e6f...0bb05c4. Read the comment docs.

@yousoph yousoph requested a review from stephenLYZ June 1, 2022 22:25
Copy link
Member

@stephenLYZ stephenLYZ left a comment

Choose a reason for hiding this comment

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

LGTM and leave one nit.

userId,
refreshData,
saveFavoriteStatus,
// userKey,
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this unused code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tested the chart list successfully (connected and anonymously); initial commit was amended minus the above chunk: 0bb05c4

@trepmag trepmag force-pushed the fix/charts_list_for_anonymous_users branch from 8ffa4eb to 0bb05c4 Compare June 2, 2022 14:09
@stephenLYZ stephenLYZ merged commit a813528 into apache:master Jun 2, 2022
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected error: ListViewError: Invalid filter config, id is not present in columns
3 participants