Skip to content

Comments

[새리][3-4-1] 그룹 조회#231

Open
sallyjellyy wants to merge 15 commits intomainfrom
3-4-1/read-all-groups
Open

[새리][3-4-1] 그룹 조회#231
sallyjellyy wants to merge 15 commits intomainfrom
3-4-1/read-all-groups

Conversation

@sallyjellyy
Copy link
Member

@sallyjellyy sallyjellyy commented Dec 5, 2021

Related issue

우선 예정보다 많이 늦어진 점 죄송합니다..ㅠㅠ:bow:

그룹과 유저를 @ManyToMany 어노테이션을 이용해 관계를 맺도록 했습니다.
그룹서비스에서는 유저를 찾을 때마다 유저레파지토리를 사용하지 않고 유저 서비스에 findById를 구현해놓고 이를 사용하도록 했습니다.

그룹 인수테스트에서 생성과 수정부분에 group.members를 인식하지 못해 우선 ignoringFields에 넣어두었습니다.

Copy link
Collaborator

@Dae-Hwa Dae-Hwa left a comment

Choose a reason for hiding this comment

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

고생하셨습니다
의견 조금 달아놨어요~ㅎㅎ

}

public static GroupReadAllResponse from(Group group) {
return new GroupReadAllResponse(group.getId(), group.getName(), group.getDescription());
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기서 빌더는 안 써지나요?

}

@GetMapping
public GroupReadAllResponses readAll(@LoggedInUser User user) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

일급 컬렉션 쓰셨네요!👍

@Mapping(target = "owner", source = "owner")
Group map(GroupCreationRequest groupCreationRequest, User owner);

GroupReadAllResponse map(Group group);
Copy link
Collaborator

Choose a reason for hiding this comment

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

별도의 매퍼를 정의해야 할 것 같아요
파라미터가 Group으로 고정되면 오버로딩이 힘들 것 같습니다

Comment on lines 55 to 61
return GroupReadAllResponses
.builder()
.groupReadAllResponse(
loggedInUser.getJoinedGroups().stream()
.map(GroupMapper.INSTANCE::map)
.collect(Collectors.toList()))
.build();
Copy link
Collaborator

Choose a reason for hiding this comment

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

이런 부분도 매퍼로 처리할 수 있는 것 같았는데, 살펴보고 한 번 정리해볼게요 ㅎㅎ

Comment on lines +440 to +441
Group group = Group.builder().name("name").description("").owner(testUser).schedules(new ArrayList<>()).build();
Group group2 = Group.builder().name("group2").description("group2").owner(testUser).schedules(new ArrayList<>()).build();
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기는 Provider에 넣지 않으신 이유가 있나요?

Comment on lines +77 to +79
if (!user.getJoinedGroups().contains(this)) {
user.getJoinedGroups().add(this);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

멤버를 추가하는게 아닌 것 같은데 맞나요?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Dae-Hwa 멤버를 User 타입으로 정의해 놓으셔서 그런 것 같아요!

janeljs
janeljs previously approved these changes Dec 7, 2021
Copy link
Contributor

@janeljs janeljs left a comment

Choose a reason for hiding this comment

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

새리 고생 많으셨습니다아~~!

@Data
public class GroupReadAllResponses {

private List<GroupReadAllResponse> groupReadAllResponse;
Copy link
Contributor

Choose a reason for hiding this comment

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

groupReadAllResponses가 되어야 할 것 같아요!

Comment on lines +77 to +79
if (!user.getJoinedGroups().contains(this)) {
user.getJoinedGroups().add(this);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@Dae-Hwa 멤버를 User 타입으로 정의해 놓으셔서 그런 것 같아요!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants