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: alert modal bug fix #22688

Conversation

AAfghahi
Copy link
Member

SUMMARY

Due to some refactoring, we were no longer updating the dropdown in the alert/report modal when a user switched from one alert option to another.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

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

@AAfghahi AAfghahi force-pushed the arash.afghahi/sc-65239/alerts-reports-the-same-list-is-displayed branch from df8941b to abbdd78 Compare January 11, 2023 17:59
@codecov
Copy link

codecov bot commented Jan 11, 2023

Codecov Report

Merging #22688 (1024cd6) into master (0b22287) will decrease coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #22688      +/-   ##
==========================================
- Coverage   67.07%   66.92%   -0.16%     
==========================================
  Files        1869     1870       +1     
  Lines       71579    72501     +922     
  Branches     7806     8162     +356     
==========================================
+ Hits        48015    48519     +504     
- Misses      21536    21910     +374     
- Partials     2028     2072      +44     
Flag Coverage Δ
hive 52.60% <ø> (ø)
javascript 53.89% <100.00%> (-0.07%) ⬇️
mysql 78.01% <ø> (?)
postgres 78.08% <ø> (+0.01%) ⬆️
presto 52.49% <ø> (ø)
python 81.31% <ø> (+0.04%) ⬆️
sqlite 76.48% <ø> (ø)
unit 51.47% <ø> (ø)

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

Impacted Files Coverage Δ
...set-frontend/src/components/Select/AsyncSelect.tsx 88.52% <100.00%> (+0.28%) ⬆️
...ns/legacy-plugin-chart-map-box/src/controlPanel.ts 23.07% <0.00%> (-6.93%) ⬇️
...perset-frontend/src/views/components/RightMenu.tsx 77.43% <0.00%> (-0.80%) ⬇️
...ase/DatabaseModal/DatabaseConnectionForm/index.tsx 90.47% <0.00%> (-0.44%) ⬇️
...d/src/dashboard/components/gridComponents/Tabs.jsx 80.57% <0.00%> (-0.24%) ⬇️
superset-frontend/src/types/bootstrapTypes.ts 100.00% <0.00%> (ø)
...et-frontend/src/explore/controlPanels/sections.tsx 100.00% <0.00%> (ø)
...rontend/src/filters/components/Range/buildQuery.ts 0.00% <0.00%> (ø)
...d/src/SqlLab/components/QueryLimitSelect/index.tsx 100.00% <0.00%> (ø)
...ackages/superset-ui-core/src/utils/featureFlags.ts 100.00% <0.00%> (ø)
... and 40 more

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

@@ -442,7 +442,7 @@ const AsyncSelect = forwardRef(
fetchedQueries.current.clear();
setAllValuesLoaded(false);
setSelectOptions(initialOptions);
}, [initialOptions]);
}, [initialOptions, options]);
Copy link
Member

Choose a reason for hiding this comment

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

@AAfghahi can you add more context to the PR description? I'm trying to understand the problem because initialOptions is calculated from options:

const initialOptions = useMemo(
  () => options && Array.isArray(options) ? options.slice() : EMPTY_OPTIONS,
  [options],
);

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah! So, in AlertReportModal, what we are sending over to options is not an array, it returns this:
https://github.com/apache/superset/blob/master/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx#L811

so this fails the Array.isArray(options) and does not update initialOptions, and leaves initialOptions as EMPTY_OPTIONS. This is what was causing the behavior from before.

Copy link
Member

Choose a reason for hiding this comment

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

I understood. I think we had a problem during the refactoring. For the async Select, we don't have initial options so we can remove the references to it. I'm sending you a patch with the changes. Let me know if AlertReportModal works after applying it with git apply patch.txt

patch.txt

@AAfghahi
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

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

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM

@michael-s-molina michael-s-molina merged commit 5a422b3 into master Jan 12, 2023
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
@mistercrunch mistercrunch deleted the arash.afghahi/sc-65239/alerts-reports-the-same-list-is-displayed branch March 26, 2024 16:07
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 size/S 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants