fix(dashboard list): do not show favorite star for anonymous users #18210 #19409

dudasaron · 2022-03-29T13:28:30Z

SUMMARY

A previous commit removed the FaveStar component and the id column with it from the dashboards list view for anonymous users. This broke the CERTIFIED filter because it also depends on the id column.
This PR re-adds the id column, but removes the favorite star and hides the column if the user is not logged in. Also removing the stars from the DashboardCard component if not applicable.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

Anonymous user, list/card view:

Logged in user, dashboard (for reference, not supposed to change):

Logged in user, list view (for reference, not supposed to change):

Logged in user, card view (for reference, not supposed to change):

After

Anonymous user, list view:

Anonymous user, card view:

Logged in user, dashboard (not supposed to change visually, but screenshot is included because some components are modified):

Logged in user, list view (not supposed to change visually, but screenshot is included because some components are modified):

Logged in user, card view (not supposed to change visually, but screenshot is included because some components are modified):

TESTING INSTRUCTIONS

Automated tests added for both logged in/anonymous cases for table and card view.

For manual testing add

PUBLIC_ROLE_LIKE = "Gamma"

to docker/pythonpath_dev/superset_config.py and run docker compose up.

Log in as admin and add the all datasource access on all_datasource_access and all database access on all_database_access permissions to the public role.

After logging out, navigate to http://localhost:3000/superset/dashboards/list and the dashboard list should open without an error and the CERTIFIED filter should work and the favorite star should not be present on the list view nor the table view. When logging in, the favorite star should be there.

ADDITIONAL INFORMATION

Has associated issue: Fixes Unexpected error: ListViewError: Invalid filter config, id is not present in columns #18210
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

…pache#18210

codecov · 2022-03-31T12:30:44Z

Codecov Report

Merging #19409 (305a262) into master (d304849) will increase coverage by 0.10%.
The diff coverage is 71.71%.

❗ Current head 305a262 differs from pull request most recent head ced4cea. Consider uploading reports for the commit ced4cea to get more accurate results

@@            Coverage Diff             @@
##           master   #19409      +/-   ##
==========================================
+ Coverage   66.48%   66.59%   +0.10%     
==========================================
  Files        1670     1677       +7     
  Lines       63965    64237     +272     
  Branches     6512     6538      +26     
==========================================
+ Hits        42528    42778     +250     
- Misses      19748    19764      +16     
- Partials     1689     1695       +6

