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: Edit physical dataset from the Edit Dataset modal #15770

Merged
merged 5 commits into from Jul 19, 2021
Merged

fix: Edit physical dataset from the Edit Dataset modal #15770

merged 5 commits into from Jul 19, 2021

Conversation

geido
Copy link
Member

@geido geido commented Jul 18, 2021

SUMMARY

It renames the onChange event that was conflicting with the Fieldset onChange event, blocking the Dataset editor from saving the changes to the data source successfully.

Fixes #14592

BEFORE

BEFORE_DATASET_FIX.mp4

AFTER

AFTER_DATASET_FIX.mp4

TESTING INSTRUCTIONS

  1. Go to the Datasets page
  2. Click the edit icon on an existing Physical dataset
  3. In the Edit Dataset modal, click the lock icon to make changes to the dataset
  4. Select a different database from the current database. Then select a schema/table
  5. Save changes
  6. Open the Edit Dataset modal again and make sure the changes have been saved successfully

ADDITIONAL INFORMATION

  • Has associated issue: Unable to change database from Edit Dataset modal #14592
  • 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

@geido geido changed the title fix: Edit Physical dataset from the Datasets page fix: Edit physical dataset from the Edit Dataset modal Jul 18, 2021
@codecov
Copy link

codecov bot commented Jul 18, 2021

Codecov Report

Merging #15770 (f04155c) into master (7d0f6ab) will decrease coverage by 0.00%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15770      +/-   ##
==========================================
- Coverage   76.99%   76.99%   -0.01%     
==========================================
  Files         983      983              
  Lines       51621    51621              
  Branches     6991     6991              
==========================================
- Hits        39746    39745       -1     
- Misses      11650    11651       +1     
  Partials      225      225              
Flag Coverage Δ
javascript 71.68% <75.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...nd/src/views/CRUD/data/dataset/AddDatasetModal.tsx 56.25% <ø> (ø)
...et-frontend/src/components/TableSelector/index.tsx 84.25% <50.00%> (-0.79%) ⬇️
...frontend/src/components/DatabaseSelector/index.tsx 92.70% <100.00%> (ø)

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 7d0f6ab...f04155c. Read the comment docs.

@rusackas
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

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

Copy link
Member

@rusackas rusackas 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 works on the ephemeral env. However, I noticed that on virtual datasets, I can't change the name or the sql statement... are virtual datasets suffering a similar fate somehow? (cc @junlincc)

@geido
Copy link
Member Author

geido commented Jul 19, 2021

Hey @rusackas. Thanks for spotting that issue. It appears that the conflict was much deeper in the component, causing the onChange event to affect the other fields. I have removed the conflicting event and tested all the possible combinations of changes manually. This should be good for another round of testing.

@junlincc @adam-stasiak @rosemarie-chiu @jinghua-qa

@junlincc
Copy link
Member

/testenv up

@junlincc
Copy link
Member

junlincc commented Jul 19, 2021

Awesome thank you for the quick fix! Testing now @geido

@apache apache deleted a comment from rosemarie-chiu Jul 19, 2021
@apache apache deleted a comment from github-actions bot Jul 19, 2021
@apache apache deleted a comment from github-actions bot Jul 19, 2021
@geido
Copy link
Member Author

geido commented Jul 19, 2021

/testenv up

@github-actions
Copy link
Contributor

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

@rosemarie-chiu
Copy link
Contributor

Verified changing database on virtual dataset works. Launching in explore, in sql lab all works as expected.

CleanShot.2021-07-19.at.11.10.22.mp4

verified changing physical dataset's database works too

CleanShot.2021-07-19.at.11.13.38.mp4

Copy link
Contributor

@rosemarie-chiu rosemarie-chiu left a comment

Choose a reason for hiding this comment

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

QA visual test approved

@srinify srinify merged commit a7cbcde into apache:master Jul 19, 2021
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@rosemarie-chiu
Copy link
Contributor

🏷 2021.27

@junlincc junlincc added rush! Requires immediate attention #bug:blocking! Blocking issues with high priority labels Jul 19, 2021
henryyeh pushed a commit to preset-io/superset that referenced this pull request Jul 19, 2021
* Remove unnecessary onChange

* Remove confliciting onChange

* Revert unnecessary change

* Enhance and fix tests

