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

학생회 활동보고 crud #335

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

학생회 활동보고 crud #335

wants to merge 6 commits into from

Conversation

leeeryboy
Copy link
Contributor

report crud api

한줄 요약

CouncilEntity가 intro, report 까지 커버
소개 페이지 하나 때문에 type을 만드는게 좀 불필요하다고 생각해서 repository를 보면 title이 intro 인걸 제외하도록 되어있는데 id가 바뀔일이 없을거라 db 레코드 만든 후에 id 제외하도록 하는게 나을 것 같기도 한데 의견 부탁드립니다.

@leeeryboy leeeryboy requested a review from huGgW February 4, 2025 11:27
@leeeryboy leeeryboy self-assigned this Feb 4, 2025
Copy link

github-actions bot commented Feb 4, 2025

Test Results

11 files  11 suites   1s ⏱️
54 tests 54 ✔️ 0 💤 0
58 runs  58 ✔️ 0 💤 0

Results for commit 3bb663e.

♻️ This comment has been updated with latest results.

Copy link
Member

@huGgW huGgW left a comment

Choose a reason for hiding this comment

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

자세히는 내일봄 (문제는 없는거 같긴 하지만)


@Service
@Transactional
Copy link
Member

Choose a reason for hiding this comment

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

마이너하긴 한데 method에 붙이는 걸로 변경하면 더 좋을 듯!
(어차피 readonly 따로 명시하고 있어서 class에 붙이는 이점도 적은 듯 해서)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

readonly만 붙이는게 덜 번거롭지 않나?.?

Copy link
Member

Choose a reason for hiding this comment

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

번거로운 것도 있는데 transaction과 연관 없는 method까지 proxy 타는게 싫어서 그러긴 했는데
큰 상관은 없어서 이대로 가도 괜찮긴 할듯!

@huGgW
Copy link
Member

huGgW commented Feb 4, 2025

report crud api

한줄 요약

CouncilEntity가 intro, report 까지 커버 소개 페이지 하나 때문에 type을 만드는게 좀 불필요하다고 생각해서 repository를 보면 title이 intro 인걸 제외하도록 되어있는데 id가 바뀔일이 없을거라 db 레코드 만든 후에 id 제외하도록 하는게 나을 것 같기도 한데 의견 부탁드립니다.

id는 혹시나 마이그레이션이나 db 수정 있을 때 꼬일수도 있을거 같아서 조금 걱정되기는 하고,
제일 좋은거는 추후 혹시라도 확장될 가능성을 대비해서 type을 만드는게 좋다고 생각하기는 하는데,
당장 필요없다고 생각하면 title만 조금 복잡한 걸로 해놓고 CRUD에 가드하는 거 잘 챙기면 되지 않을까 합니다. (대신 주석 잘 남겨놓긴 해야될듯)

@leeeryboy
Copy link
Contributor Author

leeeryboy commented Feb 5, 2025

그냥 type 추가했습니다

Copy link
Member

@huGgW huGgW left a comment

Choose a reason for hiding this comment

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

리뷰 남기긴 했는데 옵셔널이긴 하고 flyway script만 적용하면 될듯!


@Service
@Transactional
Copy link
Member

Choose a reason for hiding this comment

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

번거로운 것도 있는데 transaction과 연관 없는 method까지 proxy 타는게 싫어서 그러긴 했는데
큰 상관은 없어서 이대로 가도 괜찮긴 할듯!

Comment on lines +13 to +33
@Query(
"""
SELECT c
FROM council c
WHERE c.createdAt < :timestamp
AND c.type = 'REPORT'
ORDER BY c.createdAt DESC
"""
)
fun findPreviousReport(@Param("timestamp") timestamp: LocalDateTime): CouncilEntity?

@Query(
"""
SELECT c
FROM council c
WHERE c.createdAt > :timestamp
AND c.type = 'REPORT'
ORDER BY c.createdAt ASC
"""
)
fun findNextReport(@Param("timestamp") timestamp: LocalDateTime): CouncilEntity?
Copy link
Member

Choose a reason for hiding this comment

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

findNextByType 이런식으로 type 관계없이 동작하도록 하면 좀 더 좋을듯!

Comment on lines +18 to +28
fun of(entity: CouncilEntity, prev: CouncilEntity?, next: CouncilEntity?): ReportDto = ReportDto(
id = entity.id,
title = entity.title,
description = entity.description,
author = entity.author.name,
createdAt = entity.createdAt!!,
prevId = prev?.id,
prevTitle = prev?.title,
nextId = next?.id,
nextTitle = next?.title
)
Copy link
Member

Choose a reason for hiding this comment

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

CouncilEntity type이 REPORT 아닐 시 IllegalArgumentException을 던진다던지 하는 식으로 무언가의 검사는 있으면 좋을듯?

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.

2 participants