Skip to content
This repository has been archived by the owner on Aug 13, 2022. It is now read-only.

[#1] Create Account #3

Merged
merged 24 commits into from
Sep 23, 2019
Merged

[#1] Create Account #3

merged 24 commits into from
Sep 23, 2019

Conversation

jjeda
Copy link
Collaborator

@jjeda jjeda commented Sep 6, 2019

  • Account 도메인추가
  • AccountDto, AccountService, AccountController 추가
  • Account 생성을 위한 로직 추가

- Account Entity 생성
- View Layer 와 DB Layer 분리를 위한 DTO 생성
- 영속성을 위한 AccountRepository 인터페이스 생성
- Account create에 대한 Test code 작성
- Account create에 대한 Service, controller 비즈니스 로직 완성
@jjeda
Copy link
Collaborator Author

jjeda commented Sep 6, 2019

@JsonIgnoreProperties({"email", "password", "phone", "address", "accountRole"})
를 주석 처리한 이유는 DB에는 개인정보에 대한 내용을 저장하고 메시지에만 그내용들을 포함하지 않기 위함이었는데
위에 애노테이션을 사용하면 DB에도 값이 들어가지 않는것을 확인했습니다!
이런경우에 어떻게 처리해야할지 궁금합니다

@jjeda jjeda requested a review from f-lab-dev September 6, 2019 07:49
@jjeda jjeda self-assigned this Sep 6, 2019
@f-lab-dev
Copy link
Member

f-lab-dev commented Sep 6, 2019

@jjeda 저런 개인정보는 보내지 않는게 아니라 보통 몇 글자만 남기고 마스킹처리합니다. 비즈니스로직상에서 마스킹하셔도 되고, 별도의 메소드를 만들어 마스킹하셔도 됩니다. (어떤 상황에 쓸 것이냐에 따라 다르기 때문에 가능한 상황을 모두 정의해보시는 것도 좋습니다)

DB에 안들어가는것은 setter에도 JsonIgnore처리가 되었기 때문입니다. 그래서 클라이언트가 json으로 값을 전송해도 무시하는것입니다.

Copy link
Member

@f-lab-dev f-lab-dev left a comment

Choose a reason for hiding this comment

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

전체적으로 구조와 문법을 사용하시는게 깔끔하네요. 일단 기본적인 구조에 대해서는 지식이 탄탄하신 것 같아서 좋습니다👍
비즈니스 로직을 추가하면 어떻게 될지 기대됩니다

pom.xml Outdated
<artifactId>mysql-connector-java</artifactId>
<scope>runtime</scope>
</dependency>
<!--<dependency>-->
Copy link
Member

Choose a reason for hiding this comment

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

주석은 커밋 안하셔도 됩니다. 실제 소스에 적용할 내용만 커밋하시고, 로컬 테스트용 소스는 커밋에서 제외하시는게 좋습니다~
(코드리뷰도 리뷰해주는 사람이 완벽하지 않기에 혹시라도 이 내용을 지나쳐서 실제 소스에 합쳐지면 잘못된 결과가 나올 수도 있으니까요)

Account account = accountService.saveAccount(requestAccount);
URI uri = ControllerLinkBuilder.linkTo(AccountController.class).slash(account.getId()).toUri();

return ResponseEntity.created(uri).body(account);
Copy link
Member

Choose a reason for hiding this comment

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

StatusCode를 잘 활용하셨네요👍

@RequestMapping(value = "/api/accounts", produces = MediaTypes.HAL_JSON_UTF8_VALUE)
public class AccountController {

AccountService accountService;
Copy link
Member

Choose a reason for hiding this comment

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

접근제한자는 달아주는게 좋을 것 같습니다


@RestController
@AllArgsConstructor
@RequestMapping(value = "/api/accounts", produces = MediaTypes.HAL_JSON_UTF8_VALUE)
Copy link
Member

Choose a reason for hiding this comment

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

produces = MediaTypes.HAL_JSON_UTF8_VALUE 굳이 달아주지 않아도 될 것 같습니다
기본 리턴 타입이 json입니다~

import lombok.Getter;
import lombok.NoArgsConstructor;

import javax.persistence.*;
Copy link
Member

Choose a reason for hiding this comment

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

대부분의 코딩 컨벤션에서 *을 사용하는 것은 지양하고 있습니다.
프로젝트에 check style을 적용해보시면 이런 컨벤션을 자동으로 검증해줍니다.
intellij check style로 검색해보시면 좋을 것 같습니다~

@@ -0,0 +1,5 @@
package me.jjeda.mall.accounts.domain;

public enum AccountRole {
Copy link
Member

Choose a reason for hiding this comment

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

enum을 잘 활용하셨습니다~

private MockMvc mockMvc;

@Test
@TestDescription("정상적으로 계정을 생성하는 테스트")
Copy link
Member

Choose a reason for hiding this comment

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

테스트에 주석을 달기 위해서 따로 어노테이션을 정의하신게 정말 좋네요
junit5에는 따로 지원을 해주고 있으니 버전을 올려보셔도 괜찮을 것 같아요~

@Embedded
private Address address;

private LocalDateTime createdDate;
Copy link
Member

Choose a reason for hiding this comment

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

날짜 따로 잡아두신 것도 좋네요
다만 이름을 createdAt & updatedAt으로 하는 것이 더 일반적입니다~


import java.time.LocalDateTime;

//@JsonIgnoreProperties({"email", "password", "phone", "address", "accountRole"})
Copy link
Member

Choose a reason for hiding this comment

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

이렇게 하면 직렬화뿐 아니라 역직렬화에서도 무시됩니다
JsonIgnore를 getter에만 적용하는 방법이 있으니 검색해보시면 좋을 것 같습니다~

@PostMapping
public ResponseEntity createAccount(@RequestBody AccountDto requestAccount) {
Account account = accountService.saveAccount(requestAccount);
URI uri = ControllerLinkBuilder.linkTo(AccountController.class).slash(account.getId()).toUri();
Copy link
Member

Choose a reason for hiding this comment

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

이 정보는 굳이 보내주지 않아도 되긴 합니다만 클라이언트에서 필요가 있어서 쓰신 것 같네요

- pom.xml 수정 : 불필요한 주석 커밋 제외
- AccountController 수정 : 접근제한자 추가, produces = MediaTypes.HAL_JSON_UTF8_VALUE 삭제 (Defalut)
- Account 객체 import 수정
- Account 객체 날짜변수 이름 수정 createDate, modifiedDate -> createdAt, modifiedAt
Copy link
Member

@f-lab-dev f-lab-dev left a comment

Choose a reason for hiding this comment

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

수정내용 확인했습니다~

README.md Outdated
## TODO
- intellij check style
Copy link
Member

Choose a reason for hiding this comment

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

TODO같은 이슈들의 관리는 깃허브 이슈로 올리셔서 관리하시면 좋을 것 같습니다~
소스코드 변경이 필요한 이슈의 경우는 브랜치도 따로 분리해서 개발하고요

- Account 전체 목록 불러오는 테스트 코드 작성 -> 로직 작성
- Account 하나 불러오는 테스트 코드 작성 -> 로직
- Account 하나 삭제하는 테스트 코드 작성 -> 로직
@jjeda
Copy link
Collaborator Author

jjeda commented Sep 8, 2019

새로운 commit 내용으로 pull request 를 하려고 했는데, 새로 만들어지는 것이아니라
기존 pull request에 commit 이 추가 되는걸로 파악 했습니다.

  1. 하나의 branch 는 하나의 pull request 만 가능한걸로 이해했는데 맞나요?
  2. 그래서 소스코드 변경이 필요한 이슈를 브랜치 따로 분리 하라는 말씀이었나요~?

- README TODO 내용 git issue 로 관리
@f-lab-dev
Copy link
Member

@jjeda

  1. 네 맞습니다~ 1브랜치 1PR입니다. PR이 완료되어서 닫힌 상태라면 다시 날릴 수 있지만 open상태의 PR은 브랜치당 1개입니다.

  2. 맞습니다. 이슈를 잘게 나눌수록 관리하기 편해집니다. 다른 이슈와 관련된 소스코드 변경이 합쳐지면 잘못 의존하는 코드를 만들 수도 있는 등 복잡함과 착오가 생길 가능성이 있습니다.

그래서 이 브랜치(#1)를 Account와 관련된 베이스 브랜치로 사용하고, Account와 관련된 작은 이슈들은 이 브랜치(feature/1)에서 다시 새로운 브랜치를 생성해서 작업한 다음, feature/1브랜치로 PR을 날려도 됩니다.

Copy link
Member

@f-lab-dev f-lab-dev left a comment

Choose a reason for hiding this comment

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

리뷰 완료했습니다~

README.md Outdated
@@ -1,2 +0,0 @@
## 0. 들어가며
Copy link
Member

Choose a reason for hiding this comment

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

이 내용은 지우지 않아도 될 것 같습니다. (이슈와 관련된게 아니니까요)

README.md는 다른 사람들이 이 프로젝트에 대한 내용을 쉽게 파악하기 위해 정리하는 곳으로 사용하시면 됩니다.
그래서 프로젝트의 목적과 기술선택의 이유 등 프로젝트에 관한 전반적인 내용을 여기에 정리하면 다른 사람들이 프로젝트의 목적을 파악하고 소스코드를 읽는데에 더 도움이 될 수 있습니다.

@@ -39,7 +35,7 @@
<dependency>
<groupId>com.h2database</groupId>
<artifactId>h2</artifactId>
<scope>runtime</scope>
<scope>test</scope>
Copy link
Member

Choose a reason for hiding this comment

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

[참고] 이 데이터베이스의 필요성에 대해 생각해보시면 좋을 것 같습니다.
단위 테스트를 작성할 때 이렇게 임시 데이터베이스를 사용하거나, 혹은 Repository 클래스를 Mocking해서 사용하는데 각각의 장단점에 대해 생각해보시고, 나중에 그 철학을 어필해보시면 좋을 것 같습니다.

@@ -1 +1 @@

spring.h2.console.enabled=true
Copy link
Member

Choose a reason for hiding this comment

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

h2는 테스트시에만 사용되므로 test/resources 디렉토리에 따로 만들어주면 좋을 것 같습니다.

}

@GetMapping
public ResponseEntity queryAccount(Pageable pageable, PagedResourcesAssembler<Account> pagedResourcesAssembler) {
Copy link
Member

Choose a reason for hiding this comment

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

이 기능이 꼭 필요한지 생각해봐야할 것 같습니다. 단순 전체 유저의 정보를 보여줘야할 필요는 없을 것 같고, 특정 조건을 만족하는 유저정보는 어드민페이지에서 필요할 것 같긴 합니다.

return ResponseEntity.ok(pagedResources);
}

@GetMapping("/{id}")
Copy link
Member

Choose a reason for hiding this comment

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

본인의 정보를 조회하는 API(유저)와 타인의 정보를 조회하는 API(어드민)을 분리하면 좋을 것 같습니다.
추후에 각각에 맞는 권한도 추가해주시면 좋을 것 같고요.

return ResponseEntity.ok(account);
}

@DeleteMapping("/{id}")
Copy link
Member

Choose a reason for hiding this comment

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

이곳도 마찬가지로 관리자에 의한 강제탈퇴와 스스로의 회원탈퇴를 구분하면 좋을 것 같습니다

}

public void deleteAccount(Long id) {
accountRepository.deleteById(id);
Copy link
Member

Choose a reason for hiding this comment

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

보통 회원탈퇴를 하면 바로 삭제하는 것이 아니라 재가입을 대비해 삭제했다는 플래그만 남기고 정보를 저장해둡니다.
(혹은 몇 일 뒤에 완전 삭제 이렇게 할 수도 있겠네요)

- GET, DELETE Method 회원, ADMIN로 분리
- 전체 Account List 불러오기 -> 삭제 or 삭제되지 않은 Account List 불러오기
- Local 환경 세팅
- ddl-auto = create 추가
- show-sql & format_sql 추가
import org.springframework.web.bind.annotation.RestController;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.DeleteMapping;
import org.springframework.web.bind.annotation.*;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

앗 이부분이 메서드 수정하는 부분에서 자동으로 import 되었네요
다음 commit에 다시 수정하겠습니다 + intellij code check를 설치만 했는데 좀 더 알아봐야겠네요..

accountRepository.deleteById(id);
Account account = accountRepository.findById(id).get();
account.setDeleteFlag();
accountRepository.save(account);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

JPA에서 같은 트랜잭션안에서 변경이 일어나면 자동으로 변경된 값으로 DB에 저장되는 것으로 알고있습니다.
그렇기 때문에 @transaction 애노테이션을 사용하고 42번 라인을 빼도 되는 것으로 알고있습니다.
이 경우에 save 메서드를 호출하지않고 @transaction 을 사용하는게 맞는것인가요?

Copy link
Member

Choose a reason for hiding this comment

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

굳이 save를 쓰시지 않아도 저장됩니다.
https://nkcnow.tistory.com/273 를 참고해보세요. 영속성 컨텍스트에 대해 더 검색해봐도 좋을 것 같습니다.
그리고 코드를 쓰실땐 마크다운 문법으로 `코드`를 쓰시는게 좋습니다. 저기 transaction이라는 실제 유저가 태그된것 같네요 ㅎㅎ

- update 관련 test 코드 작성
- update method 구현
@PutMapping("/{id}")
public ResponseEntity updateAccount(@PathVariable Long id, @RequestBody AccountDto accountDto) {
Account account = accountService.updateAccount(id,accountDto);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

admin페이지에서 admin이 회원의 정보를 수정할 일이 없을 것같아 회원이 직접 수정하는 메서드만 구현하였는데 제가 놓친 부분이 있을까요?

Copy link
Member

Choose a reason for hiding this comment

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

네 직접 수정할 일은 없을 것 같습니다. 그럼 id를 파라미터로 받는게 아니라 id는 세션에서 가져와야할 것 같습니다. 추후에 로그인 구현하고 하셔도 될 것 같네요~

- Spring security oAuth Dependency 추가
- Security config 클래스 추가
- Account 객체 내 userName 필드 -> nickname 으로 변경
- AccountService 객체 -> UserDetailService Implements
- 인증 & 인가를 위한 Auth 서버 설정
- AuthServer 설정
- ResourceServer 설정
- SecurityConfig -> 공통설정
- RedisTokenStore 관련 코드 추가
- 권한타입 String -> Role 변경
- PassEncoder @bean을 위한 공통 config 클래스 생성
- 명시성을 위해 빈주입시 @AllArgsConstructor -> 생성자
- AccountDto 객체에 Validation 추가
- Validation 검증을 위한 테스트코드 추가
- 관리자와 유저 account 컨트롤러 분리
- Role에 따른 Authrization 범위 설정
- Authrization 테스트 코드 추가
- isDeleted Flag 삭제 -> 상태를 다양화 할 수 있는 AccountStatus enum 추가
- 상태전이 코드 추가
- 사용자가 @Pathvariable을 통해 endpoint로 접근하는 것이 아닌 현재사용자 정보를 받아와 접근하게 수정
- 기타 : import에 check style 적용, Appconfig 이름 역할에 맞게 수정
- AccountAdapter 클래스 구현 ( extends User)
- 어댑터 패턴 적용
- Local 개발용 application-defalut.properties 설정
- schema.sql 추가
- expression = account SPEL을 통해 간결하게 변경
- 간결함, 가독성을 위해 CurrentUser 애노태이션 정의
- unused import 삭제
- 변수명 및 매서드명 수정
@jjeda jjeda merged commit 58257f2 into develop Sep 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants