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(ar-modal): updateNotificationSettings not updating state #28409

Merged

Conversation

fisjac
Copy link
Contributor

@fisjac fisjac commented May 9, 2024

SUMMARY

When changing the recipients list for an existing notification method, the state is not being changed properly. This change addresses this by reinstating missing code that was lost in a previous PR.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

image

image

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue: Unable to save report due to Add button not enable #28149 (comment)
  • Required feature flags: ALERT_REPORTS
  • 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

@fisjac fisjac force-pushed the ar-modal/fix-updateNotificationSettings branch 2 times, most recently from 10fb0a3 to 632458c Compare May 9, 2024 17:09
@fisjac fisjac force-pushed the ar-modal/fix-updateNotificationSettings branch from 632458c to dd49c60 Compare May 9, 2024 17:13
Copy link
Contributor

@sfirke sfirke left a comment

Choose a reason for hiding this comment

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

I can't speak to the code itself sorry but the bug is real and it's addressing the right part of the codebase. Thanks @fisjac !

@alexkyurtev
Copy link

Hello , when do you expect the PR to be merged ?

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

can you add a unit test for updateNotificationSetting

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

Rush-merging since this needs to make an imminent cut.

@mistercrunch mistercrunch merged commit d871b4d into apache:master May 13, 2024
30 checks passed
@michael-s-molina michael-s-molina added the v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch label May 13, 2024
michael-s-molina pushed a commit that referenced this pull request May 13, 2024
aehanno pushed a commit to kosmos-education/superset that referenced this pull request May 13, 2024
@alexkyurtev
Copy link

Hello , I suppose that the changes are live now ? Or ?

jzhao62 pushed a commit to jzhao62/superset that referenced this pull request May 16, 2024
@sfirke
Copy link
Contributor

sfirke commented May 16, 2024

These changes would be live on master branch (you could run the nightly, unstable build image apache/superset:latest to verify) but did not make it into 4.0.1. They will be present in 4.0.2 and 4.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants