Update Member: Remove email change support - #99
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
4ea8437 to
f146634
Compare
1421ff6 to
3d7816e
Compare
f146634 to
0bcc642
Compare
Graphite Automations"Request reviewers once CI passes" took an action on this PR • (08/26/26)2 reviewers were added to this PR based on Henry Chen's automation. |
0bcc642 to
118a34f
Compare
| Member updatedMember = memberRepo.updateMember(member).orElseThrow(() -> new MemberNotFoundException(id)); | ||
| return MemberDto.from(updatedMember); | ||
| } catch (DuplicateKeyException e) { | ||
| throw new MemberDuplicateException(member.getEmail()); |
There was a problem hiding this comment.
why change this block? i get that we don't need it for now and we can enable later, but it leaves us prone to forgetting to add it back.
There was a problem hiding this comment.
the only argument i could see for removing this is that we're not unnecessarily querying the DB here, but even then it should be commented out with a note to re enable when email verification is added later.
| .contentType(MediaType.APPLICATION_JSON) | ||
| .content(objectMapper.writeValueAsString(request))) | ||
| .andExpect(status().isConflict()) | ||
| .andExpect(jsonPath("$.success").value(false)); |
There was a problem hiding this comment.
why not leave this in and comment it out for when we do enable?
There was a problem hiding this comment.
I'm planning on making a separate verification flow for email changes, so even if I were to keep the test case it would still be temporary
There was a problem hiding this comment.
The idea is more about not losing the work you did to write that test logic. Even if it gets moved to a different service, you can repurpose it for later.
| void updateMember_successWhenUpdatingOtherFieldsWithUnchangedEmail() { | ||
| final UUID id = UUID.randomUUID(); | ||
| final UpdateMemberRequest request = new UpdateMemberRequest( | ||
| Optional.empty(), |
There was a problem hiding this comment.
same comment as above - we should comment out this test instead of removing it if we're going to add back the logic later
|
also add a test in for the validation exception being thrown |
| Optional.of("UpdatedFirstName"), | ||
| Optional.of("UpdatedLastName"), | ||
| Optional.of("updated@example.com"), | ||
| Optional.empty(), // Email changes are not currently supported |
There was a problem hiding this comment.
This is no representative of how the data actually comes in from the frontend, right?
There was a problem hiding this comment.
email changes are always disabled from the frontend so it will always be empty
2ca7a7d to
57ce67c
Compare
57ce67c to
512ed6f
Compare
|
| } | ||
|
|
||
| @Test | ||
| void updateMember_badRequestWhenBlankRequiredField() throws Exception { |
There was a problem hiding this comment.
removed since this test is redundant with updateMember_badRequestWhenValidationFails
| } | ||
|
|
||
| // @Test | ||
| // void updateMember_throwsExceptionWhenEmailIsDuplicate() { |
There was a problem hiding this comment.
i just commented out the code, no idea why its not lining up correctly
3 participants
Disables email changes in member profile updates until email change verification is implemented. Removed and updated some tests.
What changed?
The email field in the member profile form is now always disabled, with an updated description informing users that email changes are not currently supported. On the backend,
updateMembernow throws aValidationExceptionif an email change is attempted. The previously existing duplicate email handling logic (including theDuplicateKeyExceptioncatch block andMemberDuplicateException) has been removed from the update flow since it is no longer reachable.Why make this change?
Email changes require a verification flow (e.g., confirming ownership of the new address) that has not yet been implemented. Disabling this functionality prevents unverified email changes until proper verification support is in place.