Flag	Coverage Δ
javascript	`51.34% <53.13%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
...ntrols/src/components/CertifiedIconWithTooltip.tsx	`80.00% <ø> (ø)`
...d/packages/superset-ui-chart-controls/src/index.ts	`100.00% <ø> (ø)`
...omponents/ColumnConfigControl/ColumnConfigItem.tsx	`0.00% <ø> (ø)`
...ugins/legacy-plugin-chart-calendar/src/Calendar.js	`0.00% <ø> (ø)`
...legacy-plugin-chart-calendar/src/ReactCalendar.jsx	`0.00% <0.00%> (ø)`
...cy-plugin-chart-calendar/src/vendor/cal-heatmap.js	`0.00% <ø> (ø)`
...plugins/legacy-plugin-chart-heatmap/src/Heatmap.js	`0.00% <ø> (ø)`
...s/legacy-plugin-chart-heatmap/src/ReactHeatmap.jsx	`0.00% <0.00%> (ø)`
...plugins/legacy-plugin-chart-rose/src/ReactRose.jsx	`0.00% <0.00%> (ø)`
...ntend/plugins/legacy-plugin-chart-rose/src/Rose.js	`0.00% <0.00%> (ø)`
... and 184 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 d304849...ced4cea. Read the comment docs.

AAfghahi · 2022-03-31T14:24:43Z

superset-frontend/src/views/CRUD/dashboard/DashboardCard.tsx

@@ -44,7 +44,7 @@ interface DashboardCardProps {
  saveFavoriteStatus: (id: number, isStarred: boolean) => void;
  favoriteStatus: boolean;
  dashboardFilter?: string;
-  userId?: number;
+  userId?: string | number;


Hey! I don't think that this should ever be a string? Could you give me more context?

I found it strange too, but it comes from already existing code: DashboardList.tsx:73 and I didn't want to dig deep into this for this PR

I think this should be number. You can take note of it for a follow up.

AAfghahi · 2022-03-31T14:25:53Z

superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx

+            original: { id },
+          },
+        }: any) =>
+          userId ? (


Why this over userId &&

good point, I'll change it

pkdotson

LGTM!

pkdotson · 2022-04-01T23:54:24Z

superset-frontend/src/views/CRUD/dashboard/DashboardCard.tsx

@@ -44,7 +44,7 @@ interface DashboardCardProps {
  saveFavoriteStatus: (id: number, isStarred: boolean) => void;
  favoriteStatus: boolean;
  dashboardFilter?: string;
-  userId?: number;
+  userId?: string | number;


I think this should be number. You can take note of it for a follow up.

pkdotson · 2022-04-01T23:54:25Z

superset-frontend/src/views/CRUD/dashboard/DashboardCard.tsx

@@ -44,7 +44,7 @@ interface DashboardCardProps {
  saveFavoriteStatus: (id: number, isStarred: boolean) => void;
  favoriteStatus: boolean;
  dashboardFilter?: string;
-  userId?: number;
+  userId?: string | number;


I think this should be number. You can take note of it for a follow up.

villebro

This is great work @dudasaron ! Thanks not only for fixing this, but also cleaning up the code + adding tests ❤️

…8210 (#19409) * fix: Only show favorite star on dashboard list if user is logged in #18210 * Fix linter errors (cherry picked from commit b8891ac)

crazamahdi · 2022-04-06T19:34:45Z

Hey I am facing same error....please let me know how to fix that issue....i read the whole conversation but did not understand what changes to be done in config.py file.....I wrote PUBLIC_ROLE_LIKE = "Gamma" in config.py file then relaunched it through superset init but still i face the same unexpected error..ListViewError: Invalid filter config, id is not present in columns...pls Help me out @dudasaron or @villebro or anyone who knows how to fix this issue.

dudasaron · 2022-04-11T10:23:16Z

Hi @crazamahdi, I think you were referring to the reproduction steps of the bug. What version are you using? This fix is only merged into master, but it's not yet part of any released version. If you don't have the time to wait until it makes into the official release, you can create a fork of the latest release of superset (1.4.2 as of now) and cherry pick the merge commit on top of that.

crazamahdi · 2022-04-11T12:59:00Z

Hi @crazamahdi, I think you were referring to the reproduction steps of the bug. What version are you using? This fix is only merged into master, but it's not yet part of any released version. If you don't have the time to wait until it makes into the official release, you can create a fork of the latest release of superset (1.4.2 as of now) and cherry pick the merge commit on top of that.

Hey thanks for your reply …I am using version 1.4.1…but I have superset version 1.4.1 on my local machine which works fine (running on http)…now when I installed the same version on my work machine(Aws instance Ubuntu 20) it throws me this unexpected error ListViewError….(while running on HTTPS)…I don’t understand how same version of superset is working on my local machine and not on my work machine….is it something related to HTTP and HTTPS? If so what changes should I do in config file to run it on https. For me I am not even able to login through admin…after entering credentials this unexpected error screen shows up

I tried installing, deleting and reinstalling various versions of superset on my work machine but everytime it throws some unexpected error….please help me out with this issue I tried resolving it for more than 4 days but no success yet@dudasaron

chenshaojie · 2022-04-17T14:57:46Z

Hi @crazamahdi, I think you were referring to the reproduction steps of the bug. What version are you using? This fix is only merged into master, but it's not yet part of any released version. If you don't have the time to wait until it makes into the official release, you can create a fork of the latest release of superset (1.4.2 as of now) and cherry pick the merge commit on top of that.

Hey thanks for your reply …I am using version 1.4.1…but I have superset version 1.4.1 on my local machine which works fine (running on http)…now when I installed the same version on my work machine(Aws instance Ubuntu 20) it throws me this unexpected error ListViewError….(while running on HTTPS)…I don’t understand how same version of superset is working on my local machine and not on my work machine….is it something related to HTTP and HTTPS? If so what changes should I do in config file to run it on https. For me I am not even able to login through admin…after entering credentials this unexpected error screen shows up

I tried installing, deleting and reinstalling various versions of superset on my work machine but everytime it throws some unexpected error….please help me out with this issue I tried resolving it for more than 4 days but no success yet@dudasaron

Hello, I took the liberty of disturbing, I am using the same version of the program, but I use centos, and now I am encountering the same problem as you, do you have a solution now? I found myself that even if I logged in, I would always stay on the first page of the previous page described by the author, similar to the refresh operation, but I myself used wechat to browse the web page, although there would be an operation described by the author, I could still log in and show two or three pictures of the previous page. Thank you very much.
Sorry, my English is not good, so use the translator for conversations, thank you very much for answering my questions

crazamahdi · 2022-04-17T15:06:35Z

Hey @chenshaojie I am still stuck at the same issue…hoping they release the fixed version soon. Got to know that issue is from their end and the fix is not yet officially released but committed to master branch. U can wait for release or u can try from GitHub directly. (If GitHub one works pls let me know too)
Thanks

crazamahdi · 2022-04-24T03:04:57Z

Hey @chenshaojie I finally managed to solved that error. Solve the login error by commenting the following lines in config.py file

Comment settings as below:

#SESSION_COOKIE_SAMESITE = "None"
#SESSION_COOKIE_SECURE = True

someone helped me with the solution on stack overflow.
and it worked well for me.

chenshaojie · 2022-04-24T04:57:34Z

thank you very much !૧(●´৺`●)૭ I will try it 😘

