-
Notifications
You must be signed in to change notification settings - Fork 0
Update Member: Remove email change support #99
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -231,30 +231,6 @@ | |
| .andExpect(jsonPath("$.payload.firstName").value("UpdatedFirstName")); | ||
| } | ||
|
|
||
| @Test | ||
| void updateMember_badRequestWhenBlankRequiredField() throws Exception { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed since this test is redundant with updateMember_badRequestWhenValidationFails |
||
| final UUID id = UUID.randomUUID(); | ||
| final UpdateMemberRequest request = new UpdateMemberRequest( | ||
| Optional.of(""), | ||
| Optional.of("UpdatedLastName"), | ||
| Optional.of("updated@example.com"), | ||
| Optional.empty(), | ||
| Optional.empty(), | ||
| Optional.empty(), | ||
| Optional.empty(), | ||
| Optional.empty(), | ||
| Optional.empty(), | ||
| Optional.empty()); | ||
|
|
||
| when(memberService.updateMember(any(), any())).thenThrow(new ValidationException("firstName cannot be empty")); | ||
|
|
||
| mockMvc.perform(patch("/api/members/{id}", id) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .content(objectMapper.writeValueAsString(request))) | ||
| .andExpect(status().isBadRequest()) | ||
| .andExpect(jsonPath("$.message").value("firstName cannot be empty")); | ||
| } | ||
|
|
||
| @Test | ||
| void updateMember_notFoundWhenMemberDoesNotExist() throws Exception { | ||
| final UUID id = UUID.randomUUID(); | ||
|
|
@@ -280,30 +256,30 @@ | |
| .andExpect(jsonPath("$.success").value(false)); | ||
| } | ||
|
|
||
| @Test | ||
| void updateMember_conflictWhenEmailIsDuplicate() throws Exception { | ||
| final UUID id = UUID.randomUUID(); | ||
| final UpdateMemberRequest request = new UpdateMemberRequest( | ||
| Optional.empty(), | ||
| Optional.empty(), | ||
| Optional.of("existing@example.com"), | ||
| Optional.empty(), | ||
| Optional.empty(), | ||
| Optional.empty(), | ||
| Optional.empty(), | ||
| Optional.empty(), | ||
| Optional.empty(), | ||
| Optional.empty()); | ||
|
|
||
| when(memberService.updateMember(any(UpdateMemberRequest.class), eq(id))) | ||
| .thenThrow(new MemberDuplicateException("existing@example.com")); | ||
|
|
||
| mockMvc.perform(patch("/api/members/{id}", id) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .content(objectMapper.writeValueAsString(request))) | ||
| .andExpect(status().isConflict()) | ||
| .andExpect(jsonPath("$.success").value(false)); | ||
| } | ||
| // @Test | ||
| // void updateMember_conflictWhenEmailIsDuplicate() throws Exception { | ||
|
Check warning on line 260 in src/test/java/org/patinanetwork/patchats/api/member/MemberControllerTest.java
|
||
| // final UUID id = UUID.randomUUID(); | ||
| // final UpdateMemberRequest request = new UpdateMemberRequest( | ||
| // Optional.empty(), | ||
| // Optional.empty(), | ||
| // Optional.of("existing@example.com"), | ||
| // Optional.empty(), | ||
| // Optional.empty(), | ||
| // Optional.empty(), | ||
| // Optional.empty(), | ||
| // Optional.empty(), | ||
| // Optional.empty(), | ||
| // Optional.empty()); | ||
|
|
||
| // when(memberService.updateMember(any(UpdateMemberRequest.class), eq(id))) | ||
| // .thenThrow(new MemberDuplicateException("existing@example.com")); | ||
|
Check warning on line 275 in src/test/java/org/patinanetwork/patchats/api/member/MemberControllerTest.java
|
||
|
|
||
| // mockMvc.perform(patch("/api/members/{id}", id) | ||
| // .contentType(MediaType.APPLICATION_JSON) | ||
| // .content(objectMapper.writeValueAsString(request))) | ||
| // .andExpect(status().isConflict()) | ||
| // .andExpect(jsonPath("$.success").value(false)); | ||
|
Check warning on line 281 in src/test/java/org/patinanetwork/patchats/api/member/MemberControllerTest.java
|
||
| // } | ||
|
|
||
| @Test | ||
| void updateMember_badRequestWhenValidationFails() throws Exception { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ | |
| import org.patinanetwork.patchats.api.member.dto.UpdateMemberRequest; | ||
| import org.patinanetwork.patchats.common.web.exception.MemberDuplicateException; | ||
| import org.patinanetwork.patchats.common.web.exception.MemberNotFoundException; | ||
| import org.springframework.dao.DuplicateKeyException; | ||
| import org.patinanetwork.patchats.common.web.exception.ValidationException; | ||
|
|
||
| class MemberServiceTest { | ||
|
|
||
|
|
@@ -272,7 +272,6 @@ | |
|
|
||
| assertEquals("UpdatedFirstName", captured.getFirstName()); | ||
| assertEquals("UpdatedLastName", captured.getLastName()); | ||
| assertEquals("updated@example.com", captured.getEmail()); | ||
| assertEquals("https://linkedin.com/in/updated", captured.getLinkedInUrl()); | ||
| assertEquals("Updated intro", captured.getIntroduction()); | ||
| assertEquals("Mentor - I am looking for guidance from someone with more experience", captured.getMatchPref()); | ||
|
|
@@ -282,13 +281,40 @@ | |
| assertEquals("Notes", captured.getExtraNotes()); | ||
| } | ||
|
|
||
| // @Test | ||
| // void updateMember_throwsExceptionWhenEmailIsDuplicate() { | ||
|
Check warning on line 285 in src/test/java/org/patinanetwork/patchats/api/member/MemberServiceTest.java
|
||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i just commented out the code, no idea why its not lining up correctly |
||
| // final UUID id = UUID.randomUUID(); | ||
| // final UpdateMemberRequest request = new UpdateMemberRequest( | ||
| // Optional.empty(), | ||
| // Optional.empty(), | ||
| // Optional.of("existing@example.com"), | ||
| // Optional.empty(), | ||
| // Optional.empty(), | ||
| // Optional.empty(), | ||
| // Optional.empty(), | ||
| // Optional.empty(), | ||
| // Optional.empty(), | ||
| // Optional.empty()); | ||
|
|
||
| // final Member existingMember = Member.builder() | ||
| // .id(id) | ||
| // .firstName("John") | ||
| // .lastName("Doe") | ||
| // .email("john@example.com") | ||
| // .build(); | ||
|
|
||
| // when(memberRepo.updateMember(any(Member.class))).thenThrow(new DuplicateKeyException("Email already | ||
| // exists")); | ||
| // when(memberRepo.getMemberById(id)).thenReturn(Optional.of(existingMember)); | ||
|
Check warning on line 308 in src/test/java/org/patinanetwork/patchats/api/member/MemberServiceTest.java
|
||
| // assertThrows(MemberDuplicateException.class, () -> memberService.updateMember(request, id)); | ||
| // } | ||
| @Test | ||
| void updateMember_throwsExceptionWhenEmailIsDuplicate() { | ||
| void updateMember_throwsValidationExceptionWhenEmailIsChanged() { | ||
| final UUID id = UUID.randomUUID(); | ||
| final UpdateMemberRequest request = new UpdateMemberRequest( | ||
| Optional.empty(), | ||
| Optional.empty(), | ||
| Optional.of("existing@example.com"), | ||
| Optional.of("new@example.com"), | ||
| Optional.empty(), | ||
| Optional.empty(), | ||
| Optional.empty(), | ||
|
|
@@ -304,16 +330,17 @@ | |
| .email("john@example.com") | ||
| .build(); | ||
|
|
||
| when(memberRepo.updateMember(any(Member.class))).thenThrow(new DuplicateKeyException("Email already exists")); | ||
| when(memberRepo.getMemberById(id)).thenReturn(Optional.of(existingMember)); | ||
| assertThrows(MemberDuplicateException.class, () -> memberService.updateMember(request, id)); | ||
|
|
||
| assertThrows(ValidationException.class, () -> memberService.updateMember(request, id)); | ||
| verify(memberRepo, never()).updateMember(any()); | ||
| } | ||
|
|
||
| @Test | ||
| void updateMember_successWhenUpdatingWithSameEmail() { | ||
| void updateMember_successWhenUpdatingOtherFieldsWithUnchangedEmail() { | ||
| final UUID id = UUID.randomUUID(); | ||
| final UpdateMemberRequest request = new UpdateMemberRequest( | ||
| Optional.empty(), | ||
| Optional.of("UpdatedFirstName"), | ||
| Optional.empty(), | ||
| Optional.of("john@example.com"), | ||
| Optional.empty(), | ||
|
|
@@ -329,25 +356,19 @@ | |
| .firstName("John") | ||
| .lastName("Doe") | ||
| .email("john@example.com") | ||
| .linkedInUrl("https://linkedin.com/in/john") | ||
| .introduction("intro") | ||
| .matchPref("Mentor") | ||
| .industryPref("Tech") | ||
| .rolePref("Engineer") | ||
| .topics("AI") | ||
| .extraNotes("notes") | ||
| .build(); | ||
|
|
||
| when(memberRepo.getMemberById(id)).thenReturn(Optional.of(existingMember)); | ||
| when(memberRepo.updateMember(any())).thenReturn(Optional.of(existingMember)); | ||
| when(memberRepo.updateMember(any(Member.class))).thenReturn(Optional.of(existingMember)); | ||
|
|
||
| final ArgumentCaptor<Member> captor = ArgumentCaptor.forClass(Member.class); | ||
| memberService.updateMember(request, id); | ||
|
|
||
| verify(memberRepo).updateMember(captor.capture()); | ||
| final Member captured = captor.getValue(); | ||
|
|
||
| assertEquals("UpdatedFirstName", captured.getFirstName()); | ||
| assertEquals("john@example.com", captured.getEmail()); | ||
| verify(memberRepo, never()).getMemberByEmail(any()); | ||
| } | ||
|
|
||
| @Test | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,7 @@ final class MemberTestFixtures { | |
| static final UpdateMemberRequest UPDATE_REQUEST_ALL_FIELDS = new UpdateMemberRequest( | ||
| Optional.of("UpdatedFirstName"), | ||
| Optional.of("UpdatedLastName"), | ||
| Optional.of("updated@example.com"), | ||
| Optional.empty(), // Email changes are not currently supported | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is no representative of how the data actually comes in from the frontend, right?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. email changes are always disabled from the frontend so it will always be empty |
||
| Optional.of("https://linkedin.com/in/updated"), | ||
| Optional.of("Updated intro"), | ||
| Optional.of("Mentor - I am looking for guidance from someone with more experience"), | ||
|
|
||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.