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

create notice board post, getAll apis #74

Merged
merged 40 commits into from
Dec 5, 2023
Merged

Conversation

hobiJeong
Copy link
Collaborator

Description

공지게시판 게시글 작성, 전체 조회 api를 만들었습니다.

To Reviewer

엔티티 변경 및 history 엔티티 및 service도 추가 되었습니다.

Reference Link

Related Issue Link

#66

API

Method Path 설명 작업(추가, 수정, 삭제)
GET /api/notice-boards 공지 게시글 전체 조회 추가
POST /api/notice-boards 공지 게시글 생성 추가

@hobiJeong hobiJeong added the Feat 새로운 기능 추가 label Dec 3, 2023
@hobiJeong hobiJeong self-assigned this Dec 3, 2023
@hobiJeong hobiJeong linked an issue Dec 3, 2023 that may be closed by this pull request
6 tasks
This was referenced Dec 4, 2023
Copy link
Member

@rrgks6221 rrgks6221 left a comment

Choose a reason for hiding this comment

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

#79 #81 에서 history 와 soft delete 를 위한 컬럼들이 추가 변경 됐는데 한번 확인 부탁드리고 해당 PR 에 반영돼도 좋을거같아요.


@ApiProperty({
description: '댓글 허용 여부 (false: 비활성화, true: 허용)',
default: true,
Copy link
Member

Choose a reason for hiding this comment

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

이거 default 가 없는거아닌가요??

description: 'title 필터링',
maxLength: NOTICE_BOARD_TITLE_LENGTH.MAX,
})
@Length(NOTICE_BOARD_TITLE_LENGTH.MIN - 1, NOTICE_BOARD_TITLE_LENGTH.MAX)
Copy link
Member

Choose a reason for hiding this comment

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

min 에 -1 은 왜 하는건가요???

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Optional인데 min이 1이 걸려 있으면 안되지 않을까 했는데 직접 돌려 보니까 상관 없네용 수정하겠습니다!

})
@IsBoolean()
@IsOptional()
@Type(() => Boolean)
Copy link
Member

Choose a reason for hiding this comment

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

query string 은 문자열로 평가되기때문에 Type decorator 로 boolean 변환을 한다면 'true', 'false' 둘다 js 상에서 true 로 평가돼서 값이 들어오는 경우에 무조건적으로 true 로 들어오게 될거같네요

comment: '공지게시글 내용',
},
{
name: 'allow_comment',
Copy link
Member

Choose a reason for hiding this comment

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

이게 원래 전에 notice_board 를 생성할 때 했어야하는 피드백인데 allow_comment 는 허용여부라는 의미를 정확하게 표현하지 못하는거같아요 is_allow_comment 같은 컬럼명으로 변경하는건 어떨까요??
변경한다면 notice_board table 도 같이 변경돼야할듯합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

수정하고 다시 리뷰요청 하겠습니다!

migrations/1701328900610-notice-board-history.ts Outdated Show resolved Hide resolved

@ApiNoticeBoard.Create({ summary: '공지 게시글 생성 API' })
@UseGuards(JwtAuthGuard)
@SetResponse({ type: ResponseType.Detail, key: 'board' })
Copy link
Member

Choose a reason for hiding this comment

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

key 에 notice 도 포함되면 좋을거같네요

default: true,
})
@IsBoolean()
allowComment: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

migration 에서 드렸던 피드백을 수용한다면 여기도 변경돼야할듯 합니다.

src/entities/FreeBoard.ts Show resolved Hide resolved
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

얜 칼럼명 변경하면서 사용한 마이그레이션데 삭제해야 할 것 같으면 지우겠습니다.

@hobiJeong
Copy link
Collaborator Author

#79 #81 에서 history 와 soft delete 를 위한 컬럼들이 추가 변경 됐는데 한번 확인 부탁드리고 해당 PR 에 반영돼도 좋을거같아요.

아 이거 반영하고 다시 리뷰요청 넣겠습니다 죄송합니다

Copy link
Member

@rrgks6221 rrgks6221 left a comment

Choose a reason for hiding this comment

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

상태 변경용

Copy link
Member

@rrgks6221 rrgks6221 left a comment

Choose a reason for hiding this comment

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

dto 기본값반 수정하면 될듯합니다.

order: Order<typeof NOTICE_BOARD_ORDER_FIELD> = { id: SortOrder.Desc };

@IsDefined()
status: NoticeBoardStatus.Posting;
Copy link
Member

Choose a reason for hiding this comment

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

여기 기본값이 필요할듯합니다.

default: NoticeBoardStatus.Posting,
})
@IsDefined()
status: NoticeBoardStatus.Posting;
Copy link
Member

Choose a reason for hiding this comment

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

swagger 상에 디폴트가 아닌 서버 내부에서 디폴트가 필요할듯해요
게시글 생성 시 clident 측에서 게시글의 상태를 핸들링하게할 수 없으니 서버 내부에서 기본적으로 Posting 상태로 되게끔 해야합니다. ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

앗 넵!

@IsOptional()
order: Order<typeof NOTICE_BOARD_ORDER_FIELD> = { id: SortOrder.Desc };

@ApiProperty({
Copy link
Member

Choose a reason for hiding this comment

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

클라이언트측에 굳이 노출하지 않아도 되는 필드라 api propery 데코레이터는 제거하는게 좋을거같아요

@rrgks6221
Copy link
Member

@hobiJeong 충돌 해결 후 머지해주세요 고생하셨습니다.

@hobiJeong hobiJeong merged commit f3dd2e2 into develop Dec 5, 2023
@hobiJeong hobiJeong deleted the feat/#66/notice_board_crud branch December 5, 2023 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feat 새로운 기능 추가
Projects
None yet
Development

Successfully merging this pull request may close these issues.

notice board crud
2 participants