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

[Feature] 모집 신청하기 API #143

Merged
merged 20 commits into from
Nov 30, 2022

Conversation

pythonstrup
Copy link
Member

체크 리스트

  • 적절한 제목으로 수정했나요?
  • 관련된 이슈와 연결 시켰나요?
  • Target Branch를 올바르게 설정했나요?
  • Label을 알맞게 설정했나요?

작업 내역

  • 모집 신청하기 API 작성
  • 예외처리
    • 중복 신청
    • 글 작성자와 참가신청을 했는지
    • 그룹 아이디에 해당하는 그룹이 있는지

문제 상황과 해결

  • 인덱싱을 어떻게 처리할 지 몰라 검색하고 공부했다.
  • 결국 명일 선생님께 여쭤봄
    • 인덱싱된 요소를 순서대로 where에 나열하면 옵티마이저에 의해 자동으로 인덱싱을 탄다고 한다.
    • 도커 로그로 쿼리문이 생성되는 것을 확인했고, where문에 나열되는 것을 확인할 수 있었다.

비고

  • 참고했던 링크 등 참고 사항을 적어주세요. 코드 리뷰하는 사람이 참고해야 하는 내용을 자유로운 형식으로 적을 수 있습니다.

Copy link
Member

@username1103 username1103 left a comment

Choose a reason for hiding this comment

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

@pythonstrup 리뷰 남겼습니다!

Comment on lines +60 to +66
static cancel(userId: number, groupId: number) {
const groupApplication = new GroupApplication();
groupApplication.userId = userId;
groupApplication.groupId = groupId;
groupApplication.status = null;
return groupApplication;
}
Copy link
Member

Choose a reason for hiding this comment

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

cancel 메소드는 사용되고 있는 곳이 없는 것 같아서요. 혹시 어떤 메소드일까요? 모집 신청 취소 메소드인가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

맞습니다. 신청 취소할 때 쓰려고 만들어놓은 메소드입니다.
이런 습관 안 좋은 거 같긴 한데, 커밋에 기능 맞춰서 만들지 않고 필요하다고 생각할 때마다 만드는 버릇이 있네요.
그래도 언젠가는 꼭 쓰일 메소드입니다..

Copy link
Member

Choose a reason for hiding this comment

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

취소하게되면 기존 GroupApplication 엔티티 상태를 변경하는게 아니라 새로운 GroupApplication 엔티티를 만드는건가요/??

Comment on lines 35 to 41
@Column({
type: 'tinyint',
precision: 1,
type: 'varchar',
length: 30,
nullable: true,
comment: '삭제되었으면 NULL 아니면 1',
comment: 'enum형 - GROUP_APPLICATION_STATUS 또는 null',
})
status: number | null;
status: GROUP_APPLICATION_STATUS | null;
Copy link
Member

Choose a reason for hiding this comment

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

status값이 그러면 REGISTER, CANCEL, 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.

넵 맞습니다. 실제 로직에서는 CANCEL이 아니라 null을 넣어야한다고 하셨으니 그렇게 하려고 합니다.
무슨 문제가 있을까요??

Copy link
Member

Choose a reason for hiding this comment

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

개발자가 2명이니까 괜찮겠지만 다른 사람이 보면 알아보기 힘들것 같아요. 지금은 괜찮겠지만 추후에 typeorm transformer라는 걸 활용해 엔티티를 불러올 때, 변경해서 사용할 수 있도록 하면 좋을 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

인덱싱 때문에 DB에는 어쩔 수 없이 null이 들어가도록 만들어야하지만 엔티티에는 null이 들어가지 않고 CANCEL 넣자는 말씀이시군요! 바꾸도록 하겠습니다!!!

Copy link
Member Author

Choose a reason for hiding this comment

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

적용했습니다!!

import { BadRequestException } from '@nestjs/common';

export class CannotApplicateException extends BadRequestException {
constructor(message = '당신이 만든 그룹에 신청을 할 수 없습니다.!') {
Copy link
Member

Choose a reason for hiding this comment

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

메시지가 .도 있고 !도 있고 .!도 있네요. 하나로 통일하면 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

반영했습니다! .으로 통일했어요

Comment on lines 26 to 32
async validateGroupId(groupId: number) {
const group = await this.groupRespository.findById(groupId);
if (!group) {
throw new GroupNotFoundException();
}
return group.article;
}
Copy link
Member

Choose a reason for hiding this comment

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

groupArticle을 바로 가져오지 않고 group을 가져와서 article을 가져온 이유가 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

원래는 명일님 말처럼 그렇게 하려고 했습니다만, 이유가 있습니다!

  1. groupArticle을 가져오려면 service에서 repository를 GroupArticleRepository를 추가해야하는 불편함이 있습니다.
  2. 어차피 그룹을 확인하는 validate할 때 group을 가져오게 되고, 이를 다음 유효성 검증인 작성자와 신청자 동일 여부 확인에도 활용할 수 있으면 좋을 것 같다고 생각했습니다.
  3. 작성자는 Group테이블의 요소로 존재하지 않고 Article에만 존재하기 때문에 이것을 가지고 온 것 입니다.
  4. 고민해본 결과, 그냥 group만 리턴하고 다른 함수에서 사용할 수 있게 하는 것이 훨씬 더 유연해보이네요!

Comment on lines 4 to 12
export class AttendGroupRequest {
@ApiProperty({
example: 1,
description: '그룹 아이디',
required: true,
})
@IsNumber()
groupId: number;
}
Copy link
Member

Choose a reason for hiding this comment

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

그룹아이디가 Group 엔티티 아이디인가요 아니면 GroupArticle아이디 인가요??

Copy link
Member Author

Choose a reason for hiding this comment

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

image

  • ERD를 확인했을때 Group 테이블로 신청하는 것으로 되어있었기 때문에 Group 엔티티의 Id라고 생각했고 그대로 적었습니다.

Copy link
Collaborator

@kong430 kong430 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

@sxungchxn sxungchxn left a comment

Choose a reason for hiding this comment

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

고치고 머지해주세요~

import { Injectable } from '@nestjs/common';
import { DataSource, Repository } from 'typeorm';
import { GroupApplication } from '@app/group-application/entity/group-application.entity';
import { GROUP_APPLICATION_STATUS } from '../group-article/constants/group-article.constants';
Copy link
Member

Choose a reason for hiding this comment

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

또 빼먹으셨군요!

Copy link
Member Author

Choose a reason for hiding this comment

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

ㅠㅜ 이럴수가..
반영했습니다.

@pythonstrup pythonstrup merged commit 136dec8 into develop Nov 30, 2022
@pythonstrup pythonstrup deleted the feature/137-group-application-api branch December 1, 2022 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] 모집 신청하기 API
4 participants