Skip to content

fix(explore): update overwrite button on perm change#16437

Merged
villebro merged 6 commits into
apache:masterfrom
preset-io:villebro/fix-explore-overwrite
Aug 26, 2021
Merged

fix(explore): update overwrite button on perm change#16437
villebro merged 6 commits into
apache:masterfrom
preset-io:villebro/fix-explore-overwrite

Conversation

@villebro
Copy link
Copy Markdown
Member

@villebro villebro commented Aug 25, 2021

SUMMARY

Currently the Explore view does not update the "can_overwrite" permission when the user updates the chart ownership, as it's populated from bootstrap data (rendered server-side when page is loaded). In addition, the "Save" button in the chart metadata modal is enabled when the owners array is initially empty, but after adding and removing a user the save button gets disabled.

This removes the can_overwrite property from the Explore bootstrap data and replaces it with state from Redux, making it possible to change the disabled state of the "Save (Overwrite)" button in the Save modal. In addition, the slice.owners property in bootstrap data is changed to provide numeric ids instead of "firstname lastname" (most other models were already returning owners in this format; it appears the "firstname lastname" results weren't being used anywhere in Explore). Also update some types and remove redundant user_id property from bootstrap data.

AFTER

explore-after.mp4

BEFORE

explore-before.mp4

TESTING INSTRUCTIONS

  1. Login as Admin
  2. Open the Charts page and edit the metadata of an example dataset that doesn't have any owners
  3. Add and remove yourself as owner and notice how the Save button goes from being enabled to disabled without the owners changing. Click cancel.
  4. Open an example chart in Explore that doesn't have any owners
  5. Add yourself as an owner
  6. Try to overwrite the chart - notice that the "Save (Overwrite)" radio button is disabled

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This had to be explicitly added, as the action is missing the owners prop when all owners are missing

@pull-request-size pull-request-size Bot added size/M and removed size/S labels Aug 25, 2021
@villebro villebro changed the title fix(explore): update overwrite on perm change fix(explore): update overwrite button on perm change Aug 25, 2021
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 25, 2021

Codecov Report

Merging #16437 (529079c) into master (6a2cec5) will increase coverage by 0.10%.
The diff coverage is 84.61%.

❗ Current head 529079c differs from pull request most recent head 39a9f2c. Consider uploading reports for the commit 39a9f2c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16437      +/-   ##
==========================================
+ Coverage   76.39%   76.50%   +0.10%     
==========================================
  Files        1001     1002       +1     
  Lines       53535    53637     +102     
  Branches     6822     6853      +31     
==========================================
+ Hits        40900    41033     +133     
+ Misses      12396    12365      -31     
  Partials      239      239              
Flag Coverage Δ
javascript 70.81% <74.57%> (+<0.01%) ⬆️
mysql 81.56% <91.35%> (+0.02%) ⬆️
postgres 81.59% <91.66%> (+0.06%) ⬆️
presto 81.40% <91.66%> (?)
python 81.84% <91.66%> (+0.19%) ⬆️
sqlite 81.19% <91.35%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
...set-frontend/src/components/ModalTrigger/index.jsx 95.65% <ø> (ø)
.../components/ExploreAdditionalActionsMenu/index.jsx 100.00% <ø> (ø)
...nd/src/explore/components/ExploreViewContainer.jsx 2.11% <ø> (ø)
...et-frontend/src/explore/reducers/exploreReducer.js 32.00% <0.00%> (-0.44%) ⬇️
...t-frontend/src/explore/reducers/getInitialState.ts 30.00% <ø> (ø)
superset/models/slice.py 86.41% <ø> (ø)
...src/explore/components/controls/ViewQueryModal.tsx 76.92% <66.66%> (+0.92%) ⬆️
superset-frontend/src/components/Modal/Modal.tsx 85.22% <73.46%> (-14.78%) ⬇️
superset/utils/sqllab_execution_context.py 90.14% <90.14%> (ø)
...d/src/explore/components/PropertiesModal/index.tsx 81.94% <100.00%> (ø)
... and 9 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 6a2cec5...39a9f2c. Read the comment docs.

@stephenLYZ
Copy link
Copy Markdown
Member

Frontend looks good to me! Thanks for the fixing. Is there any other state that can be maintained on the frontend only?

@villebro villebro force-pushed the villebro/fix-explore-overwrite branch from 4908a47 to 9ad9827 Compare August 25, 2021 11:30
@villebro villebro force-pushed the villebro/fix-explore-overwrite branch from 9ad9827 to d230f82 Compare August 25, 2021 12:42
@zhaoyongjie zhaoyongjie self-requested a review August 25, 2021 14:06
Copy link
Copy Markdown
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

LGTM

@villebro
Copy link
Copy Markdown
Member Author

/testenv up

@github-actions
Copy link
Copy Markdown
Contributor

@villebro Ephemeral environment spinning up at http://54.201.70.223:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@villebro
Copy link
Copy Markdown
Member Author

villebro commented Aug 25, 2021

@stephenLYZ :

Frontend looks good to me! Thanks for the fixing. Is there any other state that can be maintained on the frontend only?

Good question; I did briefly look into moving a few other properties to the frontend, but the other obvious candidates in this template (can_add and can_download) seemed like they were actually better handled in the backend. But I will definitely be on the lookout for similar simplifications in subsequent PRs.

@villebro villebro merged commit 18be181 into apache:master Aug 26, 2021
@villebro villebro deleted the villebro/fix-explore-overwrite branch August 26, 2021 03:24
@github-actions
Copy link
Copy Markdown
Contributor

Ephemeral environment shutdown and build artifacts deleted.

amitmiran137 pushed a commit to nielsen-oss/superset that referenced this pull request Aug 26, 2021
* upstream/master: (25 commits)
  chore(ci): bump pylint to 2.10.2 (apache#16463)
  fix: prevent page crash when chart can't render (apache#16464)
  chore: fixed slack invite link (apache#16466)
  fix(native-filters): handle null values in value filter (apache#16460)
  feat: add function list to auto-complete to Clickhouse datasource (apache#16234)
  refactor(explore): improve typing for Dnd controls (apache#16362)
  fix(explore): update overwrite button on perm change (apache#16437)
  feat: Draggable and Resizable Modal (apache#16394)
  refactor: sql_json view endpoint (apache#16441)
  fix(dashboard): undo and redo buttons weird alignment  (apache#16417)
  fix: setupPlugin in chart list page (apache#16413)
  fix: Disable Slack notification method if no api token (apache#16367)
  feat: add Shillelagh DB engine spec (apache#16416)
  fix: copy to Clipboard order (apache#16299)
  docs: make FEATURE_FLAGS.md reference a link (apache#16415)
  chore(viz): bump superset-ui to 0.17.87 (apache#16420)
  feat: add activate command (apache#16404)
  Revert "fix(explore): let admin overwrite slice (apache#16290)" (apache#16408)
  fix(explore): retain chart ownership on query context update (apache#16419)
  chore: Removes the TODOs and uses the default page size (apache#16422)
  ...
@villebro villebro added v1.3 and removed v1.3 labels Aug 30, 2021
opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
* fix(explore): update overwrite on perm change

* remove redundant user_id prop

* fix types

* fix user type

* fix tests

* fix lint
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
* fix(explore): update overwrite on perm change

* remove redundant user_id prop

* fix types

* fix user type

* fix tests

* fix lint
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 First shipped in 1.4.0 labels Mar 13, 2024
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
* fix(explore): update overwrite on perm change

* remove redundant user_id prop

* fix types

* fix user type

* fix tests

* fix lint
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 preset-io size/M 🚢 1.4.0 First shipped in 1.4.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants