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: Cancel alert is not appearing to all native filters modal fields #15925

Merged

Conversation

michael-s-molina
Copy link
Member

@michael-s-molina michael-s-molina commented Jul 28, 2021

SUMMARY

Monitors changes to all fields of the native filters modal. Previously, the cancel alert was not shown when some fields changed.

@junlincc @jinghua-qa @adam-stasiak

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

screen-recording-2021-07-28-at-112714-am_cHb2HCCC.mp4
screen-recording-2021-07-28-at-112305-am_FpbHduJJ.mp4

TESTING INSTRUCTIONS

  • Go to the native filters modal
  • Create, edit, delete filters
  • Check if the cancel alert is displayed consistently

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

@junlincc
Copy link
Member

@michael-s-molina do you mind attaching BEFORE and AFTER video/gif? thanks!

@codecov
Copy link

codecov bot commented Jul 28, 2021

Codecov Report

Merging #15925 (6f8dcf2) into master (eb78f43) will decrease coverage by 0.00%.
The diff coverage is 56.00%.

❗ Current head 6f8dcf2 differs from pull request most recent head 897c9bc. Consider uploading reports for the commit 897c9bc to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15925      +/-   ##
==========================================
- Coverage   76.98%   76.98%   -0.01%     
==========================================
  Files         988      988              
  Lines       52379    52313      -66     
  Branches     6623     6623              
==========================================
- Hits        40326    40273      -53     
+ Misses      11829    11817      -12     
+ Partials      224      223       -1     
Flag Coverage Δ
javascript 71.53% <56.00%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
...nativeFilters/FiltersConfigModal/Footer/Footer.tsx 100.00% <ø> (+5.26%) ⬆️
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx 73.13% <47.61%> (-1.44%) ⬇️
...eFilters/FiltersConfigModal/FiltersConfigModal.tsx 95.40% <100.00%> (+0.10%) ⬆️
superset/charts/post_processing.py 14.63% <0.00%> (-65.37%) ⬇️
superset-frontend/src/views/CRUD/utils.tsx 64.16% <0.00%> (-4.43%) ⬇️
superset/reports/notifications/slack.py 87.50% <0.00%> (-2.67%) ⬇️
...set-frontend/src/views/CRUD/welcome/ChartTable.tsx 72.83% <0.00%> (-1.85%) ⬇️
...perset-frontend/src/views/CRUD/welcome/Welcome.tsx 81.30% <0.00%> (-1.52%) ⬇️
...frontend/src/views/CRUD/welcome/DashboardTable.tsx 64.51% <0.00%> (-1.42%) ⬇️
superset/reports/notifications/base.py 95.65% <0.00%> (-0.19%) ⬇️
... and 20 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 eb78f43...897c9bc. Read the comment docs.

@michael-s-molina
Copy link
Member Author

/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true

@github-actions
Copy link
Contributor

@michael-s-molina Ephemeral environment spinning up at http://54.188.167.53:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@junlincc junlincc added the dashboard:native-filters Related to the native filters of the Dashboard label Jul 28, 2021
@geido
Copy link
Member

geido commented Jul 29, 2021

Hello @michael-s-molina. Thanks for the fix.

I have noticed that when changing an existing filter and then creating a new one, when saving it says "1 unsaved filter". However, I would expect that to say "2 unsaved filters" as I did changes to two filters that were not saved yet. This is a minor thing though.

scroll.inconsist.mp4

@michael-s-molina
Copy link
Member Author

michael-s-molina commented Jul 29, 2021

Hello @michael-s-molina. Thanks for the fix.

I have noticed that when changing an existing filter and then creating a new one, when saving it says "1 unsaved filter". However, I would expect that to say "2 unsaved filters" as I did changes to two filters that were not saved yet. This is a minor thing though.

scroll.inconsist.mp4

@geido I made a change to make the cancel message more generic because tracking exactly which filter changed would require additional logic. Also because the previous strategy of listing the names of the filters that changed is not suitable when the number of changed filters is high.

@michael-s-molina
Copy link
Member Author

/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true

@github-actions
Copy link
Contributor

@michael-s-molina Ephemeral environment spinning up at http://34.213.224.190:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@jinghua-qa jinghua-qa left a comment

Choose a reason for hiding this comment

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

LGTM!

cancel.alert.mov

@michael-s-molina michael-s-molina merged commit cc704dd into apache:master Jul 30, 2021
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added the 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels label 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 dashboard:native-filters Related to the native filters of the Dashboard size/M 🚢 1.3.0
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants