feat: 약관 동의 저장 기능 추가 #33
Conversation
Summary of ChangesHello @slg1119, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new feature to handle user's terms and conditions agreement. It includes adding a new controller, service, repository, and DTOs to manage terms agreement. Also, it modifies the security configuration to allow public access to the terms endpoint and adds logic to delete terms agreement information upon user withdrawal. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces functionality to manage user terms agreements, including new API endpoints, service logic, and repository interfaces. It also integrates terms agreement deletion into the member withdrawal process and adds corresponding tests. The changes are well-structured and include appropriate validation and error handling.
| .requestMatchers("/error").permitAll() | ||
| .requestMatchers("/error/**").permitAll() | ||
| .requestMatchers("/actuator/**").permitAll() | ||
| .requestMatchers("/terms").permitAll() |
There was a problem hiding this comment.
The /terms endpoint is now publicly accessible. While /terms itself might be public for viewing general terms, /terms/me and /terms/me (POST) are protected by PreAuthorize. Ensure that the public access to /terms is intentional and does not expose any sensitive information or allow unauthorized actions, especially if there are plans to add more sub-paths under /terms that should be protected.
|
|
||
| Optional<MemberTermsAgreement> findByMemberIdAndTermsId(UUID memberId, Long termsId); | ||
|
|
||
| void deleteAllByMemberId(UUID memberId); |
There was a problem hiding this comment.
The deleteAllByMemberId method is used to remove all terms agreements for a given member. While this is functional, consider using a @Modifying query with @Query annotation for bulk deletion, which can be more efficient than fetching all entities and then deleting them one by one, especially for a large number of agreements. This can improve performance during member withdrawal.
| if (existingAgreement != null) { | ||
| // 기존 레코드가 있으면 삭제 후 새로 생성 (업데이트) | ||
| memberTermsAgreementRepository.delete(existingAgreement); | ||
| } |
There was a problem hiding this comment.
The current implementation deletes an existing MemberTermsAgreement and then saves a new one to update it. This approach can lead to unnecessary database operations (delete then insert) and potential issues with database triggers or auditing if not handled carefully. A more efficient approach would be to update the isAgreed status of the existingAgreement directly if it exists, and only create a new one if it doesn't.
if (existingAgreement != null) {
existingAgreement.setIsAgreed(agreementRequest.getIsAgreed());
memberTermsAgreementRepository.save(existingAgreement);
} else {| // 새로운 동의 정보 저장 | ||
| MemberTermsAgreement newAgreement = MemberTermsAgreement.builder() | ||
| .memberId(memberId) | ||
| .termsId(agreementRequest.getTermId()) | ||
| .isAgreed(agreementRequest.getIsAgreed()) | ||
| .build(); | ||
|
|
||
| memberTermsAgreementRepository.save(newAgreement); |
There was a problem hiding this comment.
Following up on the previous comment, if an existing agreement is found, it should be updated instead of deleted and re-created. The else block should contain the creation of a new agreement only if existingAgreement is null.
} else {
// 새로운 동의 정보 저장
MemberTermsAgreement newAgreement = MemberTermsAgreement.builder()
.memberId(memberId)
.termsId(agreementRequest.getTermId())
.isAgreed(agreementRequest.getIsAgreed())
.build();
memberTermsAgreementRepository.save(newAgreement);
}There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive terms and conditions (T&C) management system, allowing users to view all terms, check their agreement status, and save/update their term agreements. The implementation includes REST API endpoints, service layer logic with validation, and data persistence.
Key Changes:
- Added term management REST API endpoints with authentication and comprehensive API documentation
- Implemented business logic for retrieving, validating, and persisting term agreements
- Integrated term agreement deletion into member withdrawal flow
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
TermController.java |
Implements REST endpoints for retrieving terms and managing user agreements |
TermService.java |
Service interface defining term management operations |
TermServiceImpl.java |
Service implementation with validation logic and agreement persistence |
TermRepository.java |
Data access layer for Term entity |
TermDto.java |
DTOs for term requests and responses with validation annotations |
MemberTermsAgreementRepository.java |
Data access layer for member term agreements |
MemberServiceImpl.java |
Updated to delete term agreements on member withdrawal |
SecurityConfig.java |
Configured public access to terms list endpoint |
TermControllerTest.java |
Comprehensive controller integration tests with API documentation |
TermServiceImplTest.java |
Service layer unit tests covering all business logic |
MemberServiceImplTest.java |
Added test for term agreement deletion on withdrawal |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 기존 레코드가 있으면 삭제 후 새로 생성 (업데이트) | ||
| memberTermsAgreementRepository.delete(existingAgreement); | ||
| } | ||
|
|
||
| // 새로운 동의 정보 저장 | ||
| MemberTermsAgreement newAgreement = MemberTermsAgreement.builder() | ||
| .memberId(memberId) | ||
| .termsId(agreementRequest.getTermId()) | ||
| .isAgreed(agreementRequest.getIsAgreed()) | ||
| .build(); | ||
|
|
||
| memberTermsAgreementRepository.save(newAgreement); |
There was a problem hiding this comment.
The delete-then-insert pattern for updates is inefficient and may cause unnecessary database operations. Consider implementing an update method on the entity or using a single upsert operation. If the BaseEntity class provides timestamp tracking, the current approach also loses the original creation timestamp.
| // 기존 레코드가 있으면 삭제 후 새로 생성 (업데이트) | |
| memberTermsAgreementRepository.delete(existingAgreement); | |
| } | |
| // 새로운 동의 정보 저장 | |
| MemberTermsAgreement newAgreement = MemberTermsAgreement.builder() | |
| .memberId(memberId) | |
| .termsId(agreementRequest.getTermId()) | |
| .isAgreed(agreementRequest.getIsAgreed()) | |
| .build(); | |
| memberTermsAgreementRepository.save(newAgreement); | |
| // 기존 레코드가 있으면 업데이트 | |
| existingAgreement.setIsAgreed(agreementRequest.getIsAgreed()); | |
| memberTermsAgreementRepository.save(existingAgreement); | |
| } else { | |
| // 새로운 동의 정보 저장 | |
| MemberTermsAgreement newAgreement = MemberTermsAgreement.builder() | |
| .memberId(memberId) | |
| .termsId(agreementRequest.getTermId()) | |
| .isAgreed(agreementRequest.getIsAgreed()) | |
| .build(); | |
| memberTermsAgreementRepository.save(newAgreement); | |
| } |
| import java.time.LocalDateTime; | ||
| import java.util.List; | ||
| import kr.swyp.backend.authentication.provider.TokenProvider; | ||
| import kr.swyp.backend.common.desciptor.ErrorDescriptor; |
There was a problem hiding this comment.
Corrected spelling of 'desciptor' to 'descriptor'.
| import kr.swyp.backend.common.desciptor.ErrorDescriptor; | |
| import kr.swyp.backend.common.descriptor.ErrorDescriptor; |
Summary of ChangesHello @slg1119, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new feature to handle the storage and retrieval of user's terms and conditions agreements. It includes new API endpoints for managing terms agreement, a repository for storing the agreement information, and updates to the member withdrawal process to ensure terms agreement data is properly removed upon user's account deletion. Security configurations have also been updated to allow public access to the /terms endpoint. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
| @Override | ||
| @Transactional | ||
| public void saveMemberTermsAgreements(UUID memberId, TermsAgreementRequest request) { | ||
| for (TermAgreementRequest agreementRequest : request.getAgreements()) { | ||
| // 약관 존재 여부 확인 | ||
| Term term = termRepository.findById(agreementRequest.getTermId()) | ||
| .orElseThrow(() -> new NoSuchElementException( | ||
| "약관을 찾을 수 없습니다. ID: " + agreementRequest.getTermId())); | ||
|
|
||
| // 필수 약관인데 동의하지 않은 경우 예외 발생 | ||
| if (term.getIsRequired() && !agreementRequest.getIsAgreed()) { | ||
| throw new IllegalArgumentException( | ||
| "필수 약관에 동의해야 합니다: " + term.getTitle()); | ||
| } | ||
|
|
||
| // 기존 동의 정보 조회 | ||
| MemberTermsAgreement existingAgreement = | ||
| memberTermsAgreementRepository.findByMemberIdAndTermsId( | ||
| memberId, agreementRequest.getTermId() | ||
| ).orElse(null); | ||
|
|
||
| if (existingAgreement != null) { | ||
| // 기존 레코드가 있으면 삭제 후 새로 생성 (업데이트) | ||
| memberTermsAgreementRepository.delete(existingAgreement); | ||
| } | ||
|
|
||
| // 새로운 동의 정보 저장 | ||
| MemberTermsAgreement newAgreement = MemberTermsAgreement.builder() | ||
| .memberId(memberId) | ||
| .termsId(agreementRequest.getTermId()) | ||
| .isAgreed(agreementRequest.getIsAgreed()) | ||
| .build(); | ||
|
|
||
| memberTermsAgreementRepository.save(newAgreement); | ||
| } | ||
| } |
There was a problem hiding this comment.
현재 약관 동의 저장 로직은 요청에 포함된 각 약관에 대해 데이터베이스 조회, 삭제, 저장을 반복하고 있습니다. 이 방식은 요청하는 약관의 수(N)에 비례하여 DB I/O가 증가하여(N번의 SELECT, N번의 DELETE, N번의 INSERT 가능성) 비효율적입니다.
또한, 이 로직은 요청에 포함되지 않은 기존 약관 동의 정보를 처리하지 않아, 사용자가 선택 약관 동의를 철회하는 시나리오(요청에서 해당 약관을 제외)에서 기존 동의가 삭제되지 않는 문제가 발생할 수 있습니다.
전체 약관 동의 상태를 한 번에 교체하는 방식으로 로직을 개선하는 것을 제안합니다. 즉, 특정 회원의 기존 약관 동의를 모두 삭제한 후, 요청받은 약관 동의 목록을 한 번에 저장하는 것입니다. 이렇게 하면 DB 접근 횟수를 크게 줄일 수 있고 로직이 명확해집니다.
@Override
@Transactional
public void saveMemberTermsAgreements(UUID memberId, TermsAgreementRequest request) {
List<TermAgreementRequest> agreementRequests = request.getAgreements();
List<Long> termIds = agreementRequests.stream()
.map(TermAgreementRequest::getTermId)
.collect(Collectors.toList());
// 1. 약관 유효성 검증
Map<Long, Term> termsMap = termRepository.findAllById(termIds).stream()
.collect(Collectors.toMap(Term::getId, t -> t));
if (termsMap.size() != agreementRequests.size()) {
// 요청된 약관 ID 중 존재하지 않는 것이 있음
throw new NoSuchElementException("하나 이상의 약관을 찾을 수 없습니다.");
}
for (TermAgreementRequest agreementRequest : agreementRequests) {
Term term = termsMap.get(agreementRequest.getTermId());
if (term.getIsRequired() && !agreementRequest.getIsAgreed()) {
throw new IllegalArgumentException("필수 약관에 동의해야 합니다: " + term.getTitle());
}
}
// 2. 기존 동의 내역 일괄 삭제
memberTermsAgreementRepository.deleteAllByMemberId(memberId);
// 3. 새로운 동의 내역 일괄 저장
List<MemberTermsAgreement> newAgreements = agreementRequests.stream()
.map(agreementRequest -> MemberTermsAgreement.builder()
.memberId(memberId)
.termsId(agreementRequest.getTermId())
.isAgreed(agreementRequest.getIsAgreed())
.build())
.collect(Collectors.toList());
memberTermsAgreementRepository.saveAll(newAgreements);
}
작업 내용 ⚒️
리뷰어 참고 사항 🤔
DDL
-- 없음