…

---Original--- From: "Raza Mahdi ***@***.***> Date: Sun, Apr 24, 2022 11:05 AM To: ***@***.***>; Cc: ***@***.******@***.***>; Subject: Re: [apache/superset] fix(dashboard list): do not show favorite starfor anonymous users #18210 (PR #19409) Hey @chenshaojie I finally managed to solved that error. Solve the login error by commenting the following lines in config.py file Comment settings as below: #SESSION_COOKIE_SAMESITE = "None" #SESSION_COOKIE_SECURE = True someone helped me with the solution on stack overflow. and it worked well for me. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

…ache#18210 (apache#19409) * fix: Only show favorite star on dashboard list if user is logged in apache#18210 * Fix linter errors

pull-request-size bot added the size/L label Mar 29, 2022

dudasaron marked this pull request as ready for review March 29, 2022 14:00

fix: Only show favorite star on dashboard list if user is logged in a…

34820b4

…pache#18210

dudasaron force-pushed the fix/18210-list-view-invalid-filter-config-for-anonymous-user branch from eb597e6 to 34820b4 Compare March 30, 2022 09:28

AAfghahi requested changes Mar 31, 2022

View reviewed changes

dudasaron force-pushed the fix/18210-list-view-invalid-filter-config-for-anonymous-user branch from 5254a95 to 2fc450c Compare April 1, 2022 17:29

Fix linter errors

ced4cea

dudasaron force-pushed the fix/18210-list-view-invalid-filter-config-for-anonymous-user branch from 2fc450c to ced4cea Compare April 1, 2022 17:29

AAfghahi approved these changes Apr 1, 2022

View reviewed changes

pkdotson approved these changes Apr 1, 2022

View reviewed changes

villebro approved these changes Apr 4, 2022

View reviewed changes

villebro merged commit b8891ac into apache:master Apr 4, 2022

villebro added the lts-v1 label Apr 4, 2022

dudasaron deleted the fix/18210-list-view-invalid-filter-config-for-anonymous-user branch April 11, 2022 10:23

This was referenced May 23, 2022

Unexpected error: ListViewError: Invalid filter config, id is not present in columns #18210

Closed

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

Merged

mistercrunch added 🍒 1.5.0 🍒 1.5.1 labels Mar 13, 2024

mistercrunch added 🍒 1.5.2 🍒 1.5.3 🏷️ 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dashboard list): do not show favorite star for anonymous users #18210 #19409

fix(dashboard list): do not show favorite star for anonymous users #18210 #19409

dudasaron commented Mar 29, 2022 •

edited

Loading

codecov bot commented Mar 31, 2022 •

edited

Loading

AAfghahi Mar 31, 2022

dudasaron Apr 1, 2022

pkdotson Apr 1, 2022

pkdotson Apr 1, 2022

AAfghahi Mar 31, 2022

dudasaron Apr 1, 2022

pkdotson left a comment

pkdotson Apr 1, 2022

pkdotson Apr 1, 2022

villebro left a comment

crazamahdi commented Apr 6, 2022 •

edited

Loading

dudasaron commented Apr 11, 2022

crazamahdi commented Apr 11, 2022

chenshaojie commented Apr 17, 2022

crazamahdi commented Apr 17, 2022

crazamahdi commented Apr 24, 2022

chenshaojie commented Apr 24, 2022 via email

fix(dashboard list): do not show favorite star for anonymous users #18210 #19409

fix(dashboard list): do not show favorite star for anonymous users #18210 #19409

Conversation

dudasaron commented Mar 29, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Mar 31, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkdotson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

crazamahdi commented Apr 6, 2022 • edited Loading

dudasaron commented Apr 11, 2022

crazamahdi commented Apr 11, 2022

chenshaojie commented Apr 17, 2022

crazamahdi commented Apr 17, 2022

crazamahdi commented Apr 24, 2022

Comment settings as below:

chenshaojie commented Apr 24, 2022 via email

dudasaron commented Mar 29, 2022 •

edited

Loading

codecov bot commented Mar 31, 2022 •

edited

Loading

crazamahdi commented Apr 6, 2022 •

edited

Loading