-
Notifications
You must be signed in to change notification settings - Fork 32
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: budget invite duplicate text entry bug #1422
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1422 +/- ##
=======================================
Coverage 86.46% 86.46%
=======================================
Files 660 660
Lines 14921 14921
Branches 3167 3167
=======================================
Hits 12902 12902
Misses 1949 1949
Partials 70 70 ☔ View full report in Codecov by Sentry. |
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.
Great work. Works very smoothly now.
Could we add a test for this? I feel this could have ideally been caught by a test in the first place, and if we don't add a test now, we risk this breaking again in the future.
Maybe it would be a good time to have a look if there's any other aspect of this feature that's not sufficiently tested while doing that?
951e815
to
ebf4395
Compare
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.
If you don't mind, could you just have another look at the test? I left a comment there.
@@ -169,6 +169,20 @@ describe('<InviteMemberModal />', () => { | |||
expect(screen.getByText(`Summary (${mockLearnerEmails.length})`)).toBeInTheDocument(); | |||
}, { timeout: EMAIL_ADDRESSES_INPUT_VALUE_DEBOUNCE_DELAY + 1000 }); | |||
}); | |||
it('shows no errors for valid manual input', async () => { |
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.
Hey as far as I can tell, this test checks that when you enter a valid email, it doesn't display an error. I guess it's a good test to have if it isn't there yet, but not quite sure how it tests the bug that was fixed here?
I noticed that when I change the code back to the previous broken state, the test still succeeds. I am feeling that that could be a bit misleading and instill false confidence.
What do you think about adding a test should not produce a duplicate email when editing an email
? The test would enter an email address, wait for the debounce, and then edit the same line. It would then not just look for the warning but actually check if a duplicate was in fact produced. And it would be great to make sure that the test fails until the change you made here is active, so it doesn't create false positives.
3c1a63e
to
1728187
Compare
test: member invite shows no errors for valid manual input test: member invite shows no errors for valid manual input pt.2 test: remove unneeded test test: does not throw up spurious warnings for duplicated emails on edit
1728187
to
bb38c13
Compare
Jira Ticket
This change fixes an issue around duplicate emails being input on the Learner Credit Browse and Enroll Add Members modal.
Testing
npm run start:stage
For all changes
Only if submitting a visual change