Implement Member Update Endpoint - #73
Conversation
26b2aa9 to
73a6032
Compare
Allimonae
changed the title
Graphite Automations"Request reviewers once CI passes" took an action on this PR • (08/10/26)2 reviewers were added to this PR based on Henry Chen's automation. |
|
| } | ||
|
|
||
| private void updateOptional(Member member, UpdateMemberRequest request) { | ||
| request.linkedInUrl().ifPresent(member::setLinkedInUrl); |
There was a problem hiding this comment.
Is the only difference here between this and the validate function that you throw an exception? I feel like it'd be easier to have a mandatory flag and then you can use the helper for these as well.
|
|
||
| @Override | ||
| public Optional<Member> getMemberByEmail(String email) { | ||
| String sql = "SELECT * FROM members WHERE email = :email"; |
There was a problem hiding this comment.
Not part of the PR directly but I just realized, is this just used for the create member exists check? If we just need a yes/no we could theoretically just use SELECT EXISTS without returning all of the columns?
I don't recall if email is indexed but if it is, even better
| throw new ValidationException("email cannot be empty"); | ||
| } | ||
| if (!email.equals(member.getEmail()) | ||
| && memberRepo.getMemberByEmail(email).isPresent()) { |
There was a problem hiding this comment.
Shouldn't this field already be unique in the DB?
I feel like it's better to let the DB handle it rather than do a full scan ourselves.
The only thing may be, I'm not sure if the JDBC exception lets us see which column violated the unique constraint. It is nicer that we have a clear exception that tells the user that the email is the culprit.
Regardless, adding onto my other comment about returning everything, I think this should be at least replaced with just a true/false return.
| void updateMember_badRequestWhenValidationFails() throws Exception { | ||
| final UUID id = UUID.randomUUID(); | ||
| final UpdateMemberRequest request = new UpdateMemberRequest( | ||
| Optional.empty(), |
There was a problem hiding this comment.
Not sure if this is an actual expected case, but we probably shouldn't run an update when the update is empty (it would also change the updated at time)
maybe have a flag that we set to true only if any fields are present, and if not, we abort the update?
|
|
||
| when(memberService.updateMember(any(), any())).thenThrow(new ValidationException("firstName cannot be empty")); | ||
|
|
||
| mockMvc.perform(patch("/api/members/{id}", id) |
There was a problem hiding this comment.
Nit: you could probably have some test helpers for success/failure code to not have to rewrite this every time
| void updateMember_throwsExceptionWhenEmailIsDuplicate() { | ||
| final UUID id = UUID.randomUUID(); | ||
| final UUID otherMemberId = UUID.randomUUID(); | ||
| final UpdateMemberRequest request = new UpdateMemberRequest( |
There was a problem hiding this comment.
Another nit - can add constants for these in a third test helper file so you're not rewriting them for essentially the same tests in member services tests and member controller tests.
2 participants
Implement Member Update Endpoint
Summary
Add a PATCH
/members/{id}endpoint to update member profiles with support for partial updates, comprehensive validation, and proper error handling.Changes
updateMemberendpoint with@Validrequest validationUpdateMemberRequestto useOptional<String>fields to distinguish between "field not provided" (null) and "field provided as empty" (blank validation)ValidationExceptionand handler for service-level validation failuresDesign Notes
Testing
Testing Null vs Blank