-
Notifications
You must be signed in to change notification settings - Fork 0
FEAT: 문제 풀이 상태 관리 API #26
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
base: dev
Are you sure you want to change the base?
Conversation
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.
Code Review
이번 PR은 문제 풀이 상태 관리 API를 추가하는 중요한 기능 개발이네요. 전반적으로 서비스 레이어를 도입하여 비즈니스 로직을 분리하고, 다양한 조회 옵션(개인/그룹/멤버)을 제공하는 등 구조를 잘 설계하셨습니다. 몇 가지 개선점을 제안드리며, 특히 보안 및 데이터 정확성과 관련된 부분은 꼭 확인해 주시면 좋겠습니다. 코드 리뷰를 통해 더 완성도 높은 코드가 되기를 바랍니다.
| elif view == "member": | ||
| if member_id is None: | ||
| raise ValueError("view='member'일 때 member_id가 필요합니다.") | ||
| return self._get_member_statistics(study, member_id, start_date, end_date) |
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.
_get_member_statistics 메서드에서 member_id로 사용자를 조회할 때, 해당 사용자가 현재 study_id에 속한 멤버인지 확인하는 로직이 필요합니다. 현재 코드는 요청하는 사용자가 스터디 멤버인지만 확인하고 있어, 다른 스터디 멤버가 임의의 member_id를 사용하여 시스템의 다른 사용자 통계를 조회할 수 있는 잠재적인 데이터 노출 위험이 있습니다. StudyMember 모델을 통해 study와 member_id의 관계를 확인하는 검증 로직을 추가해야 합니다.
예시:
# study 객체를 가져온 후
if not StudyMember.objects.filter(study=study, user_id=member_id).exists():
raise PermissionDenied("해당 멤버는 스터디에 속해있지 않습니다.")References
- 권한 체크는 뷰 레벨과 모델 레벨에서 모두 수행해야 합니다. 이 의견은 서비스 레이어에서 데이터 접근 권한을 더 엄격하게 제어해야 한다는 점에서 이 규칙과 관련이 있습니다. (link)
| user = models.ForeignKey( | ||
| "User", | ||
| on_delete=models.CASCADE, | ||
| related_name="solving_statuses", | ||
| verbose_name="사용자", | ||
| ) |
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.
User 모델에 대한 ForeignKey를 정의할 때 Django의 권장사항에 따라 settings.AUTH_USER_MODEL을 사용하는 것이 좋습니다. 이렇게 하면 User 모델이 변경되거나 교체될 경우에 더 유연하게 대처할 수 있습니다. 현재 코드도 동작하지만, 일관성과 유지보수성을 위해 변경을 제안합니다.
변경을 위해서는 파일 상단에 from django.conf import settings를 추가해야 합니다.
| user = models.ForeignKey( | |
| "User", | |
| on_delete=models.CASCADE, | |
| related_name="solving_statuses", | |
| verbose_name="사용자", | |
| ) | |
| user = models.ForeignKey( | |
| settings.AUTH_USER_MODEL, | |
| on_delete=models.CASCADE, | |
| related_name="solving_statuses", | |
| verbose_name="사용자", | |
| ) |
| statuses = ProblemSolvingStatus.objects.filter( | ||
| assignment__in=assignments, user=user | ||
| ).select_related("assignment", "assignment__problem") |
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.
select_related에 중복된 관계가 포함되어 있습니다. select_related("assignment", "assignment__problem")은 select_related("assignment__problem")으로 최적화할 수 있습니다. 후자는 assignment와 assignment를 통해 연결된 problem을 모두 한번에 가져오므로 더 효율적입니다.
| statuses = ProblemSolvingStatus.objects.filter( | |
| assignment__in=assignments, user=user | |
| ).select_related("assignment", "assignment__problem") | |
| statuses = ProblemSolvingStatus.objects.filter( | |
| assignment__in=assignments, user=user | |
| ).select_related("assignment__problem") |
References
- N+1 쿼리 문제를 방지하기 위해
select_related()와prefetch_related()를 적절히 사용해야 합니다. 이 제안은select_related를 더 효율적으로 사용하는 방법에 대한 것입니다. (link)
| statuses = ProblemSolvingStatus.objects.filter( | ||
| assignment__in=assignments | ||
| ).select_related("assignment", "assignment__problem", "user") |
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.
select_related에 중복된 관계가 포함되어 있습니다. select_related("assignment", "assignment__problem", "user")는 select_related("assignment__problem", "user")으로 최적화할 수 있습니다. assignment__problem은 assignment와 problem을 모두 포함하므로, assignment를 별도로 지정할 필요가 없습니다.
| statuses = ProblemSolvingStatus.objects.filter( | |
| assignment__in=assignments | |
| ).select_related("assignment", "assignment__problem", "user") | |
| statuses = ProblemSolvingStatus.objects.filter( | |
| assignment__in=assignments | |
| ).select_related("assignment__problem", "user") |
References
- N+1 쿼리 문제를 방지하기 위해
select_related()와prefetch_related()를 적절히 사용해야 합니다. 이 제안은select_related를 더 효율적으로 사용하는 방법에 대한 것입니다. (link)
| """ | ||
| # 스터디 및 문제 조회 | ||
| study = Study.objects.get(id=study_id) | ||
| from core.models import Problem |
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.
| average_problem_rate = ( | ||
| sum(completion_rates) / len(completion_rates) | ||
| if completion_rates | ||
| else 0.0 | ||
| ) | ||
| average_note_rate = ( | ||
| sum( | ||
| [ | ||
| m["note_completion_rate"] | ||
| for m in members_statistics | ||
| if m["total_assigned"] > 0 | ||
| ] | ||
| ) | ||
| / len([m for m in members_statistics if m["total_assigned"] > 0]) | ||
| if members_statistics | ||
| else 0.0 | ||
| ) |
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.
average_problem_completion_rate와 average_note_completion_rate를 계산할 때, 각 멤버의 완료율을 산술 평균하고 있습니다. 이는 멤버별 과제 수가 다를 경우 통계를 왜곡할 수 있습니다 (심슨의 역설). 전체 그룹의 정확한 평균 완료율을 계산하려면, 모든 멤버의 총 완료 수를 모든 멤버의 총 할당 수로 나누는 것이 더 정확합니다. overall_problem_counts와 overall_note_counts, 그리고 total_assigned를 사용하여 계산하는 것을 권장합니다. 이 경우 completion_rates 리스트는 더 이상 필요하지 않습니다.
average_problem_rate = (
overall_problem_counts["completed_count"] / total_assigned
if total_assigned > 0
else 0.0
)
average_note_rate = (
overall_note_counts["completed_count"] / total_assigned
if total_assigned > 0
else 0.0
)| def _get_member_statistics( | ||
| self, study, member_id: int, start_date: Optional[date], end_date: Optional[date] | ||
| ) -> dict: | ||
| """특정 멤버의 통계 조회""" | ||
| from core.models import User | ||
|
|
||
| member = User.objects.get(id=member_id) | ||
|
|
||
| # 날짜 범위 필터링 | ||
| assignments_query = DailyAssignment.objects.filter(study=study) | ||
| if start_date: | ||
| assignments_query = assignments_query.filter( | ||
| assigned_date__gte=start_date | ||
| ) | ||
| if end_date: | ||
| assignments_query = assignments_query.filter(assigned_date__lte=end_date) | ||
|
|
||
| assignments = assignments_query.select_related("problem") | ||
|
|
||
| # 멤버의 ProblemSolvingStatus 조회 | ||
| statuses = ProblemSolvingStatus.objects.filter( | ||
| assignment__in=assignments, user=member | ||
| ).select_related("assignment", "assignment__problem") | ||
|
|
||
| # 멤버의 SolutionNote 조회 | ||
| problems = [a.problem for a in assignments] | ||
| solution_notes = SolutionNote.objects.filter( | ||
| study=study, user=member, problem__in=problems | ||
| ).select_related("problem") | ||
|
|
||
| # status와 note를 매핑 | ||
| status_dict = {status.assignment_id: status for status in statuses} | ||
| note_dict = {note.problem_id: note for note in solution_notes} | ||
|
|
||
| # 전체 통계 계산 | ||
| total_assigned = len(assignments) | ||
| problem_status_counts = { | ||
| "not_attempted_count": 0, | ||
| "in_progress_count": 0, | ||
| "completed_count": 0, | ||
| } | ||
| note_status_counts = {"not_completed_count": 0, "completed_count": 0} | ||
|
|
||
| for assignment in assignments: | ||
| status = status_dict.get(assignment.id) | ||
| note = note_dict.get(assignment.problem_id) | ||
|
|
||
| problem_status = ( | ||
| status.status if status else ProblemStatus.NOT_ATTEMPTED | ||
| ) | ||
| note_status = "completed" if note else "not_completed" | ||
|
|
||
| if problem_status == ProblemStatus.NOT_ATTEMPTED: | ||
| problem_status_counts["not_attempted_count"] += 1 | ||
| elif problem_status == ProblemStatus.IN_PROGRESS: | ||
| problem_status_counts["in_progress_count"] += 1 | ||
| elif problem_status == ProblemStatus.COMPLETED: | ||
| problem_status_counts["completed_count"] += 1 | ||
|
|
||
| if note_status == "not_completed": | ||
| note_status_counts["not_completed_count"] += 1 | ||
| else: | ||
| note_status_counts["completed_count"] += 1 | ||
|
|
||
| # 완료율 계산 | ||
| completed_count = problem_status_counts["completed_count"] | ||
| problem_completion_rate = ( | ||
| completed_count / total_assigned if total_assigned > 0 else 0.0 | ||
| ) | ||
| note_completed_count = note_status_counts["completed_count"] | ||
| note_completion_rate = ( | ||
| note_completed_count / total_assigned if total_assigned > 0 else 0.0 | ||
| ) | ||
|
|
||
| # 일별 통계 계산 | ||
| daily_statistics = self._calculate_daily_statistics( | ||
| member, study, assignments, status_dict, note_dict, start_date, end_date | ||
| ) | ||
|
|
||
| return { | ||
| "view": "member", | ||
| "member_id": member.id, | ||
| "member_email": member.email, | ||
| "username": member.boj_username, | ||
| "total_assigned": total_assigned, | ||
| "problem_status_summary": problem_status_counts, | ||
| "note_status_summary": note_status_counts, | ||
| "problem_completion_rate": problem_completion_rate, | ||
| "note_completion_rate": note_completion_rate, | ||
| "date_range": { | ||
| "start_date": start_date.isoformat() if start_date else None, | ||
| "end_date": end_date.isoformat() if end_date else None, | ||
| }, | ||
| "daily_statistics": daily_statistics, | ||
| } |
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.
_get_my_statistics와 _get_member_statistics 메서드는 거의 동일한 로직을 공유하고 있습니다. 코드 중복을 줄이고 유지보수성을 높이기 위해, 사용자 객체를 인자로 받아 통계를 계산하는 공통 헬퍼 메서드로 추출하는 것을 고려해 보세요.
References
- 복잡한 로직/상호작용을 전용 서비스/유틸리티로 추상화하여 코드의 명확성과 재사용성을 높여야 합니다. 이 제안은 중복된 통계 계산 로직을 별도의 헬퍼 함수로 분리하여 이 원칙을 따르도록 합니다. (link)
| except requests.RequestException: | ||
| # API 호출 실패 시 모든 문제를 풀지 않은 것으로 처리 | ||
| return {pid: False for pid in problem_ids} |
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.
requests.RequestException 발생 시 API 호출 실패를 인지하고 모든 문제를 '풀지 않음'으로 처리하는 것은 좋은 방어적 프로그래밍입니다. 하지만 디버깅 및 모니터링을 위해 예외가 발생했을 때 로그를 남기는 것이 좋습니다. logging 모듈을 사용하여 에러를 기록하면 나중에 어떤 문제가 발생했는지 추적하기 용이합니다.
파일 상단에 import logging과 logger = logging.getLogger(__name__)를 추가하고, except 블록에서 logger.error()를 호출하는 것을 권장합니다.
| except requests.RequestException: | |
| # API 호출 실패 시 모든 문제를 풀지 않은 것으로 처리 | |
| return {pid: False for pid in problem_ids} | |
| except requests.RequestException as e: | |
| logger.error(f"solved.ac API 호출에 실패했습니다 (user: {username}): {e}") | |
| # API 호출 실패 시 모든 문제를 풀지 않은 것으로 처리 | |
| return {pid: False for pid in problem_ids} |
- 오늘의 추천 문제 조회 리스폰스를 problemId 순으로 정렬하도록 수정
- 풀이 상태 조회 리스폰스에서 마지막 업데이트 시각이 없다면 오늘 정각을 주는 것으로 변경
🚀 개요 (Overview)
[ resolves #25 ]
🔎 변경사항 (Changes)
모델 및 마이그레이션
core/models.py:ProblemSolvingStatus모델 추가ProblemStatusTextChoices 추가 (NOT_ATTEMPTED, IN_PROGRESS, COMPLETED)core/migrations/0009_alter_study_template_content_problemsolvingstatus.py: 마이그레이션 파일 생성서비스 레이어
core/problem_solving_status/services.py:ProblemSolvingStatusService클래스 구현update_solving_status(): 백준 API를 통한 문제 풀이 상태 자동 업데이트get_solving_statuses(): 문제 풀이 상태 조회 (me/group 뷰)get_problem_members_status(): 특정 문제의 모든 멤버 상태 조회get_statistics(): 통계 조회 (me/group/member 뷰)백준 API 연동
core/utils/solvedac.py:check_user_solved_problems()메서드 추가API 엔드포인트
POST /studies/<study_id>/assignments/update/: 문제 풀이 상태 업데이트GET /studies/<study_id>/assignments/status/: 문제 풀이 상태 조회 (date, view 파라미터)GET /studies/<study_id>/problems/<problem_id>/status/members/: 특정 문제의 모든 멤버 상태 조회GET /studies/<study_id>/statistics/: 통계 조회 (view, member_id, start_date, end_date 파라미터)Serializers
core/problem_solving_status/serializers.py: 모든 API의 요청/응답 Serializer 구현Views
core/problem_solving_status/views.py: 4개 API View 구현ProblemStatusUpdateViewProblemStatusViewProblemMembersStatusViewStatisticsViewURL 설정
core/problem_solving_status/urls.py: URL 패턴 정의core/urls.py: problem_solving_status URLs 포함