(cherry picked from commit a7cbcde)
@geido geido moved this from In Review to Done in Community Usability Bash! 🐛🐞🕷 Jul 21, 2021
lyndsiWilliams pushed a commit to preset-io/superset that referenced this pull request Jul 26, 2021
* Remove unnecessary onChange

* Remove confliciting onChange

* Revert unnecessary change

* Enhance and fix tests
eschutho pushed a commit that referenced this pull request Jul 28, 2021
#15856)

* style: Rough draft of email report modal (#15666)

* clears errors when closing out of modal (#15623)

* add test + fix types

* fix lint errors

* Building ReportModal component

* Continued ReportModal creation

* Visual details updated

* CronError style

* Very basic testing added

Co-authored-by: AAfghahi <48933336+AAfghahi@users.noreply.github.com>

* feat: toggle the ReportModal Icon based on user permissions (#15780)

* style: Rough draft of email report modal (#15666)

* clears errors when closing out of modal (#15623)

* fix: avoid fetching favorite status for anonymous user (#15590)

* avoid fetching favorite status for anonymous user

* add test + fix types

* fix lint errors

* Building ReportModal component

* Continued ReportModal creation

* Visual details updated

* CronError style

* Very basic testing added

Co-authored-by: AAfghahi <48933336+AAfghahi@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* first draft

* added testing props

* cleaned up rebase

* changed name and type

Co-authored-by: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* feat: Begin incorporation of email report modal to Charts (#15793)

* Add email report modal to Charts

* Fix px themes

* feat: fetch UI specific reports  (#15785)

* style: Rough draft of email report modal (#15666)

* clears errors when closing out of modal (#15623)

* fix: avoid fetching favorite status for anonymous user (#15590)

* avoid fetching favorite status for anonymous user

* add test + fix types

* fix lint errors

* Building ReportModal component

* Continued ReportModal creation

* Visual details updated

* CronError style

* Very basic testing added

Co-authored-by: AAfghahi <48933336+AAfghahi@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* first draft

* created Api call

* added click logic

* made the fetch report into a action/reducer

* abstracted report action

* revision

* added reportState to reducer

* reports conditions

* revisions

* revisions

Co-authored-by: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* fix: Edit physical dataset from the Edit Dataset modal (#15770)

* Remove unnecessary onChange

* Remove confliciting onChange

* Revert unnecessary change

* Enhance and fix tests

* feat: add show columns to Reports model (#15712)

* added logic for creation_method

* revisions

* added index

* Update superset/migrations/versions/3317e9248280_add_creation_method_to_reports_model.py

* filters

* search columns updated

* fix: margin right on warning icon to 8px (#15715)

* changed margin right on warning icon to 8px

* fixed to grid units from pixels

* feat: adding Progress Bar to Benchmark script (#15719)

* rough draft of benchmark script

* revisions

* revisions

* rough draft of benchmark script

* revisions

* Update requirements/development.in

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

* Update superset/utils/mock_data.py

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

* more revisions

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

* fix: migration downgrade references wrong column (#15791)

* fix(dashboard): Add resize handles to right and bottom of component (#15778)

* fix(dashboard): Add resize handles to right and bottom of component

* Fix test

* chore: Add tags to the new viz gallery (#15734)

* chore: add tags to gallery

* fix UT

* fix lint

* redesign tags filter

* chore: change to Highly-used and fix some css

* fix UT

Co-authored-by: stephenLYZ <750188453@qq.com>

* chore: bump 0.17.70 (#15795)

* docs: Adding Sunbird to users list (#15794)

* chore: Improves the Select component UI/UX - iteration 4 (#15480)

* Add z-index only on maximize (#15800)

* chore: remove `retry` dependency in favor of `backoff` (#15788)

* chore: remove retry dep in favor of backoff

* Fix lint

* fix: create fk model in benchmark script (#15804)

* style: Rough draft of email report modal (#15666)

* clears errors when closing out of modal (#15623)

* fix: avoid fetching favorite status for anonymous user (#15590)

* avoid fetching favorite status for anonymous user

* add test + fix types

* fix lint errors

* Building ReportModal component

* Continued ReportModal creation

* Visual details updated

* CronError style

* Very basic testing added

Co-authored-by: AAfghahi <48933336+AAfghahi@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* Working on onSave functionality

* style: Rough draft of email report modal (#15666)

* clears errors when closing out of modal (#15623)

* add test + fix types

* fix lint errors

* Building ReportModal component

* Continued ReportModal creation

* Visual details updated

* CronError style

* Very basic testing added

Co-authored-by: AAfghahi <48933336+AAfghahi@users.noreply.github.com>

* feat: toggle the ReportModal Icon based on user permissions (#15780)

* style: Rough draft of email report modal (#15666)

* clears errors when closing out of modal (#15623)

* fix: avoid fetching favorite status for anonymous user (#15590)

* avoid fetching favorite status for anonymous user

* add test + fix types

* fix lint errors

* Building ReportModal component

* Continued ReportModal creation

* Visual details updated

* CronError style

* Very basic testing added

Co-authored-by: AAfghahi <48933336+AAfghahi@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* first draft

* added testing props

* cleaned up rebase

* changed name and type

Co-authored-by: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* feat: fetch UI specific reports  (#15785)

* style: Rough draft of email report modal (#15666)

* clears errors when closing out of modal (#15623)

* fix: avoid fetching favorite status for anonymous user (#15590)

* avoid fetching favorite status for anonymous user

* add test + fix types

* fix lint errors

* Building ReportModal component

* Continued ReportModal creation

* Visual details updated

* CronError style

* Very basic testing added

Co-authored-by: AAfghahi <48933336+AAfghahi@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* first draft

* created Api call

* added click logic

* made the fetch report into a action/reducer

* abstracted report action

* revision

* added reportState to reducer

* reports conditions

* revisions

* revisions

Co-authored-by: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* Dashboard onSave progress

* More onSave/create progress

* Dashboard POST working!

* Dashboard POST relocated to redux

* POST now also working in Charts

* making linter happy

* linter again

Co-authored-by: AAfghahi <48933336+AAfghahi@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>
Co-authored-by: Geido <60598000+geido@users.noreply.github.com>
Co-authored-by: stellalc7 <stellalc7@gmail.com>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>
Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>
Co-authored-by: stephenLYZ <750188453@qq.com>
Co-authored-by: Kumar <kumarks1122@gmail.com>
Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
eschutho pushed a commit that referenced this pull request Jul 30, 2021
#15856)

* style: Rough draft of email report modal (#15666)

* clears errors when closing out of modal (#15623)

* add test + fix types

* fix lint errors

* Building ReportModal component

* Continued ReportModal creation

* Visual details updated

* CronError style

* Very basic testing added

Co-authored-by: AAfghahi <48933336+AAfghahi@users.noreply.github.com>

* feat: toggle the ReportModal Icon based on user permissions (#15780)

* style: Rough draft of email report modal (#15666)

* clears errors when closing out of modal (#15623)

* fix: avoid fetching favorite status for anonymous user (#15590)

* avoid fetching favorite status for anonymous user

* add test + fix types

* fix lint errors

* Building ReportModal component

* Continued ReportModal creation

* Visual details updated

* CronError style

* Very basic testing added

Co-authored-by: AAfghahi <48933336+AAfghahi@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* first draft

* added testing props

* cleaned up rebase

* changed name and type

Co-authored-by: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* feat: Begin incorporation of email report modal to Charts (#15793)

* Add email report modal to Charts

* Fix px themes

* feat: fetch UI specific reports  (#15785)

* style: Rough draft of email report modal (#15666)

* clears errors when closing out of modal (#15623)

* fix: avoid fetching favorite status for anonymous user (#15590)

* avoid fetching favorite status for anonymous user

* add test + fix types

* fix lint errors

* Building ReportModal component

* Continued ReportModal creation

* Visual details updated

* CronError style

* Very basic testing added

Co-authored-by: AAfghahi <48933336+AAfghahi@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* first draft

* created Api call

* added click logic

* made the fetch report into a action/reducer

* abstracted report action

* revision

* added reportState to reducer

* reports conditions

* revisions

* revisions

Co-authored-by: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* fix: Edit physical dataset from the Edit Dataset modal (#15770)

* Remove unnecessary onChange

* Remove confliciting onChange

* Revert unnecessary change

* Enhance and fix tests

* feat: add show columns to Reports model (#15712)

* added logic for creation_method

* revisions

* added index

* Update superset/migrations/versions/3317e9248280_add_creation_method_to_reports_model.py

* filters

* search columns updated

* fix: margin right on warning icon to 8px (#15715)

* changed margin right on warning icon to 8px

* fixed to grid units from pixels

* feat: adding Progress Bar to Benchmark script (#15719)

* rough draft of benchmark script

* revisions

* revisions

* rough draft of benchmark script

* revisions

* Update requirements/development.in

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

* Update superset/utils/mock_data.py

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

* more revisions

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

* fix: migration downgrade references wrong column (#15791)

* fix(dashboard): Add resize handles to right and bottom of component (#15778)

* fix(dashboard): Add resize handles to right and bottom of component

* Fix test

* chore: Add tags to the new viz gallery (#15734)

* chore: add tags to gallery

* fix UT

* fix lint

* redesign tags filter

* chore: change to Highly-used and fix some css

* fix UT

Co-authored-by: stephenLYZ <750188453@qq.com>

* chore: bump 0.17.70 (#15795)

* docs: Adding Sunbird to users list (#15794)

* chore: Improves the Select component UI/UX - iteration 4 (#15480)

* Add z-index only on maximize (#15800)

* chore: remove `retry` dependency in favor of `backoff` (#15788)

* chore: remove retry dep in favor of backoff

* Fix lint

* fix: create fk model in benchmark script (#15804)

* style: Rough draft of email report modal (#15666)

* clears errors when closing out of modal (#15623)

* fix: avoid fetching favorite status for anonymous user (#15590)

* avoid fetching favorite status for anonymous user

* add test + fix types

* fix lint errors

* Building ReportModal component

* Continued ReportModal creation

* Visual details updated

* CronError style

* Very basic testing added

Co-authored-by: AAfghahi <48933336+AAfghahi@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* Working on onSave functionality

* style: Rough draft of email report modal (#15666)

* clears errors when closing out of modal (#15623)

* add test + fix types

* fix lint errors

* Building ReportModal component

* Continued ReportModal creation

* Visual details updated

* CronError style

* Very basic testing added

Co-authored-by: AAfghahi <48933336+AAfghahi@users.noreply.github.com>

* feat: toggle the ReportModal Icon based on user permissions (#15780)

* style: Rough draft of email report modal (#15666)

* clears errors when closing out of modal (#15623)

* fix: avoid fetching favorite status for anonymous user (#15590)

* avoid fetching favorite status for anonymous user

* add test + fix types

* fix lint errors

* Building ReportModal component

* Continued ReportModal creation

* Visual details updated

* CronError style

* Very basic testing added

Co-authored-by: AAfghahi <48933336+AAfghahi@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* first draft

* added testing props

* cleaned up rebase

* changed name and type

Co-authored-by: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* feat: fetch UI specific reports  (#15785)

* style: Rough draft of email report modal (#15666)

* clears errors when closing out of modal (#15623)

* fix: avoid fetching favorite status for anonymous user (#15590)

* avoid fetching favorite status for anonymous user

* add test + fix types

* fix lint errors

* Building ReportModal component

* Continued ReportModal creation

* Visual details updated

* CronError style

* Very basic testing added

Co-authored-by: AAfghahi <48933336+AAfghahi@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* first draft

* created Api call

* added click logic

* made the fetch report into a action/reducer

* abstracted report action

* revision

* added reportState to reducer

* reports conditions

* revisions

* revisions

Co-authored-by: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>

* Dashboard onSave progress

* More onSave/create progress

* Dashboard POST working!

* Dashboard POST relocated to redux

* POST now also working in Charts

* making linter happy

* linter again

Co-authored-by: AAfghahi <48933336+AAfghahi@users.noreply.github.com>
Co-authored-by: aspedrosa <aspedrosa@ua.pt>
Co-authored-by: Geido <60598000+geido@users.noreply.github.com>
Co-authored-by: stellalc7 <stellalc7@gmail.com>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>
Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>
Co-authored-by: stephenLYZ <750188453@qq.com>
Co-authored-by: Kumar <kumarks1122@gmail.com>
Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
* Remove unnecessary onChange

* Remove confliciting onChange

* Revert unnecessary change

* Enhance and fix tests
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* Remove unnecessary onChange

* Remove confliciting onChange

* Revert unnecessary change

* Enhance and fix tests
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* Remove unnecessary onChange

* Remove confliciting onChange

* Revert unnecessary change

* Enhance and fix tests
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 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 #bug:blocking! Blocking issues with high priority preset:2021.27 rush! Requires immediate attention 🚢 1.3.0
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Unable to change database from Edit Dataset modal
6 participants