Skip to content

refactor: semester 기반 시간표/위시리스트 조작 정합성 강화 #17

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.

Sign up for GitHub

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

Open
coldmans wants to merge 2 commits into
base: main
Choose a base branch
from
Open

refactor: semester 기반 시간표/위시리스트 조작 정합성 강화 #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2770c2
refactor: semester 기준으로 시간표/위시리스트 조작 정합성 개선
Feb 14, 2026
83156c8
fix: PR17 리뷰 반영(semester 필수화 및 오삭제 방지)
Feb 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/main/java/inu/timetable/controller/TimetableController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ public ResponseEntity<?> addSubjectToTimetable(@RequestBody Map<String, Object>
}

@DeleteMapping("/remove")
public ResponseEntity<?> removeSubjectFromTimetable(@RequestParam Long userId, @RequestParam Long subjectId) {
public ResponseEntity<?> removeSubjectFromTimetable(@RequestParam Long userId, @RequestParam Long subjectId, @RequestParam String semester) {
try {
timetableService.removeSubjectFromTimetable(userId, subjectId);
timetableService.removeSubjectFromTimetable(userId, subjectId, semester);
return ResponseEntity.ok(Map.of("message", "과목이 시간표에서 제거되었습니다."));

} catch (RuntimeException e) {
Expand All @@ -46,7 +46,7 @@ public ResponseEntity<?> removeSubjectFromTimetable(@RequestParam Long userId, @
@GetMapping("/user/{userId}")
public ResponseEntity<List<UserTimetable>> getUserTimetable(
@PathVariable Long userId,
@RequestParam(required = false) String semester) {
@RequestParam String semester) {

List<UserTimetable> timetable = timetableService.getUserTimetable(userId, semester);
return ResponseEntity.ok(timetable);
Expand All @@ -57,9 +57,10 @@ public ResponseEntity<?> updateMemo(@RequestBody Map<String, Object> request) {
try {
Long userId = Long.valueOf(request.get("userId").toString());
Long subjectId = Long.valueOf(request.get("subjectId").toString());
String semester = (String) request.get("semester");
String memo = (String) request.get("memo");

UserTimetable userTimetable = timetableService.updateMemo(userId, subjectId, memo);
UserTimetable userTimetable = timetableService.updateMemo(userId, subjectId, semester, memo);
return ResponseEntity.ok(userTimetable);
Comment on lines 57 to 64

Copilot AI Feb 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

semester is optional in this request body, but the service update is semester-specific. Without validating presence/non-blank semester, clients that omit it will get inconsistent behavior or failures. Recommend rejecting missing/blank semester with 400 (or implementing an explicit fallback semester selection).

Copilot uses AI. Check for mistakes.

} catch (RuntimeException e) {
Expand All @@ -68,7 +69,7 @@ public ResponseEntity<?> updateMemo(@RequestBody Map<String, Object> request) {
}

@DeleteMapping("/clear")
public ResponseEntity<?> clearTimetable(@RequestParam Long userId, @RequestParam(required = false) String semester) {
public ResponseEntity<?> clearTimetable(@RequestParam Long userId, @RequestParam String semester) {
try {
timetableService.removeAllSubjectsFromTimetable(userId, semester);
String message = semester != null && !semester.isEmpty()
Expand Down
10 changes: 6 additions & 4 deletions src/main/java/inu/timetable/controller/WishlistController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ public ResponseEntity<?> addToWishlist(@RequestBody Map<String, Object> request)
}

@DeleteMapping("/remove")
public ResponseEntity<?> removeFromWishlist(@RequestParam Long userId, @RequestParam Long subjectId) {
public ResponseEntity<?> removeFromWishlist(@RequestParam Long userId, @RequestParam Long subjectId, @RequestParam String semester) {
try {
wishlistService.removeFromWishlist(userId, subjectId);
wishlistService.removeFromWishlist(userId, subjectId, semester);
return ResponseEntity.ok(Map.of("message", "위시리스트에서 제거되었습니다."));

} catch (RuntimeException e) {
Expand Down Expand Up @@ -75,9 +75,10 @@ public ResponseEntity<?> updatePriority(@RequestBody Map<String, Object> request
try {
Long userId = Long.valueOf(request.get("userId").toString());
Long subjectId = Long.valueOf(request.get("subjectId").toString());
String semester = (String) request.get("semester");
Integer priority = Integer.valueOf(request.get("priority").toString());

wishlistService.updatePriority(userId, subjectId, priority);
wishlistService.updatePriority(userId, subjectId, semester, priority);
return ResponseEntity.ok(Map.of("message", "우선순위가 업데이트되었습니다."));
Comment on lines 75 to 82

Copilot AI Feb 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The request body semester is optional here (String semester = (String) request.get("semester")), but the service lookup is semester-specific. If semester is missing, updates will likely fail or target null-semester rows only. Recommend rejecting missing/blank semester with a 400 response (or documenting/implementing an explicit fallback) to avoid surprising behavior.

Copilot uses AI. Check for mistakes.

} catch (RuntimeException e) {
Expand All @@ -90,9 +91,10 @@ public ResponseEntity<?> updateRequired(@RequestBody Map<String, Object> request
try {
Long userId = Long.valueOf(request.get("userId").toString());
Long subjectId = Long.valueOf(request.get("subjectId").toString());
String semester = (String) request.get("semester");
Boolean isRequired = Boolean.valueOf(request.get("isRequired").toString());

wishlistService.updateRequired(userId, subjectId, isRequired);
wishlistService.updateRequired(userId, subjectId, semester, isRequired);
return ResponseEntity.ok(Map.of("message", "필수 과목 설정이 업데이트되었습니다."));
Comment on lines 91 to 98

Copilot AI Feb 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the priority endpoint: semester is read as optional from the request body, but the service now requires semester to uniquely identify the row. Consider validating semester is provided/non-blank and returning a 400 with a specific message when absent.

Copilot uses AI. Check for mistakes.

} catch (RuntimeException e) {
Expand Down
7 changes: 6 additions & 1 deletion src/main/java/inu/timetable/repository/UserTimetableRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import inu.timetable.entity.UserTimetable;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.Modifying;
import org.springframework.data.jpa.repository.Query;
import org.springframework.data.repository.query.Param;
import org.springframework.stereotype.Repository;
Expand All @@ -16,8 +17,12 @@ public interface UserTimetableRepository extends JpaRepository<UserTimetable, Lo

List<UserTimetable> findByUserId(Long userId);

Optional<UserTimetable> findByUserIdAndSubjectId(Long userId, Long subjectId);
Optional<UserTimetable> findByUserIdAndSubjectIdAndSemester(Long userId, Long subjectId, String semester);

@Modifying(clearAutomatically = true)
@Query("DELETE FROM UserTimetable ut WHERE ut.user.id = :userId AND ut.subject.id = :subjectId AND ut.semester = :semester")
int deleteByUserIdAndSubjectIdAndSemester(@Param("userId") Long userId, @Param("subjectId") Long subjectId, @Param("semester") String semester);

@Query("SELECT DISTINCT ut FROM UserTimetable ut JOIN FETCH ut.subject s WHERE ut.user.id = :userId AND ut.semester = :semester")
List<UserTimetable> findByUserIdAndSemesterWithSubjectAndSchedules(@Param("userId") Long userId, @Param("semester") String semester);
}
10 changes: 5 additions & 5 deletions src/main/java/inu/timetable/repository/WishlistRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ public interface WishlistRepository extends JpaRepository<WishlistItem, Long> {
"ORDER BY w.priority")
List<WishlistItem> findByUserIdAndSemesterWithSubjectAndSchedules(@Param("userId") Long userId, @Param("semester") String semester);

@Query("SELECT w FROM WishlistItem w JOIN FETCH w.subject s WHERE w.user.id = :userId AND s.id = :subjectId")
Optional<WishlistItem> findByUserIdAndSubjectId(@Param("userId") Long userId, @Param("subjectId") Long subjectId);
@Query("SELECT w FROM WishlistItem w JOIN FETCH w.subject s WHERE w.user.id = :userId AND s.id = :subjectId AND w.semester = :semester")

Copilot AI Feb 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findByUserIdAndSubjectIdAndSemester uses w.semester = :semester. If the caller passes semester=null (which can happen from the controllers), Hibernate will generate semester = NULL and the row will not be found even when w.semester is null. Either enforce non-null/blank semester at the API boundary, or change the query to handle null explicitly (e.g., match both-null via IS NULL).

Suggested change
@Query("SELECT w FROM WishlistItem w JOIN FETCH w.subject s WHERE w.user.id = :userId AND s.id = :subjectId AND w.semester = :semester")
@Query("SELECT w FROM WishlistItem w JOIN FETCH w.subject s " +
"WHERE w.user.id = :userId AND s.id = :subjectId " +
"AND ((:semester IS NULL AND w.semester IS NULL) OR w.semester = :semester)")

Copilot uses AI. Check for mistakes.
Optional<WishlistItem> findByUserIdAndSubjectIdAndSemester(@Param("userId") Long userId, @Param("subjectId") Long subjectId, @Param("semester") String semester);

@Modifying
@Query("DELETE FROM WishlistItem w WHERE w.user.id = :userId AND w.subject.id = :subjectId")
void deleteByUserIdAndSubjectId(@Param("userId") Long userId, @Param("subjectId") Long subjectId);
@Modifying(clearAutomatically = true)
@Query("DELETE FROM WishlistItem w WHERE w.user.id = :userId AND w.subject.id = :subjectId AND w.semester = :semester")
int deleteByUserIdAndSubjectIdAndSemester(@Param("userId") Long userId, @Param("subjectId") Long subjectId, @Param("semester") String semester);
}
32 changes: 22 additions & 10 deletions src/main/java/inu/timetable/service/TimetableService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,20 @@
import inu.timetable.repository.UserTimetableRepository;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

import java.util.List;
import java.util.Optional;

@Service
public class TimetableService {

private String requireSemester(String semester) {
if (semester == null || semester.isBlank()) {
throw new RuntimeException("학기(semester)는 필수입니다.");
}
return semester;
}

private final UserTimetableRepository userTimetableRepository;
private final UserRepository userRepository;
Expand All @@ -36,32 +44,35 @@ public UserTimetable addSubjectToTimetable(Long userId, Long subjectId, String s
.orElseThrow(() -> new RuntimeException("과목을 찾을 수 없습니다."));

// 이미 추가된 과목인지 확인
Optional<UserTimetable> existing = userTimetableRepository.findByUserIdAndSubjectId(userId, subjectId);
String sem = requireSemester(semester);
Optional<UserTimetable> existing = userTimetableRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, sem);
if (existing.isPresent()) {
throw new RuntimeException("이미 시간표에 추가된 과목입니다.");
}

// 시간표 겹침 확인
List<UserTimetable> currentTimetable = userTimetableRepository.findByUserIdAndSemesterWithSubjectAndSchedules(userId, semester);
List<UserTimetable> currentTimetable = userTimetableRepository.findByUserIdAndSemesterWithSubjectAndSchedules(userId, sem);
if (hasTimeConflict(currentTimetable, subject)) {
throw new RuntimeException("시간표가 겹치는 과목이 있습니다.");
}

UserTimetable userTimetable = UserTimetable.builder()
.user(user)
.subject(subject)
.semester(semester)
.semester(sem)
.memo(memo)
.build();

return userTimetableRepository.save(userTimetable);
}

public void removeSubjectFromTimetable(Long userId, Long subjectId) {
UserTimetable userTimetable = userTimetableRepository.findByUserIdAndSubjectId(userId, subjectId)
.orElseThrow(() -> new RuntimeException("시간표에서 해당 과목을 찾을 수 없습니다."));

userTimetableRepository.delete(userTimetable);
@Transactional
public void removeSubjectFromTimetable(Long userId, Long subjectId, String semester) {
String sem = requireSemester(semester);
int deleted = userTimetableRepository.deleteByUserIdAndSubjectIdAndSemester(userId, subjectId, sem);
if (deleted == 0) {
throw new RuntimeException("시간표에서 해당 과목을 찾을 수 없습니다.");
}
}
Comment on lines +69 to 76

Copilot AI Feb 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removeSubjectFromTimetable can be called with semester=null (controller marks it optional). With the current delete query, that will remove the subject from all semesters for the user, and deleted == 0 won’t catch multi-row deletes. Consider requiring semester here, or safely handling the null case by resolving to a single target row before deleting.

Copilot uses AI. Check for mistakes.

public List<UserTimetable> getUserTimetable(Long userId, String semester) {
Expand All @@ -72,8 +83,9 @@ public List<UserTimetable> getUserTimetable(Long userId, String semester) {
}
}

public UserTimetable updateMemo(Long userId, Long subjectId, String memo) {
UserTimetable userTimetable = userTimetableRepository.findByUserIdAndSubjectId(userId, subjectId)
public UserTimetable updateMemo(Long userId, Long subjectId, String semester, String memo) {
String sem = requireSemester(semester);
UserTimetable userTimetable = userTimetableRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, sem)
.orElseThrow(() -> new RuntimeException("시간표에서 해당 과목을 찾을 수 없습니다."));

userTimetable.setMemo(memo);
Expand Down
30 changes: 22 additions & 8 deletions src/main/java/inu/timetable/service/WishlistService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@

@Service
public class WishlistService {

private String requireSemester(String semester) {
if (semester == null || semester.isBlank()) {
throw new RuntimeException("학기(semester)는 필수입니다.");
}
return semester;
}

private final WishlistRepository wishlistRepository;
private final UserRepository userRepository;
Expand All @@ -41,15 +48,16 @@ public WishlistItem addToWishlist(Long userId, Long subjectId, String semester,
.orElseThrow(() -> new RuntimeException("과목을 찾을 수 없습니다."));

// 이미 위시리스트에 있는지 확인
Optional<WishlistItem> existing = wishlistRepository.findByUserIdAndSubjectId(userId, subjectId);
String sem = requireSemester(semester);
Optional<WishlistItem> existing = wishlistRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, sem);
if (existing.isPresent()) {
throw new RuntimeException("이미 위시리스트에 추가된 과목입니다.");
}

WishlistItem wishlistItem = WishlistItem.builder()
.user(user)
.subject(subject)
.semester(semester)
.semester(sem)
.priority(priority != null ? priority : 3) // 기본 우선순위: 중간
.isRequired(isRequired != null ? isRequired : false)
.build();
Expand All @@ -58,24 +66,30 @@ public WishlistItem addToWishlist(Long userId, Long subjectId, String semester,
}

@Transactional
public void removeFromWishlist(Long userId, Long subjectId) {
wishlistRepository.deleteByUserIdAndSubjectId(userId, subjectId);
public void removeFromWishlist(Long userId, Long subjectId, String semester) {
String sem = requireSemester(semester);
int deleted = wishlistRepository.deleteByUserIdAndSubjectIdAndSemester(userId, subjectId, sem);
if (deleted == 0) {
throw new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다.");
}
Comment on lines +69 to +74

Copilot AI Feb 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removeFromWishlist can be invoked with semester=null (controller marks it optional), and the repository delete query will then delete across all semesters. Also, the logic only checks deleted == 0, so accidental multi-row deletes won't be detected. Recommend requiring semester here, or handling the null case safely (e.g., resolve to exactly one target row before deleting, and error if ambiguous).

Suggested change
public void removeFromWishlist(Long userId, Long subjectId, String semester) {
int deleted = wishlistRepository.deleteByUserIdAndSubjectIdAndSemester(userId, subjectId, semester);
if (deleted == 0) {
throw new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다.");
}
public void removeFromWishlist(Long userId, Long subjectId, String semester) {
if (semester == null || semester.trim().isEmpty()) {
throw new IllegalArgumentException("학기를 지정해야 위시리스트에서 과목을 삭제할 수 있습니다.");
}
int deleted = wishlistRepository.deleteByUserIdAndSubjectIdAndSemester(userId, subjectId, semester);
if (deleted == 0) {
throw new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다.");
}
if (deleted > 1) {
throw new RuntimeException("위시리스트에서 여러 개의 항목이 삭제되었습니다. 관리자에게 문의하세요.");
}

Copilot uses AI. Check for mistakes.
}

public List<WishlistItem> getUserWishlist(Long userId, String semester) {
return wishlistRepository.findByUserIdAndSemesterWithSubjectAndSchedules(userId, semester);
}

public WishlistItem updatePriority(Long userId, Long subjectId, Integer priority) {
WishlistItem wishlistItem = wishlistRepository.findByUserIdAndSubjectId(userId, subjectId)
public WishlistItem updatePriority(Long userId, Long subjectId, String semester, Integer priority) {
String sem = requireSemester(semester);
WishlistItem wishlistItem = wishlistRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, sem)
.orElseThrow(() -> new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다."));

wishlistItem.setPriority(priority);
return wishlistRepository.save(wishlistItem);
Comment on lines +81 to 87

Copilot AI Feb 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updatePriority now depends on a semester argument, but the controller can pass null (body field not required). With the current repository query, null semester will likely fail to find existing rows (or target only rows with null semester), breaking updates for existing data. Consider validating semester is present/non-blank for this endpoint (or providing a deterministic default) before calling the repository.

Copilot uses AI. Check for mistakes.
}

public WishlistItem updateRequired(Long userId, Long subjectId, Boolean isRequired) {
WishlistItem wishlistItem = wishlistRepository.findByUserIdAndSubjectId(userId, subjectId)
public WishlistItem updateRequired(Long userId, Long subjectId, String semester, Boolean isRequired) {
String sem = requireSemester(semester);
WishlistItem wishlistItem = wishlistRepository.findByUserIdAndSubjectIdAndSemester(userId, subjectId, sem)
.orElseThrow(() -> new RuntimeException("위시리스트에서 해당 과목을 찾을 수 없습니다."));

wishlistItem.setIsRequired(isRequired);
Expand Down