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: show Import button only if has perms #16763

Merged

Conversation

betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Sep 21, 2021

SUMMARY

Only show the "Import" button if the user has the permission to create the object.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

I created a gamma user and removed the Can write on dashboard permission. Before this PR they see the import button:

Screenshot 2021-09-21 at 11-12-54 Superset

After:

Screenshot 2021-09-21 at 11-12-45  DEV  Superset

TESTING INSTRUCTIONS

Create a gamma user and remove the Can write dashboard permission. Import button should not show up.

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

@betodealmeida betodealmeida marked this pull request as ready for review September 21, 2021 18:15
buttonStyle: 'link',
onClick: openChartImportModal,
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Note line 580 above. I moved this to inside the if (canCreate) block. Similar for other changes.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 22, 2021
@@ -190,7 +190,7 @@ describe('RTL', () => {
});

it('renders an "Import Chart" tooltip under import button', async () => {
const importButton = screen.getByTestId('import-button');
const importButton = await screen.findByTestId('import-button');
Copy link
Member Author

Choose a reason for hiding this comment

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

We now need to wait til the permissions are fetched in useEffect().

@codecov
Copy link

codecov bot commented Sep 22, 2021

Codecov Report

Merging #16763 (9fcf3db) into master (48a61ba) will decrease coverage by 0.08%.
The diff coverage is 70.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16763      +/-   ##
==========================================
- Coverage   76.76%   76.67%   -0.09%     
==========================================
  Files        1007     1005       -2     
  Lines       54186    54138      -48     
  Branches     7464     7446      -18     
==========================================
- Hits        41595    41511      -84     
- Misses      12351    12383      +32     
- Partials      240      244       +4     
Flag Coverage Δ
javascript 71.09% <70.00%> (-0.20%) ⬇️

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

Impacted Files Coverage Δ
...rontend/src/views/CRUD/dashboard/DashboardList.tsx 72.22% <50.00%> (+0.39%) ⬆️
...ontend/src/views/CRUD/data/dataset/DatasetList.tsx 69.41% <66.66%> (ø)
...perset-frontend/src/views/CRUD/chart/ChartList.tsx 74.35% <100.00%> (-3.63%) ⬇️
.../src/views/CRUD/data/savedquery/SavedQueryList.tsx 73.10% <100.00%> (+0.18%) ⬆️
...rontend/src/components/ListView/Filters/Select.tsx 76.92% <0.00%> (-20.64%) ⬇️
superset-frontend/src/views/CRUD/utils.tsx 55.81% <0.00%> (-13.04%) ⬇️
...et-frontend/src/components/TableSelector/index.tsx 77.55% <0.00%> (-6.71%) ⬇️
superset-frontend/src/components/Select/utils.ts 92.30% <0.00%> (-3.85%) ⬇️
...tend/src/components/ListView/Filters/DateRange.tsx 50.00% <0.00%> (-2.18%) ⬇️
... and 57 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 48a61ba...9fcf3db. Read the comment docs.

@betodealmeida betodealmeida added the need:merge The PR is ready to be merged label Sep 22, 2021
@betodealmeida betodealmeida merged commit 6921d94 into apache:master Sep 23, 2021
opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
* fix: show Import button only if has perms

* Fix tests
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
* fix: show Import button only if has perms

* Fix tests
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels need:merge The PR is ready to be merged size/L 🚢 1.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants