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

[jinyoungchoi95-issue63] Article 단일조회 /수정 / 삭제 기능 구현 #64

Merged
merged 15 commits into from Oct 25, 2021

Conversation

jinyoungchoi95
Copy link
Member

Issue: #63

작업 내용

  • 게시글 단일 조회 기능 구현
  • 게시글 업데이트 기능 구현
  • 게시글 삭제 기능 구현

생성/변경 로직

  • Article내에 delete_at 추가기능 구현 (soft delete)
  • Article field update 메소드 구현

개인 코멘트

  • 로직짜는거 자체는 이전 user 도메인과 똑같아서 크게 어려운점 없이 기능 추가한 것 같네요.
  • 다만 authentication을 optional 하게 주는 api들이 몇가지 있을 것 같은데 차후에 정리해서 적용해야할 것 같아요 :)

@codecov-commenter
Copy link

Codecov Report

Merging #64 (7c4cb44) into jinyoungchoi95 (ade24bb) will increase coverage by 1.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                 Coverage Diff                  @@
##             jinyoungchoi95      #64      +/-   ##
====================================================
+ Coverage             90.57%   91.58%   +1.01%     
- Complexity              289      314      +25     
====================================================
  Files                    47       49       +2     
  Lines                   753      808      +55     
  Branches                 29       29              
====================================================
+ Hits                    682      740      +58     
+ Misses                   33       30       -3     
  Partials                 38       38              
Impacted Files Coverage Δ
.../java/com/study/realworld/article/domain/Slug.java 78.94% <ø> (ø)
...ealworld/article/controller/ArticleController.java 100.00% <100.00%> (ø)
...ticle/controller/request/ArticleUpdateRequest.java 100.00% <100.00%> (ø)
...va/com/study/realworld/article/domain/Article.java 79.31% <100.00%> (+7.88%) ⬆️
...study/realworld/article/domain/ArticleContent.java 82.50% <100.00%> (+3.08%) ⬆️
.../com/study/realworld/article/domain/SlugTitle.java 75.00% <100.00%> (+6.25%) ⬆️
...tudy/realworld/article/service/ArticleService.java 100.00% <100.00%> (ø)
...orld/article/service/model/ArticleUpdateModel.java 100.00% <100.00%> (ø)
...udy/realworld/global/config/WebSecurityConfig.java 100.00% <100.00%> (ø)
...om/study/realworld/global/exception/ErrorCode.java 100.00% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ade24bb...7c4cb44. Read the comment docs.

Copy link

@kwj1270 kwj1270 left a comment

Choose a reason for hiding this comment

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

LGTM!

Article article = articleService.findBySlug(Slug.of(slug));
return ResponseEntity.ok().body(ArticleResponse.fromArticle(article));
}

@PostMapping("/articles")
public ResponseEntity<ArticleResponse> createArticle(@RequestBody ArticleCreateRequest request,
@AuthenticationPrincipal Long loginId) {
Article article = articleService.createArticle(loginId, request.toArticleContent());
return ResponseEntity.ok().body(ArticleResponse.fromArticle(article));
Copy link

@kwj1270 kwj1270 Oct 25, 2021

Choose a reason for hiding this comment

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

API 명세가 어떻게 되어있는지 확인해야하는데
제가 요번에 안 지식으로 생성로직에서는 create() 하면서 URL 넘기더라고요(이건 참고)

Copy link
Member Author

Choose a reason for hiding this comment

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

생성하는 경우에는 201 반환하는 것도 좋은 것 같네요 :)

Comment on lines 44 to 45
public ResponseEntity<ArticleResponse> updateArticle(@PathVariable String slug,
@RequestBody ArticleUpdateRequest request, @AuthenticationPrincipal Long loginId) {
Copy link

Choose a reason for hiding this comment

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

인덴트를 동일하게 내리는게 어떨까요

Suggested change
public ResponseEntity<ArticleResponse> updateArticle(@PathVariable String slug,
@RequestBody ArticleUpdateRequest request, @AuthenticationPrincipal Long loginId) {
public ResponseEntity<ArticleResponse> updateArticle(
@PathVariable String slug,
@RequestBody ArticleUpdateRequest request,
@AuthenticationPrincipal Long loginId) {

이게 더 깔끔해보이더라고요!

Comment on lines +28 to +34
public ArticleUpdateModel toArticleUpdateModel() {
return new ArticleUpdateModel(
Optional.ofNullable(title).map(Title::of).orElse(null),
Optional.ofNullable(description).map(Description::of).orElse(null),
Optional.ofNullable(body).map(Body::of).orElse(null)
);
}
Copy link

Choose a reason for hiding this comment

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

저도 이 부분에 대해서 고민했는데

Dto Model
String VO

String이 널값이면, VO 내에서 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.

지금 이제 마지막 update 구현인데 이거는 조금 더 고민해봐야할 거 같아요.
지금 구조가 마음에 안들긴한데 말씀해주신게 더 좋아보이는거 같긴 하네요 :)

@@ -25,7 +26,7 @@
@JoinTable(name = "article_tag",
joinColumns = @JoinColumn(name = "article_id"),
inverseJoinColumns = @JoinColumn(name = "tag_id"))
@ManyToMany(cascade = CascadeType.PERSIST)
@ManyToMany(cascade = CascadeType.PERSIST, fetch = FetchType.LAZY)
Copy link

Choose a reason for hiding this comment

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

Suggested change
@ManyToMany(cascade = CascadeType.PERSIST, fetch = FetchType.LAZY)
@ManyToMany( fetch = FetchType.LAZY, cascade = CascadeType.PERSIST)

별거 아닐 수 있는데 순서는 중요한 쪽으로 놓는게 좋을 것 같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

안그래도 순서에 대해서 계속 고민하고 있었는데 확인 감사합니다 :)

@@ -13,7 +13,7 @@ spring:
use_sql_comments: true
hibernate:
ddl-auto: create
open-in-view: false
open-in-view: true
Copy link

Choose a reason for hiding this comment

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

OSIV!

Copy link
Member Author

Choose a reason for hiding this comment

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

일단은 키고 사용하게 되어서 켜두었습니다 :)
나중에 변경예정!!

@jinyoungchoi95 jinyoungchoi95 merged commit 3d2a189 into jinyoungchoi95 Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ori 오리의 이슈
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants