Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix(SavedQueries): allow other admin users see "saved queries" (#20604) #21769

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pengwk
Copy link

@pengwk pengwk commented Oct 11, 2022

SUMMARY

Show the list of all saved queries despite of creators. If the fix is acceptable, then add the test.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Follow #20604 issue's description.

ADDITIONAL INFORMATION

@pengwk
Copy link
Author

pengwk commented Oct 12, 2022

@dpgaspar Any time to look at this fix?

@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

Merging #21769 (242893a) into master (406e44b) will decrease coverage by 11.38%.
The diff coverage is 25.00%.

❗ Current head 242893a differs from pull request most recent head a7bdb78. Consider uploading reports for the commit a7bdb78 to get more accurate results

@@             Coverage Diff             @@
##           master   #21769       +/-   ##
===========================================
- Coverage   66.88%   55.50%   -11.39%     
===========================================
  Files        1800     1800               
  Lines       68967    68970        +3     
  Branches     7339     7339               
===========================================
- Hits        46128    38281     -7847     
- Misses      20943    28793     +7850     
  Partials     1896     1896               
Flag Coverage Δ
hive 52.92% <25.00%> (-0.01%) ⬇️
mysql ?
postgres ?
presto 52.82% <25.00%> (-0.01%) ⬇️
python 57.93% <25.00%> (-23.53%) ⬇️
sqlite ?
unit 51.05% <25.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
superset/queries/saved_queries/filters.py 74.07% <25.00%> (-21.76%) ⬇️
superset/utils/dashboard_import_export.py 0.00% <0.00%> (-100.00%) ⬇️
superset/tags/core.py 4.54% <0.00%> (-95.46%) ⬇️
superset/key_value/commands/update.py 0.00% <0.00%> (-90.91%) ⬇️
superset/key_value/commands/delete.py 0.00% <0.00%> (-87.88%) ⬇️
superset/key_value/commands/delete_expired.py 0.00% <0.00%> (-84.00%) ⬇️
superset/dashboards/commands/importers/v0.py 15.62% <0.00%> (-76.25%) ⬇️
superset/datasets/commands/update.py 25.00% <0.00%> (-69.05%) ⬇️
superset/datasets/commands/importers/v0.py 24.03% <0.00%> (-69.00%) ⬇️
superset/datasets/commands/create.py 31.25% <0.00%> (-68.75%) ⬇️
... and 284 more

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

@yousoph
Copy link
Member

yousoph commented Oct 13, 2022

Hi @pengwk ! Thanks for opening this PR

Would it be possible to make the following additions to help make sure the Saved Queries page is still usable for admins?

  1. Add a "Created by" column to the list view, between "Modified" and "Actions"
  2. Add a "Created by" filter to the top of the page, to the left of "Search"
  3. If possible, default the "Created by" filter to the current user so that admin users can easily find their own Saved queries and the initial state of the page is similar to how it is currently.

Thanks!

@pengwk
Copy link
Author

pengwk commented Oct 13, 2022

Hi @yousoph ! Thank you very much for your guidance and I am happy to be able to contribute to the project. Need to add a filter to the homepage?

image

@yousoph
Copy link
Member

yousoph commented Oct 13, 2022

Hi @yousoph ! Thank you very much for your guidance and I am happy to be able to contribute to the project. Need to add a filter to the homepage?

image

Good question! I think having the current "Mine" filter on the home page is enough. The other sections have "Favorite" and "Examples" filters which I don't think we need here

… "Actions"

2. Add a "Created by" filter to the top of the page, to the left of "Search"
@pengwk
Copy link
Author

pengwk commented Oct 17, 2022

Hi @pengwk ! Thanks for opening this PR

Would it be possible to make the following additions to help make sure the Saved Queries page is still usable for admins?

  1. Add a "Created by" column to the list view, between "Modified" and "Actions"
  2. Add a "Created by" filter to the top of the page, to the left of "Search"
  3. If possible, default the "Created by" filter to the current user so that admin users can easily find their own Saved queries and the initial state of the page is similar to how it is currently.

Thanks!

Hi, @yousoph! Both tasks 1 and 2 have been achieved, but I don't know how to do task 3. Can you find someone familiar with you for some guidance?

@mayurnewase
Copy link
Contributor

can you add tests?

@pengwk
Copy link
Author

pengwk commented Dec 21, 2022

Yes, I can add tests, wait for me. Thanks for your reply.

@matt-lessig
Copy link

Will this fix also apply to the REST API? We are trying to use an administrative service account to pull all saved queries for all users via the REST API.

@Bill0412
Copy link

Bill0412 commented Apr 20, 2023

Yes, I can add tests, wait for me. Thanks for your reply.

It would be great if you could write tests.

@pengwk
Copy link
Author

pengwk commented May 27, 2023

@Bill0412 I'm working on it, I was too busy with work, sorry.

@Bill0412
Copy link

I've already started using it without test. Don't worry about that too much.

@rusackas
Copy link
Member

rusackas commented Aug 1, 2024

Looks like this is in need of a rebase. Personally, I think we might be able to merge it without added filters and tests. While they'd be ideal, it seems like this is still an improvement and closes a long-outstanding Issue. Curious if you agree @yousoph

@pengwk
Copy link
Author

pengwk commented Sep 9, 2024

It looks like this PR is still useful, I will complete the rest @rusackas @yousoph

@Bill0412
Copy link

Bill0412 commented Oct 29, 2024

It looks like this PR is still useful, I will complete the rest @rusackas @yousoph

Yes, it will be really helpful if it's merged.

@zachoooo
Copy link

Would love to have this PR merged in

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.

Other admin users can't see "saved queries" created by another admin users
7 participants