-
Notifications
You must be signed in to change notification settings - Fork 0
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
골 참여자 방출 기능 구현 #99
골 참여자 방출 기능 구현 #99
Changes from 11 commits
d6df41a
7440c2d
ce65227
8097b98
c69a1d4
4ee2bd6
04042d2
0d8fe1d
3100d79
5b4f9e0
c3b55c5
157465a
d070051
a3b8651
4996979
c8d2760
1683ec2
4ca692c
07dc4bc
c6517ad
62ea675
42d3d58
41c9d66
80fbb73
e3ff334
13c9f2e
560e6ca
80a9449
f0daa20
43a95f3
a8a8d1e
4d35b0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
package com.backend.blooming.goal.application.exception; | ||
|
||
import com.backend.blooming.exception.BloomingException; | ||
import com.backend.blooming.exception.ExceptionMessage; | ||
|
||
public class ForbiddenGoalToDeleteException extends BloomingException { | ||
|
||
private ForbiddenGoalToDeleteException(final ExceptionMessage exceptionMessage) { | ||
super(exceptionMessage); | ||
} | ||
|
||
public static class ForbiddenUserToDeleteGoal extends ForbiddenGoalToDeleteException { | ||
|
||
public ForbiddenUserToDeleteGoal() { | ||
super(ExceptionMessage.DELETE_GOAL_FORBIDDEN); | ||
} | ||
} | ||
|
||
public static class ForbiddenUserToDeleteUser extends ForbiddenGoalToDeleteException { | ||
|
||
public ForbiddenUserToDeleteUser() { | ||
super(ExceptionMessage.FORBIDDEN_USER_TO_DELETE_USER); | ||
} | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package com.backend.blooming.goal.application.exception; | ||
|
||
import com.backend.blooming.exception.BloomingException; | ||
import com.backend.blooming.exception.ExceptionMessage; | ||
|
||
public class ForbiddenGoalToUpdateException extends BloomingException { | ||
|
||
private ForbiddenGoalToUpdateException(final ExceptionMessage exceptionMessage) { | ||
super(exceptionMessage); | ||
} | ||
|
||
public static class ForbiddenUserToUpdateGoalToUpdate extends ForbiddenGoalToUpdateException { | ||
|
||
public ForbiddenUserToUpdateGoalToUpdate() { | ||
super(ExceptionMessage.UPDATE_GOAL_FORBIDDEN); | ||
} | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
package com.backend.blooming.goal.domain; | ||
|
||
import com.backend.blooming.common.entity.BaseTimeEntity; | ||
import com.backend.blooming.goal.application.exception.DeleteGoalForbiddenException; | ||
import com.backend.blooming.goal.application.exception.ForbiddenGoalToDeleteException; | ||
import com.backend.blooming.goal.application.exception.InvalidGoalAcceptException; | ||
import com.backend.blooming.goal.application.exception.InvalidGoalException; | ||
import com.backend.blooming.user.domain.User; | ||
|
@@ -107,7 +107,7 @@ public void updateDeleted(final Long userId) { | |
|
||
private void validUserToDelete(final Long userId) { | ||
if (!isManager(userId)) { | ||
throw new DeleteGoalForbiddenException(); | ||
throw new ForbiddenGoalToDeleteException.ForbiddenUserToDeleteGoal(); | ||
} | ||
} | ||
public boolean isTeam(final User user) { | ||
|
@@ -139,4 +139,22 @@ private void validateUserToAccept(final User user) { | |
public boolean isTeamAndAccepted(final User user) { | ||
return teams.isTeamAndAccepted(user); | ||
} | ||
|
||
public void deleteUser(final User user, final Long managerId) { | ||
validUserToDelete(user); | ||
validUserToDeleteUser(managerId); | ||
teams.deleteUser(user.getId()); | ||
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. 굳이 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. 여기까진 미처 생각을 못한 것 같습니다..! user 전체 넘겨주는 것으로 수정했습니다! |
||
} | ||
|
||
private void validUserToDelete(final User user) { | ||
if (!this.isTeam(user)) { | ||
throw new InvalidGoalException.InvalidInvalidUserToDelete(); | ||
} | ||
} | ||
|
||
private void validUserToDeleteUser(final Long userId) { | ||
if (!isManager(userId)) { | ||
throw new ForbiddenGoalToDeleteException.ForbiddenUserToDeleteUser(); | ||
} | ||
} | ||
Comment on lines
+156
to
+160
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. 해당 부분은 생각하지 못했는데 좋네요! 👍 |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,4 +57,12 @@ private void processDefaultManager(final User user, final Goal goal) { | |
public void updateAccepted() { | ||
this.accepted = true; | ||
} | ||
|
||
public void updateDeleted() { | ||
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.deleted = true; | ||
} | ||
|
||
public boolean isDeletedFalse() { | ||
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. 해당 메서드를 만들어 사용하는 이유는 무엇일까요? 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.
위 같은 로직에서 filter를 걸 때 !isDeleted()를 사용하게 되면 goalTeam -> !goalTeam.isDeleted()로 사용하게 됩니다. 이것이 반복되니 아예 따로 deleted가 false인지를 true로 반환하는 메서드를 만들어서 간결하게 표현하는 것이 낫지 않나 생각해 따로 메서드를 만들었습니다. 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. 저는 오히려 동일한 로직이 추가되어 코드의 중복이 발생하는 것이 아닌가 하는 의문이 있습니다. 또한, 해당 메서드를 사용할 것이라면 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. 고민해봤는데 isDeletedFalse 자체가 굳이 필요하지 않은 메서드를 만든 것 같아 삭제하고 isDeleted의 True/False 여부 확인으로 수정했습니다! |
||
return !this.deleted; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -37,6 +37,7 @@ public static Teams create(final List<User> users, final Goal goal) { | |||||||||||||||||||||||
validateUsersSize(users); | ||||||||||||||||||||||||
final List<GoalTeam> goalTeams = users.stream() | ||||||||||||||||||||||||
.map(user -> new GoalTeam(user, goal)) | ||||||||||||||||||||||||
.filter(GoalTeam::isDeletedFalse) | ||||||||||||||||||||||||
.toList(); | ||||||||||||||||||||||||
return new Teams(goalTeams); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
@@ -50,6 +51,7 @@ private static void validateUsersSize(final List<User> users) { | |||||||||||||||||||||||
public List<GoalTeam> update(final List<User> users, final Goal goal) { | ||||||||||||||||||||||||
validateUsersSize(users); | ||||||||||||||||||||||||
final List<User> usersBeforeUpdate = this.goalTeams.stream() | ||||||||||||||||||||||||
.filter(GoalTeam::isDeletedFalse) | ||||||||||||||||||||||||
.map(GoalTeam::getUser) | ||||||||||||||||||||||||
.toList(); | ||||||||||||||||||||||||
final List<GoalTeam> updatedUsers = users.stream() | ||||||||||||||||||||||||
|
@@ -78,4 +80,20 @@ public boolean isTeamAndAccepted(final User user) { | |||||||||||||||||||||||
return goalTeams.stream() | ||||||||||||||||||||||||
.anyMatch(goalTeam -> goalTeam.getUser().equals(user) && goalTeam.isAccepted()); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
public void deleteUser(final Long userId) { | ||||||||||||||||||||||||
this.goalTeams.forEach(goalTeam -> { | ||||||||||||||||||||||||
if (goalTeam.getUser().getId().equals(userId)) { | ||||||||||||||||||||||||
goalTeam.updateDeleted(); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
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. 개인적으로 아래와 같은 for문을 통해 break를 추가하면 어떨까 하는 생각이 있습니다.
Suggested change
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. 위에서 코멘트 남긴대로 for문 돌다가 if문에서 걸리면 goalTeam delete 해주고 Teams에서는 remove해주는 식으로 구현했습니다. 감사합니다! |
||||||||||||||||||||||||
final List<GoalTeam> updatedGoalTeams = this.goalTeams.stream() | ||||||||||||||||||||||||
.filter(GoalTeam::isDeletedFalse) | ||||||||||||||||||||||||
.toList(); | ||||||||||||||||||||||||
updateGoalTeams(updatedGoalTeams); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
private void updateGoalTeams(final List<GoalTeam> goalTeams) { | ||||||||||||||||||||||||
this.goalTeams = goalTeams; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
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. 이렇게 하더라도 goalTeam 테이블에서 삭제된 사용자가 제거되거나 goalId를 null로 만드는 등의 행위는 하지 않을 것으로 보이는데, 삭제 후 다시 조회했을 때 삭제된 사용자는 불러와지지 않나요? 어떤 의미가 있는 것인지 궁금합니다. 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. 이렇게 하면 삭제된 사용자는 불러와지지 않습니다. updateGoalTeams를 하지 않을 경우, 삭제된 사용자가 그대로 goalTeams에 남아있기 때문에 삭제를 하지 않은 사용자들만 다시 goalTeams에 할당했습니다. 그런데 너무 어렵게 생각한 것 같네요.. 정수님이 밑에서 피드백 주신대로 for문 돌면서 if문 걸리면 delete하고 goalTeams에서도 remove 하는 방식으로 수정했습니다! softDelete vs hardDelete 관련해서는 골 팀은 hardDelete를 해도 문제가 없을 것 같아요. 정말 단지 골 참여자 목록에 추가하기 위해 만들어진 테이블이기 때문에 소속 되어있던 골에서 방출되면 더 이상 테이블의 존재 의미가 없어지긴 합니다. 추후 분석용 데이터로서는 어느정도 의미가 있을까요? 딱히 생각나는 활용방안이 없어서 저는 hardDelete를 해도 좋을 것 같습니다. 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. hardDelete를 해도 괜찮을 것 같습니다. 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. 그런데 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. hardDelete로 구현하기로 결정된 게 아니라고 판단해 로직을 추가하지 않았습니다. 로직 추가해서 리뷰 재요청하도록 하겠습니다. |
||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,4 +116,17 @@ public ResponseEntity<Void> acceptGoal( | |
return ResponseEntity.noContent() | ||
.build(); | ||
} | ||
|
||
// TO DO: api 형식에서 /user는 불필요할까요? | ||
@DeleteMapping(value = "/{goalId}/user/{userId}", headers = "X-API-VERSION=1") | ||
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. 저는 필요하다고 생각합니다! 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. |
||
public ResponseEntity<Void> deleteUser( | ||
@PathVariable("goalId") final Long goalId, | ||
@PathVariable("userId") final Long userId, | ||
@Authenticated AuthenticatedUser authenticatedUser | ||
) { | ||
goalService.deleteUser(userId, goalId, authenticatedUser.userId()); | ||
|
||
return ResponseEntity.noContent() | ||
.build(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,4 +112,13 @@ private void validateUserToRead(final Goal goal, final User user) { | |
throw new ForbiddenStampToReadException(); | ||
} | ||
} | ||
|
||
/* | ||
TO DO: 골 아이디와 사용자 아이디를 받아서 모든 스탬프를 삭제하는 로직을 스탬프 서비스에 구현했는데, | ||
골 서비스에서 한꺼번에 구현하는 것이 나을까요? | ||
*/ | ||
public void deleteAllByGoalIdAndUserId(final Long goalId, final Long userId) { | ||
final List<Stamp> stamps = stampRepository.findAllByGoalIdAndUserId(goalId, userId); | ||
stamps.forEach(Stamp::delete); | ||
} | ||
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. 해당 부분은 좀 고민이 되네요.... 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. 좋습니다! 그러면 지금은 일단 이렇게 두고, 의존성 관련 리팩터링에 대한 논의 후에 정해진 대로 리팩터링 하는 것으로 해요! |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,4 +43,11 @@ SELECT EXISTS( | |
WHERE s.id = :stampId | ||
""") | ||
Optional<Stamp> findByIdAndFetchGoalAndUser(final Long stampId); | ||
|
||
@Query(""" | ||
SELECT s | ||
FROM Stamp s | ||
WHERE s.goal.id = :goalId AND s.user.id = :userId | ||
""") | ||
List<Stamp> findAllByGoalIdAndUserId(final Long goalId, final Long userId); | ||
Comment on lines
+47
to
+52
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. jpa 네이밍만으로 안 되기에 쿼리문을 사용한 것이 맞을까요? 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. 네 맞습니다! 네이밍만으로 조회가 불가하다고 판단해 쿼리문 사용했습니다! |
||
} |
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.
managerId에 들어오는 값은 항상 매니저가 아니라고 생각합니다.
그래서 해당 변수를 userId로 하고, userId deleteUserId라고 하는 건 어떨까요?
이건 그냥 제안사항이기에 효선님께서 판단해 주시고 더 적절하다고 생각되는 것으로 수정해 주시면 됩니다!
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.
좋습니다! managerId를 userId로 바꾸고 현 userId를 deleteUserId로 바꾸는 게 좋은 것 같네요!