-
Notifications
You must be signed in to change notification settings - Fork 0
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
Issue/168/update best review #169
base: dev
Are you sure you want to change the base?
Changes from 2 commits
890bbe7
1cae752
6f50cde
5d82844
1604dd8
1baca8b
fc045e5
0d65e55
2d889ab
e481eff
75fe8b4
e916be6
e49b35b
005761a
94af378
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,7 @@ | |
"@nestjs/jwt": "^10.2.0", | ||
"@nestjs/passport": "^10.0.3", | ||
"@nestjs/platform-express": "^10.4.15", | ||
"@nestjs/schedule": "^4.1.2", | ||
"@nestjs/swagger": "^8.1.0", | ||
"@prisma/client": "^6.1.0", | ||
"@slack/web-api": "^7.8.0", | ||
|
@@ -142,5 +143,6 @@ | |
}, | ||
"overrides": { | ||
"glob": "^9" | ||
} | ||
}, | ||
"packageManager": "[email protected]+sha512.1acb565e6193efbebda772702950469150cf12bcc764262e7587e71d19dc98a423dff9536e57ea44c49bdf790ff694e83c27be5faa23d67e0c033b583be4bfcf" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,13 +10,13 @@ import { | |
Query, | ||
} from '@nestjs/common'; | ||
import { session_userprofile } from '@prisma/client'; | ||
import { ReviewProhibited } from '@src/common/decorators/prohibit-review.decorator'; | ||
import { GetUser } from 'src/common/decorators/get-user.decorator'; | ||
import { Public } from 'src/common/decorators/skip-auth.decorator'; | ||
import { IReview } from 'src/common/interfaces/IReview'; | ||
import { ReviewsService } from './reviews.service'; | ||
import { EReview } from '../../common/entities/EReview'; | ||
import { ReviewsService } from './reviews.service'; | ||
import EReviewVote = EReview.EReviewVote; | ||
import { ReviewProhibited } from '@src/common/decorators/prohibit-review.decorator'; | ||
|
||
@Controller('api/reviews') | ||
export class ReviewsController { | ||
|
@@ -52,6 +52,13 @@ export class ReviewsController { | |
} | ||
} | ||
|
||
@Get('update-best-reviews') | ||
async triggerUpdateBestReviews() { | ||
console.log('Triggered manual update for Best Reviews'); | ||
await this.reviewsService.updateBestReviewsCron(); | ||
return { message: 'BestReviews update triggered successfully' }; | ||
} | ||
|
||
@Public() | ||
@Get(':reviewId') | ||
async getReviewInstance( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 코드 패치에 대한 리뷰는 다음과 같습니다:
이와 같은 수정사항을 고려하면 코드의 품질과 안정성을 높일 수 있을 것입니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Gerbera3090 이렇다고 하네요. console.log는 지워주세용 |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,11 @@ | ||
import { Transactional } from '@nestjs-cls/transactional'; | ||
import { HttpException, HttpStatus, Injectable } from '@nestjs/common'; | ||
import { session_userprofile } from '@prisma/client'; | ||
import { Cron } from '@nestjs/schedule'; | ||
import { review_review, session_userprofile } from '@prisma/client'; | ||
import { IReview } from 'src/common/interfaces/IReview'; | ||
import { toJsonReview } from 'src/common/interfaces/serializer/review.serializer'; | ||
import { LectureRepository } from 'src/prisma/repositories/lecture.repository'; | ||
import { ReviewsRepository } from 'src/prisma/repositories/review.repository'; | ||
import { Transactional } from '@nestjs-cls/transactional'; | ||
import { EReview } from '../../common/entities/EReview'; | ||
import EReviewVote = EReview.EReviewVote; | ||
|
||
|
@@ -174,4 +175,68 @@ export class ReviewsService { | |
): Promise<EReviewVote.Basic> { | ||
return await this.reviewsRepository.upsertReviewVote(reviewId, user.id); | ||
} | ||
|
||
@Cron('0 0 * * *') // 매일 자정에 실행하는 Cron 작업 설정: 과거 update-best-reviews.py를 계승 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이거 이렇게 해놓으면 cron 도는거에요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이거 서버에서 지금 실행되는거죠..? 저희가 서버가 여러개라서 이렇게 해놓으면 모든 서버에서 다 돌아갈거에요. 지금 상황은 그런데 저거 쓰는거는 긍정적인 방향인거 같긴 합니다.
로 바꾸는게 좋을 거 같네요. 이 작업까지 한 번에 해줄래요 아니면 잘라서 진행할래요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 아마 그러면 secure-vm에 cron에 열어뒀던 get요청 보내는 코드를 추가하면 될 것 같습니다. 지금 cron은 주석 해두고요. |
||
async updateBestReviewsCron() { | ||
function calculateKey(review: EReview.WithLectures): number { | ||
const baseYear = new Date().getFullYear(); | ||
const lectureYear = review.lecture.year; | ||
const yearDiff = baseYear - lectureYear > 0 ? baseYear - lectureYear : 0; | ||
return Math.floor( | ||
(review.like / (review.lecture.audience + 1)) * | ||
Math.pow(0.85, yearDiff), | ||
); | ||
} | ||
|
||
function getBestReviews( | ||
reviews: EReview.WithLectures[], | ||
minLikedCount: number, | ||
maxResultCount: number, | ||
): review_review[] { | ||
const likedCount = Math.max( | ||
minLikedCount, | ||
Math.floor(reviews.length / 10), | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이거 이렇게 메소드 안에 함수 쓰면 아마..? 함수 오브젝트가 계속 생성될거라서 분리 부탁드립니다 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저 calculateKey 함수는 어떤건가요...? 약간 시간이 흐름에 따라 감쇠하는 factor가 있는건가..? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이거 혹시 하고 싶다면...? 그냥 좋아요 개수 / 청중 수를 감쇠로하지 말고, 시간 흐름별 좋아요 개수를 moving average로 해보는건 어때요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 아마 시간이 지날수록 해당 과목이 현재랑은 좀 달라져서 효과는 떨어지니까 그걸 노리고 만들었던 것 같아요 |
||
|
||
const mostLikedReviews = reviews | ||
.sort((a, b) => calculateKey(b) - calculateKey(a)) | ||
.slice(0, likedCount); | ||
|
||
const latestDateStart = new Date(); | ||
latestDateStart.setDate(latestDateStart.getDate() - 7); | ||
|
||
const latestReviews = reviews.filter( | ||
(review) => | ||
review.written_datetime && | ||
new Date(review.written_datetime) >= latestDateStart, | ||
); | ||
|
||
const bestCandidateReviews = [...mostLikedReviews, ...latestReviews]; | ||
return bestCandidateReviews.length > maxResultCount | ||
? bestCandidateReviews.slice(0, maxResultCount) | ||
: bestCandidateReviews; | ||
} | ||
|
||
console.log('Running scheduled job: Update Best Reviews'); | ||
|
||
// Process humanity reviews | ||
const humanityReviews = await this.reviewsRepository.getHumanityReviews(); | ||
const humanityBestReviews = getBestReviews(humanityReviews, 50, 20); | ||
|
||
await this.reviewsRepository.clearHumanityBestReviews(); | ||
await this.reviewsRepository.addHumanityBestReviews( | ||
humanityBestReviews.map((r) => ({ reviewId: r.id })), | ||
); | ||
|
||
// Process major reviews | ||
const majorReviews = await this.reviewsRepository.getMajorReviews(); | ||
const majorBestReviews = getBestReviews(majorReviews, 2000, 1000); | ||
|
||
await this.reviewsRepository.clearMajorBestReviews(); | ||
await this.reviewsRepository.addMajorBestReviews( | ||
majorBestReviews.map((r) => ({ reviewId: r.id })), | ||
); | ||
|
||
console.log('BestReview updated by scheduled job'); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 코드 리뷰를 진행하겠습니다. 전체적으로 동작 로직은 명확하게 작성되어 있습니다. 그러나 다음과 같은 개선 사항 및 버그 위험 요소들이 있습니다:
이러한 지점들을 고려하면 코드의 품질과 유지보수성이 높아질 것으로 생각됩니다. 코드를 잘 작성하셨습니다. 앞으로도 계속 개선하는 방향으로 나아가시길 바랍니다! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
-- CreateIndex | ||
CREATE INDEX `review_humanitybestreview_review_id_idx` ON `review_humanitybestreview`(`review_id`); | ||
|
||
-- CreateIndex | ||
CREATE INDEX `review_majorbestreview_review_id_idx` ON `review_majorbestreview`(`review_id`); | ||
|
||
-- AddForeignKey | ||
ALTER TABLE `review_humanitybestreview` ADD CONSTRAINT `review_humanitybestreview_review_id_fkey` FOREIGN KEY (`review_id`) REFERENCES `review_review`(`id`) ON DELETE CASCADE ON UPDATE CASCADE; | ||
|
||
-- AddForeignKey | ||
ALTER TABLE `review_majorbestreview` ADD CONSTRAINT `review_majorbestreview_review_id_fkey` FOREIGN KEY (`review_id`) REFERENCES `review_review`(`id`) ON DELETE CASCADE ON UPDATE CASCADE; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 코드 패치는 두 개의 인덱스를 생성하고, 두 개의 외래 키 제약 조건을 추가하는 SQL 스크립트입니다. 다음은 코드 리뷰 및 개선 사항입니다:
결론적으로 코드 구조나 명명 규칙은 적절하나, 비즈니스 로직과 성능 고려를 위해 추가적인 검토가 필요합니다. 에러 처리 및 유지 보수성을 높이기 위한 개선이 필요합니다. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -401,4 +401,62 @@ export class ReviewsRepository { | |
}, | ||
}); | ||
} | ||
|
||
// Fetch humanity reviews (HSS department) | ||
async getHumanityReviews(): Promise<EReview.WithLectures[]> { | ||
return this.prisma.review_review.findMany({ | ||
where: { | ||
course: { | ||
subject_department: { | ||
code: 'HSS', | ||
}, | ||
}, | ||
}, | ||
include: { | ||
lecture: true, | ||
}, | ||
}); | ||
} | ||
|
||
// Fetch major reviews (Non-HSS department) | ||
async getMajorReviews(): Promise<EReview.WithLectures[]> { | ||
return this.prisma.review_review.findMany({ | ||
where: { | ||
NOT: { | ||
course: { | ||
subject_department: { | ||
code: 'HSS', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이거 기필도 다 포함되어서 subject_department 긁어와서 in으로 넣는게 낫지 않을까 싶긴합니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 그리고 EReview.withLectures 만드셨으니까 include에 저렇게 lecture:true를 쓰시는게 아니라 include: EReview.withLectures.include 이렇게 하시면 됩니다 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. subject_department 테이블 보니까 전공만 따로 뽑는게 없는 거 같더라구요... 또 그냥 학과들만 넣자니 부전공 복수전공도 띄우는 것 같아서 일단은 visible 정도만 추가하겠습니다 |
||
}, | ||
}, | ||
}, | ||
}, | ||
include: { | ||
lecture: true, | ||
}, | ||
}); | ||
} | ||
|
||
// Clear all humanity best reviews | ||
async clearHumanityBestReviews() { | ||
return this.prisma.review_humanitybestreview.deleteMany(); | ||
} | ||
|
||
// Clear all major best reviews | ||
async clearMajorBestReviews() { | ||
return this.prisma.review_majorbestreview.deleteMany(); | ||
} | ||
|
||
// Add new humanity best reviews | ||
async addHumanityBestReviews(reviews: { reviewId: number }[]) { | ||
return this.prisma.review_humanitybestreview.createMany({ | ||
data: reviews.map((review) => ({ review_id: review.reviewId })), | ||
}); | ||
} | ||
|
||
// Add new major best reviews | ||
async addMajorBestReviews(reviews: { reviewId: number }[]) { | ||
return this.prisma.review_majorbestreview.createMany({ | ||
data: reviews.map((review) => ({ review_id: review.reviewId })), | ||
}); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 코드 패치에 대한 간단한 리뷰를 제공하겠습니다. 코드 리뷰
버그 위험
이와 같은 피드백을 바탕으로 코드를 보강하고 안정성을 높일 수 있을 것입니다. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -216,10 +216,16 @@ model main_reviewwritedailyuserfeed { | |
|
||
model review_humanitybestreview { | ||
review_id Int @id | ||
review review_review @relation(fields: [review_id], references: [id], onDelete: Cascade) | ||
|
||
@@index([review_id]) | ||
} | ||
|
||
model review_majorbestreview { | ||
review_id Int @id | ||
review_id Int @id | ||
review review_review @relation(fields: [review_id], references: [id], onDelete: Cascade) | ||
|
||
@@index([review_id]) | ||
} | ||
|
||
model review_review { | ||
|
@@ -242,6 +248,9 @@ model review_review { | |
lecture subject_lecture @relation(fields: [lecture_id], references: [id]) | ||
writer session_userprofile? @relation(fields: [writer_id], references: [id]) | ||
review_reviewvote review_reviewvote[] | ||
review_majorbestreview review_majorbestreview? @relation | ||
review_humanitybestreview review_humanitybestreview? @relation | ||
|
||
|
||
@@unique([writer_id, lecture_id], map: "review_comment_writer_id_af700a5d_uniq") | ||
@@index([written_datetime], map: "review_comment_e5e30a4a") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 코드 패치에 대한 리뷰를 진행하겠습니다.
종합적으로 보았을 때, 전반적으로 잘 작성된 코드이나, 몇 가지 개선 사항과 주의해야 할 점이 있습니다. 이러한 사항들을 고려하여 더 안정적이고 효율적인 코드를 작성하길 바랍니다. |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 패치에 대한 간단한 리뷰를 제공하겠습니다.
코드 가독성: 새로운
WithLectures
상수와 타입이 추가되었습니다. 전반적으로 코드가 잘 정리되어 있어 가독성이 좋습니다. 하지만 각 프로퍼티의 역할이나 사용 목적에 대한 주석을 추가하면 다른 개발자들이 이해하는 데 도움이 될 것입니다.정의 및 사용 장소:
WithLectures
상수와 타입이EReview
네임스페이스에 잘 포함되어 있지만, 전체적인 구조에서 이 상수가 어디에서 사용될지 명확하지 않습니다. 만약 이 상수가 다른 모듈 또는 네임스페이스에서 사용될 가능성이 있다면, 예시나 문서를 추가해주는 것이 좋습니다.버그 리스크: 현재 코드에는 명백한 버그가 있는 것 같지는 않습니다. 하지만
ELecture.Basic
이 유효한지 확인해야 합니다. 만약Basic
이 정의되지 않은 상태에서 접근하게 되면 런타임 오류가 발생할 수 있습니다.타입 안정성:
WithLectures
의 타입이Prisma.review_reviewGetPayload
를 기반으로 하고 있으므로 타입 안정성은 잘 유지되고 있는 것 같습니다. 그러나 언제든지 Prisma의 API가 변경될 수 있으니, 이러한 의존성에 유의해야 합니다.테스트: 이 새로운 추가 기능에 대해 테스트가 필요한지 검토해야 합니다.
WithLectures
와 관련된 단위 테스트가 없다면, 나중에 버그가 발생할 가능성을 높일 수 있습니다.추가적인 개선: 자주 사용하는 패턴에 대한 헬퍼 함수나 유틸리티를 도입하면 코드의 재사용성과 유지보수성이 향상될 수 있습니다.
전반적으로 코드는 명확하고 잘 작성되어 있지만, 위에 언급한 몇 가지 사항을 고려하면 코드의 품질과 안정성을 더 높일 수 있을 것입니다.