Skip to content
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

Feat #77 영수증 수정 기능 추가 #78

Merged
merged 8 commits into from
Nov 26, 2024
Merged

Feat #77 영수증 수정 기능 추가 #78

merged 8 commits into from
Nov 26, 2024

Conversation

jj0526
Copy link
Collaborator

@jj0526 jj0526 commented Nov 25, 2024

PR 내용

  • 영수증 수정 기능을 추가하였습니다

PR 세부사항

  • 수정할 수 있는 값은 description, amount, date, files 입니다.

관련 스크린샷

image


주의사항

  • 파일 수정이 없을 시, 기존 파일이 유지
  • 파일 수정이 있다면 기존 파일을 전부 삭제하고 추가된 파일들만 반영
  • amount를 수정한 경우 기존의 amount를 더하고 새로운 amount의 값을 빼는 형식으로 진행

체크 리스트

  • 리뷰어 설정
  • Assignee 설정
  • Label 설정
  • 제목 양식 맞췄나요? (ex. #0 Feat: 기능 추가)
  • 변경 사항에 대한 테스트

@jj0526 jj0526 added the feature label Nov 25, 2024
@jj0526 jj0526 self-assigned this Nov 25, 2024
@jj0526 jj0526 linked an issue Nov 25, 2024 that may be closed by this pull request
Copy link
Member

@huncozyboy huncozyboy left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다 !


public interface ReceiptUseCase {
void save(ReceiptDTO.Save dto);

void update(Long reciptId, ReceiptDTO.Update dto);
Copy link
Member

Choose a reason for hiding this comment

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

영수증 아이디 변수명이 reciptId로 되어있는데 의도하신걸까요 아니면 오타일까요 ??
만약 오타가 맞다면 receiptId로 수정해주시는게 좋아보입니당

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

수정했습니다!

Comment on lines 41 to 49
public void update(ReceiptDTO.Update dto){
this.updateUpperClass(dto);
}

public void updateUpperClass(ReceiptDTO.Update dto) {
this.description = dto.description();
this.amount = dto.amount();
this.date = dto.date();
}
Copy link
Member

@huncozyboy huncozyboy Nov 25, 2024

Choose a reason for hiding this comment

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

데이터 전달을 ReceiptDTO.Update dto로 객체로 사용해서 분리해준 점이 좋습니다

Copy link
Member

Choose a reason for hiding this comment

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

updateUpperClass를 따로 분리해서 작성하신 이유가 있을까요?
Receipt는 별도의 상속 관계를 지니지 않아서 굳이 분리할 필요는 없어 보여요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

알겠습니다!

Copy link
Member

@hyxklee hyxklee left a comment

Choose a reason for hiding this comment

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

구현하시느라 고생하셨어요! 로직에는 문제가 없어 보여요
@convert(converter = FileListConverter.class)
private List images;
이 부분은 더 이상 사용을 하지 않기 때문에 제거 부탁드릴게요!
그리고 PR 명 컨벤션에 맞게 수정 부탁드려요!

@NotNull Integer amount,
@NotNull LocalDate date,
@NotNull Integer cardinal,
@Valid List<FileSaveRequest> files
Copy link
Member

Choose a reason for hiding this comment

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

의도하지 않은 null을 막기 위해 @Valid List<@NotNull FileSaveRequest> files로 작성하는 것은 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

알겠습니다! 수정하겠습니다

Comment on lines 41 to 49
public void update(ReceiptDTO.Update dto){
this.updateUpperClass(dto);
}

public void updateUpperClass(ReceiptDTO.Update dto) {
this.description = dto.description();
this.amount = dto.amount();
this.date = dto.date();
}
Copy link
Member

Choose a reason for hiding this comment

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

updateUpperClass를 따로 분리해서 작성하신 이유가 있을까요?
Receipt는 별도의 상속 관계를 지니지 않아서 굳이 분리할 필요는 없어 보여요!

@jj0526 jj0526 changed the title #77 Feat: 영수증 수정 기능 추가 Feat #77 영수증 수정 기능 추가 Nov 26, 2024
@jj0526 jj0526 merged commit dd1a7d9 into develop Nov 26, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feat #77 영수증 수정하는 api
3 participants