-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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: remove reason when password change is set to false #31363
Conversation
🦋 Changeset detectedLatest commit: 2d1b2bb The changes in this PR will be included in the next version bump. This PR includes changesets to release 32 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #31363 +/- ##
===========================================
- Coverage 56.39% 55.91% -0.48%
===========================================
Files 2437 2386 -51
Lines 53773 52826 -947
Branches 11082 10843 -239
===========================================
- Hits 30325 29539 -786
+ Misses 20802 20672 -130
+ Partials 2646 2615 -31
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test case to cover this?
Not for this case, since we cannot simulate the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test that covers this part of the function - even if you can't validate this specific bug in an api test, we would at least have a test running the functionality
Looks like this PR is ready to merge! 🎉 |
@pierre-lehnen-rc, I didn't find a way to test it. |
E2E can cover this, just create an user with the reason set, then write a test that checks if the reason is removed when the flag is set to false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a changeset?
Proposed changes (including videos or screenshots)
Remove the
requirePasswordChangeReason
when the fieldrequirePasswordChange
is set to false.Issue(s)
Steps to test or reproduce
Further comments
SUP-444