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

1186 feat storage request: schema #1208

Draft
wants to merge 28 commits into
base: dev
Choose a base branch
from
Draft

Conversation

shiro-el
Copy link
Contributor

@shiro-el shiro-el commented Nov 13, 2024

요약 *

Storage feature api, db 스키마, api Sto001~012 작성했어요

스크린샷

이후 Task *

작동 검증을 해야할듯?

  • 없음

@shiro-el shiro-el marked this pull request as draft November 16, 2024 14:44
Copy link
Contributor

@Engineer-JJHaMa Engineer-JJHaMa left a comment

Choose a reason for hiding this comment

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

👍 👍 👍 백엔드만 쭉 살펴봤고, 고민하면 좋은 점들 위주로 쭉 메모해 두었어요.
오랜기간 클럽스에서 코딩했다고 해도 믿을 정도로 코딩하는 패턴들이 비슷하게 잘 적혀있어서 읽기 편했습니다!


const url = (applicationId: string) =>
`/executive/storage/applications/application/${applicationId}`;
const method = "PUT";
Copy link
Contributor

Choose a reason for hiding this comment

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

구현하신 내용들은 put 보다는 patch가 적절한 메소드 표현일 것 같습니다. 아래 참고될 만할 글 남겨 두어요!
https://papababo.tistory.com/entry/HTTP-METHOD-PUT-vs-PATCH-%EC%B0%A8%EC%9D%B4%EC%A0%90

Copy link
Contributor

Choose a reason for hiding this comment

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

❓ 이 index.ts 파일이 존재하는 이유는 무엇일까요? 처음엔 이 인덱스 파일을 구성하다가 요즘은 구성을 안해도 된다는 입장인데, 어디에 활용할 수 있는지 알아보면 좋을거 같아요! 바쁘면 스킵해도 좋습니다

packages/api/src/drizzle/schema/storage.schema.ts Outdated Show resolved Hide resolved
return nonStandardItems;
}

async updateApplication(
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ updateApplication은 여러 유저(집행부원과 신청자)가 접근 가능한 메소드로 보입니다. 이때 동시성에 문제가 발생한지는 않을까죠? 만약 두 유저가 거의 동시에 patch를 요청해서 updateApplication에 동시에 진입한다면 문제가 발생하지는 않을까요? 이를 어떻게 해결할까요? 굉장히 중요한 주제이므로 고민해보면 좋겠어요 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

💬 이를 테스트해보고 싶다면 app.module에 넣어야 합니다!

babycroc
babycroc previously approved these changes Nov 20, 2024
Copy link
Contributor

@babycroc babycroc left a comment

Choose a reason for hiding this comment

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

프론트는 초기 구조 세팅 정도만 해둔 것 같아서 실행해보지는 않았고, 코드 위주로만 슥 봤습니닥
어차피 이것저것 추가되면서 나중에 QA 쫙 함 돌아야 해서..! 요 PR은 minor한 코멘트 위주로 남겻어요
(나중에 훅폼까지 붙여주겠지? ㅎㅎㅎ)

setErrorStatus={setUserNameError}
/>
<PhoneInput
placeholder="신청자 전화번호"
Copy link
Contributor

Choose a reason for hiding this comment

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

minor하지만 전화번호 같은 건 입력 형식을 알 수 있게끔 placeholder를 XXX-XXXX-XXXX 같은 걸로 하면 좋아요!

packages/web/src/constants/paths.ts Outdated Show resolved Hide resolved
packages/web/src/app/storage/page.tsx Outdated Show resolved Hide resolved
@babycroc babycroc dismissed their stale review November 20, 2024 06:09

아직 draft군

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.

3 participants