Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.specialwarriors.conal.contribution.service.ContributionService;
import com.specialwarriors.conal.contributor.domain.Contributor;
import com.specialwarriors.conal.github_repo.domain.GithubRepo;
import com.specialwarriors.conal.github_repo.repository.GithubRepoRepository;
import com.specialwarriors.conal.notification.domain.NotificationAgreement;
import com.specialwarriors.conal.notification.enums.NotificationType;
import com.specialwarriors.conal.notification.service.NotificationAgreementQuery;
Expand All @@ -17,6 +18,7 @@
public class ContributionScheduler {

private final NotificationAgreementQuery notificationAgreementQuery;
private final GithubRepoRepository githubRepoRepository;
private final ContributionService contributionService;

private final MailUtil mailUtil;
Expand All @@ -26,10 +28,12 @@ public void sendContribution() {
List<NotificationAgreement> notificationAgreements = notificationAgreementQuery
.findAllByType(NotificationType.CONTRIBUTION);

List<GithubRepo> githubRepos = notificationAgreements.stream()
.map(NotificationAgreement::getGithubRepo)
List<Long> githubRepoId = notificationAgreements.stream()
.map(NotificationAgreement::getGithubRepoId)
.toList();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
List<Long> githubRepoId = notificationAgreements.stream()
.map(NotificationAgreement::getGithubRepoId)
.toList();
List<Long> githubRepoIds = notificationAgreements.stream()
.map(NotificationAgreement::getGithubRepoId)
.toList();

리스트 타입의 변수가 단수형으로 지어질 경우 의미가 불명확해진다고 생각합니다. 수정 부탁드립니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

리스트 타입의 변수가 단수형으로 지어질 경우 의미가 불명확해진다고 생각합니다. 수정 부탁드립니다.

넵!


List<GithubRepo> githubRepos = githubRepoRepository.findAllById(githubRepoId);

for (GithubRepo githubRepo : githubRepos) {
for (Contributor contributor : githubRepo.getContributors()) {
mailUtil.sendContributionForm(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,6 @@ public class GithubRepo {
@OneToMany(mappedBy = "githubRepo", cascade = CascadeType.ALL)
private List<Contributor> contributors = new ArrayList<>();

@OneToMany(mappedBy = "githubRepo")
private List<NotificationAgreement> notificationAgreements = new ArrayList<>();

@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "user_id")
private User user;
Expand All @@ -67,8 +64,7 @@ public void addContributors(List<Contributor> contributors) {

public void setNotificationAgreement(List<NotificationAgreement> agreements) {
for (NotificationAgreement agreement : agreements) {
this.notificationAgreements.add(agreement);
agreement.setGitHubRepo(this);
agreement.setGitHubRepoId(this.getId());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

특별한 목적이 있는 것이 아니라면 Stream API를 활용해보는 건 어떨까요? 또한, 값을 변경하는 메서드를 단순히 setter로 작명할 경우 정확히 어떤 작업을 수행하는 지 의미가 모호할 수 있다고 생각합니다. updateGithubRepoId와 같은 메서드명은 어떨까요?

Copy link
Member Author

@songhyeongyu songhyeongyu May 15, 2025

Choose a reason for hiding this comment

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

특별한 목적이 있는 것이 아니라면 Stream API를 활용해보는 건 어떨까요? 또한, 값을 변경하는 메서드를 단순히 setter로 작명할 경우 정확히 어떤 작업을 수행하는 지 의미가 모호할 수 있다고 생각합니다. updateGithubRepoId와 같은 메서드명은 어떨까요?

넵 그런식으로 수정하겠습니다.

}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public interface GithubRepoMapper {

default GithubRepoCreateResponse toGithubRepoCreateResponse(String owner,
String reponame) {

return new GithubRepoCreateResponse(
owner,
reponame
Comment on lines 18 to 23
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 클래스의 매핑 메서드들에 default 가시성을 설정한 이유에 대해 설명 부탁드립니다.

Copy link
Member Author

@songhyeongyu songhyeongyu May 15, 2025

Choose a reason for hiding this comment

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

해당 클래스의 매핑 메서드들에 default 가시성을 설정한 이유에 대해 설명 부탁드립니다.

owner와 reponame 두 개의 파라미터를 받아 Response를 생성하는 방식은 MapStruct가 처리할 수 없기 때문에 default를 사용했습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

좀 더 상세히 설명주시겠어요? Mapstruct가 default 가시성을 설정해주어야 처리를 위해 접근하다는 의미로 이해하는 것이 맞는 지 부연 설명 부탁드립니다.

Expand All @@ -26,12 +27,12 @@ default GithubRepoCreateResponse toGithubRepoCreateResponse(String owner,

default GithubRepoGetResponse toGithubRepoGetResponse(GithubRepo repo, String owner,
String reponame, Long userId) {

return new GithubRepoGetResponse(
userId,
repo.getId(),
repo.getName(),
repo.getUrl(),
repo.getNotificationAgreements(),
repo.getEndDate(),
owner,
reponame
Expand All @@ -42,6 +43,7 @@ default GithubRepoGetResponse toGithubRepoGetResponse(GithubRepo repo, String ow
GithubRepoSummary toGithubRepoSummary(GithubRepo repo);

default GithubRepoPageResponse toGithubRepoPageResponse(Page<GithubRepo> page, Long userId) {

return new GithubRepoPageResponse(
page.getContent().stream()
.map(this::toGithubRepoSummary)
Expand All @@ -54,6 +56,7 @@ default GithubRepoPageResponse toGithubRepoPageResponse(Page<GithubRepo> page, L
}

default GithubRepoDeleteResponse toGithubDeleteRepoResponse() {

return new GithubRepoDeleteResponse(
true
);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
package com.specialwarriors.conal.github_repo.dto.response;

import com.specialwarriors.conal.notification.domain.NotificationAgreement;
import java.time.LocalDate;
import java.util.List;

public record GithubRepoGetResponse(
Long userId,
Long repoId,
String name,
String url,
List<NotificationAgreement> agreement,
LocalDate endDate,
String owner,
String repo
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ private List<Contributor> createAndSaveContributors(Set<String> emails) {
}

private List<NotificationAgreement> createAndAttachNotifications() {

return notificationAgreementRepository.saveAll(
List.of(
new NotificationAgreement(NotificationType.VOTE),
Expand Down Expand Up @@ -109,7 +110,7 @@ public GithubRepoPageResponse getGithubRepoInfos(Long userId, int page) {
public GithubRepoDeleteResponse deleteRepo(Long userId, Long repositoryId) {
GithubRepo repo = githubRepoQuery.findByUserIdAndRepositoryId(userId, repositoryId);
contributorRepository.deleteAllByGithubRepo(repo);
notificationAgreementRepository.deleteByGithubRepo(repo);
notificationAgreementRepository.deleteByGithubRepoId(repo.getId());
githubRepoRepository.delete(repo);

return githubRepoMapper.toGithubDeleteRepoResponse();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,13 @@
package com.specialwarriors.conal.notification.domain;

import com.specialwarriors.conal.github_repo.domain.GithubRepo;
import com.specialwarriors.conal.notification.converter.NotificationTypeConverter;
import com.specialwarriors.conal.notification.enums.NotificationType;
import jakarta.persistence.Column;
import jakarta.persistence.Convert;
import jakarta.persistence.Entity;
import jakarta.persistence.FetchType;
import jakarta.persistence.GeneratedValue;
import jakarta.persistence.GenerationType;
import jakarta.persistence.Id;
import jakarta.persistence.JoinColumn;
import jakarta.persistence.ManyToOne;
import jakarta.persistence.Table;
import lombok.AccessLevel;
import lombok.Getter;
Expand All @@ -28,9 +25,8 @@ public class NotificationAgreement {

private boolean isAgree;

@ManyToOne(fetch = FetchType.EAGER)
@JoinColumn(name = "github_repo_id")
private GithubRepo githubRepo;
@Column(name = "github_repo_id", nullable = false)
private Long githubRepoId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
@Column(name = "github_repo_id", nullable = false)
private Long githubRepoId;
@Column(name = "github_repo_id")
private long githubRepoId;

기본 타입으로 설정할 경우 굳이 래퍼 타입을 쓰고 nullable=false를 설정할 필요가 없습니다. 굳이 래퍼 타입을 쓰신 이유가 있다면 설명 부탁드립니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

기본 타입으로 설정할 경우 굳이 래퍼 타입을 쓰고 nullable=false를 설정할 필요가 없습니다. 굳이 래퍼 타입을 쓰신 이유가 있다면 설명 부탁드립니다.

몰랐습니다! 수정하겠습니다


@Convert(converter = NotificationTypeConverter.class)
private NotificationType notificationType;
Expand All @@ -43,8 +39,8 @@ public void disagree() {
this.isAgree = false;
}

public void setGitHubRepo(GithubRepo githubRepo) {
this.githubRepo = githubRepo;
public void setGitHubRepoId(Long githubRepoId) {
this.githubRepoId = githubRepoId;
}

public NotificationAgreement(NotificationType notificationType) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
package com.specialwarriors.conal.notification.repository;

import com.specialwarriors.conal.github_repo.domain.GithubRepo;
import com.specialwarriors.conal.notification.domain.NotificationAgreement;
import com.specialwarriors.conal.notification.enums.NotificationType;
import java.util.List;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.stereotype.Repository;

@Repository
public interface NotificationAgreementRepository extends JpaRepository<NotificationAgreement, Long> {
public interface NotificationAgreementRepository extends
JpaRepository<NotificationAgreement, Long> {

List<NotificationAgreement> findAllByGithubRepoAndNotificationType(GithubRepo githubRepo,
NotificationType notificationType);
List<NotificationAgreement> findAllByGithubRepoIdAndNotificationType(Long githubRepoId,
NotificationType notificationType);

List<NotificationAgreement> findAllByNotificationType(NotificationType notificationType);

void deleteByGithubRepo(GithubRepo repo);
void deleteByGithubRepoId(Long githubRepoId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

githubRepoId가 not null이라면 파라미터를 기본 타입으로 다뤄도 괜찮지 않을까요? 타입의 적절한 사용에 대해 고민해보시면 좋을 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

githubRepoId가 not null이라면 파라미터를 기본 타입으로 다뤄도 괜찮지 않을까요? 타입의 적절한 사용에 대해 고민해보시면 좋을 것 같습니다.

넵! 수정하겠습다

}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public NotificationAgreement findByGithubRepoAndType(GithubRepo githubRepo,
NotificationType type) {

return notificationAgreementRepository
.findAllByGithubRepoAndNotificationType(githubRepo, type)
.findAllByGithubRepoIdAndNotificationType(githubRepo.getId(), type)
.stream()
.findFirst()
.orElseThrow(() -> new GeneralException(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package com.specialwarriors.conal.vote.scheduler;

import com.specialwarriors.conal.github_repo.domain.GithubRepo;
import com.specialwarriors.conal.notification.domain.NotificationAgreement;
import com.specialwarriors.conal.notification.enums.NotificationType;
import com.specialwarriors.conal.notification.service.NotificationAgreementQuery;
Expand All @@ -23,7 +22,7 @@ public class VoteScheduler {
@Scheduled(cron = "0 0 9 ? * FRI")
public void openWeeklyVote() {
List<NotificationAgreement> notificationAgreements = notificationAgreementQuery
.findAllByType(NotificationType.VOTE);
.findAllByType(NotificationType.VOTE);

List<Long> repositoryIds = extractGithubRepoIdsFrom(notificationAgreements);

Expand All @@ -33,7 +32,7 @@ public void openWeeklyVote() {
@Scheduled(cron = "0 0 18 ? * FRI")
public void sendWeeklyVoteForm() {
List<NotificationAgreement> notificationAgreements = notificationAgreementQuery
.findAllByType(NotificationType.VOTE);
.findAllByType(NotificationType.VOTE);

List<Long> repositoryIds = extractGithubRepoIdsFrom(notificationAgreements);

Expand All @@ -45,12 +44,12 @@ public void sendWeeklyVoteForm() {
@Scheduled(cron = "0 0 9 ? * MON")
public void sendWeeklyVoteResult() {
List<NotificationAgreement> notificationAgreements = notificationAgreementQuery
.findAllByType(NotificationType.VOTE);
.findAllByType(NotificationType.VOTE);

List<Long> repositoryIds = extractGithubRepoIdsFrom(notificationAgreements);
List<VoteResultResponse> voteResults = repositoryIds.stream()
.map(voteService::getVoteResult)
.toList();
.map(voteService::getVoteResult)
.toList();

for (VoteResultResponse voteResult : voteResults) {
List<String> emails = voteResult.emails();
Expand All @@ -60,11 +59,11 @@ public void sendWeeklyVoteResult() {


private List<Long> extractGithubRepoIdsFrom(
List<NotificationAgreement> notificationAgreements) {
List<NotificationAgreement> notificationAgreements) {

return notificationAgreements.stream()
.map(NotificationAgreement::getGithubRepo)
.map(GithubRepo::getId)
.toList();
.map(NotificationAgreement::getGithubRepoId)
.distinct()
.toList();
}
}