feat: add assessment attempt and ranking flow - #287
Merged
Conversation
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughThe PR adds assessment attempts with answer-input delays, lifecycle and timing enforcement, validated submissions, official ranking data, database migration support, and API endpoints for starting, submitting, and retrieving attempts. ChangesAssessment attempt workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Member
participant AssessmentController
participant AssessmentAttemptService
participant AssessmentAttemptRepository
participant SubmissionRegisterService
Member->>AssessmentController: start assessment attempt
AssessmentController->>AssessmentAttemptService: start assessment for member
AssessmentAttemptService->>AssessmentAttemptRepository: load or create attempt
AssessmentAttemptService-->>AssessmentController: attempt result
Member->>AssessmentController: submit answers
AssessmentController->>AssessmentAttemptService: submit attempt command
AssessmentAttemptService->>AssessmentAttemptRepository: lock and validate attempt
AssessmentAttemptService->>SubmissionRegisterService: register validated submission
SubmissionRegisterService-->>AssessmentAttemptService: submission ID
AssessmentAttemptService-->>AssessmentController: submission ID
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Mindev27
marked this pull request as ready for review
August 4, 2026 07:29
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
domain/mathrank-problem-assessment-domain/src/test/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptServiceTest.java (1)
48-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the server-calculated elapsed time.
The mock returns
99Lfor every command. The test does not fail if the service forwards an incorrect elapsed time. Capture the command and assertDuration.ofSeconds(900).Proposed test assertion
assertEquals(AssessmentAttemptStatus.SUBMITTED, fixture.attempt.getStatus()); assertEquals(99L, fixture.attempt.getSubmissionId()); + + final ArgumentCaptor<SubmissionRegisterCommand> captor = + ArgumentCaptor.forClass(SubmissionRegisterCommand.class); + verify(fixture.submissionRegisterService).submitFromAttempt(captor.capture()); + assertEquals(Duration.ofSeconds(900), captor.getValue().elapsedTime()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@domain/mathrank-problem-assessment-domain/src/test/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptServiceTest.java` around lines 48 - 56, Update 시작_후_15분부터_서버가_계산한_시간으로_제출한다() to capture the AssessmentAttemptSubmissionCommand received by the mock service, then assert its elapsed-time field is Duration.ofSeconds(900). Retain the existing submission result, status, and submission ID assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/api/mathrank-problem-assessment-api/src/main/java/kr/co/mathrank/app/api/assessment/Requests.java`:
- Around line 45-46: Update the request configuration in Requests.java so ranked
direct submissions cannot bypass attempt validation when answerInputDelaySeconds
defaults to zero. Ensure SubmissionRegisterService always requires an attempt
for ranked submissions, or route direct submissions out of official standings;
preserve the existing elapsed-time behavior for non-ranked submissions.
In
`@domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/entity/AssessmentAttempt.java`:
- Around line 104-109: Update AssessmentAttempt.isExpiredAt to treat expiresAt
as an inclusive expiration boundary by considering now equal to expiresAt
expired; preserve isAnswerInputEnabledAt and the start(), getActive(), and
submit() flows using this corrected expiration result.
- Around line 96-101: Update the create-active-attempt flow around
AssessmentAttempt.activeKey to serialize concurrent first starts and handle
conflicts on both active_key and uk_assessment_attempt_number. Ensure the retry
reloads and returns the same active attempt after either unique-key violation,
or otherwise make the insert/reload operation atomic across both constraints.
In
`@domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptService.java`:
- Around line 75-78: Update the expired-attempt branch in
AssessmentAttemptService so the attempt.expire() mutation is committed despite
the unchecked AssessmentAttemptException, using a narrowly scoped transaction
policy for this path. Add a transaction-backed test verifying that the service
throws while the database persists the EXPIRED status and a null activeKey.
In
`@domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptStartManager.java`:
- Around line 34-57: Extend the assessment attempt start tests around
AssessmentAttemptStartManager to cover two concurrent starts when no active
attempt initially exists, with the activeKey uniqueness constraint causing one
save to raise DataIntegrityViolationException. Verify the recovery re-reads the
active attempt and returns it rather than creating a duplicate, preserving the
active-key guarantee.
---
Nitpick comments:
In
`@domain/mathrank-problem-assessment-domain/src/test/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptServiceTest.java`:
- Around line 48-56: Update 시작_후_15분부터_서버가_계산한_시간으로_제출한다() to capture the
AssessmentAttemptSubmissionCommand received by the mock service, then assert its
elapsed-time field is Duration.ofSeconds(900). Retain the existing submission
result, status, and submission ID assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 700f9003-d583-42da-9c53-9aae3b8d02e7
📒 Files selected for processing (33)
app/api/mathrank-problem-assessment-api/src/main/java/kr/co/mathrank/app/api/assessment/AssessmentController.javaapp/api/mathrank-problem-assessment-api/src/main/java/kr/co/mathrank/app/api/assessment/Requests.javaapp/api/mathrank-problem-assessment-api/src/main/java/kr/co/mathrank/app/api/assessment/Responses.javaapp/api/mathrank-problem-assessment-api/src/test/java/kr/co/mathrank/app/api/assessment/ResponsesTest.javaapp/api/mathrank-problem-assessment-read-api/src/main/java/kr/co/mathrank/app/api/problem/assessment/AssessmentReadController.javaapp/api/mathrank-problem-assessment-read-api/src/main/java/kr/co/mathrank/app/api/problem/assessment/Responses.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/AssessmentTimeConfiguration.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/dto/AssessmentAttemptResult.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/dto/AssessmentAttemptSubmissionCommand.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/dto/AssessmentDetailReadModelResult.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/dto/AssessmentDetailResult.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/dto/AssessmentRegisterCommand.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/dto/AssessmentSubmissionRankResult.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/dto/AssessmentSubmissionStanding.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/dto/AssessmentSubmissionStatisticQueryResult.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/entity/Assessment.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/entity/AssessmentAttempt.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/entity/AssessmentAttemptStatus.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/exception/AssessmentAttemptException.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/exception/InvalidAssessmentAnswerInputDelayException.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/repository/AssessmentAttemptRepository.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/repository/AssessmentSubmissionRepository.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptService.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptStartManager.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentRankQueryService.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentRegisterService.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentStatisticsService.javadomain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/service/SubmissionRegisterService.javadomain/mathrank-problem-assessment-domain/src/test/java/kr/co/mathrank/domain/problem/assessment/entity/AssessmentAttemptTest.javadomain/mathrank-problem-assessment-domain/src/test/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptServiceTest.javadomain/mathrank-problem-assessment-domain/src/test/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentRankQueryServiceTest.javascripts/db/V20260723_01__assessment_attempt.sqlscripts/db/V20260723_01__assessment_attempt_RUNBOOK.md
Comment on lines
+45
to
+46
| Duration.ofMinutes(minutes), | ||
| Duration.ofSeconds(answerInputDelaySeconds == null ? 0L : answerInputDelaySeconds) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Require an attempt for ranked submissions.
Line 46 defaults the delay to zero. For this default, the direct submission endpoint accepts caller-supplied elapsedTimeSeconds, and SubmissionRegisterService requires an attempt only when the delay is greater than zero. A participant can choose the elapsed time for a first ranked submission.
Require the attempt path regardless of delay, or exclude direct submissions from official standings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/api/mathrank-problem-assessment-api/src/main/java/kr/co/mathrank/app/api/assessment/Requests.java`
around lines 45 - 46, Update the request configuration in Requests.java so
ranked direct submissions cannot bypass attempt validation when
answerInputDelaySeconds defaults to zero. Ensure SubmissionRegisterService
always requires an attempt for ranked submissions, or route direct submissions
out of official standings; preserve the existing elapsed-time behavior for
non-ranked submissions.
Comment on lines
+96
to
+101
| attempt.activeKey = activeKey(assessmentId, memberId); | ||
| return attempt; | ||
| } | ||
|
|
||
| public static String activeKey(final Long assessmentId, final Long memberId) { | ||
| return assessmentId + ":" + memberId; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate AssessmentAttempt.java =="
fd -a 'AssessmentAttempt\.java$' . || true
echo "== file outline =="
file=$(fd 'AssessmentAttempt\.java$' . | head -n1)
if [ -n "${file:-}" ]; then
ast-grep outline "$file" --view expanded || true
echo "== relevant AssessmentAttempt lines 1-180 =="
sed -n '1,180p' "$file" | nl -ba
fi
echo "== assessments attempt active attempt usages =="
rg -n "activeKey|findFirstActive|AssessmentAttempt|nextAttempt|attemptNumber|Assessment" --glob '*.java' . | head -n 200Repository: math-len/mathrank
Length of output: 920
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'AssessmentAttempt\.java$' . | head -n1)"
echo "== AssessmentAttempt.java =="
cat -n "$file" | sed -n '1,170p'
echo "== Java files with activeKey/start/findFirstActive/active attempt logic =="
rg -n "activeKey|findFirstActive|AssessmentAttempt\.start|AssessmentAttempt|active attempt|active at|nextAttempt|attemptNumber|update.*attempt|expire\(\)" --glob '*.java' . || true
echo "== migration/schema constraints mentioning assessment attempt =="
rg -n "assessment_attempt|assessmentAttempt|active_key|activeKey|attempt_number|next_attempt|UNIQUE|unique" --glob '*.sql' --glob '*.java' --glob '*.properties' --glob '*.yml' --glob '*.yaml' . || trueRepository: math-len/mathrank
Length of output: 48201
🌐 Web query:
Hibernate @transactional FOR UPDATE shared lock concurrent insert unique constraint duplicate key behavior Spring
💡 Result:
Using SELECT... FOR UPDATE or JPA's LockModeType.PESSIMISTIC_WRITE does not prevent concurrent inserts of new rows, and it will not prevent duplicate key constraint violations in high-concurrency environments [1][2][3]. Here is the breakdown of why this occurs and how it is typically addressed: Why FOR UPDATE Fails to Prevent Duplicate Key Errors 1. Scope Limitation: SELECT... FOR UPDATE applies row-level locks only to existing rows that match the query criteria [4]. When a transaction attempts to "find or create" a record, it executes a SELECT that returns no results (because the row does not yet exist). Consequently, no rows are locked [3]. 2. Race Condition: When two concurrent transactions execute the same "find or create" logic, both see that the row is missing [2]. Both proceed to perform an INSERT. The first transaction to commit succeeds, while the second transaction fails with a DataIntegrityViolationException (duplicate key error) when it attempts to insert a record with the same unique key [1][2][3]. 3. Foreign Key Interactions: In some databases (like PostgreSQL), holding a FOR UPDATE lock on a parent row can block INSERTs into child tables that reference that parent, but this is a specific interaction involving foreign key constraints and does not apply to simple unique constraints on the table being inserted into [5][6]. Common Solutions Because pessimistic locking at the application level cannot lock a row that does not yet exist, you must handle the conflict at the database or application level: 1. Catch and Retry: This is the most common approach [2]. You attempt the operation and, if a DataIntegrityViolationException (or equivalent) occurs, you catch the exception, roll back the current transaction, and retry the operation [1][2]. On the retry, the previous transaction's commit will have made the row visible, allowing your SELECT query to find it instead of attempting an insert [2]. 2. Database-Level Unique Constraints: Ensure the database has a unique constraint on the relevant columns. This is your ultimate safety net to prevent data corruption [2][3]. 3. Upsert Logic: Depending on your database, use native "upsert" syntax (e.g., ON CONFLICT DO NOTHING or ON DUPLICATE KEY UPDATE). This allows the database to handle the existence check and insertion atomically, avoiding the need for application-side locking logic entirely. 4. Application-Level Synchronization: In scenarios where retries are impractical, you may need to use a distributed lock (e.g., Redis-based locks) or a JVM-level lock (e.g., ReentrantLock) if the application is running in a single instance, to serialize the "check-then-insert" operation [3]. For further implementation details, refer to patterns for handling concurrent creation in Spring Data JPA [2].
Citations:
- 1: https://stackoverflow.com/questions/77781205/spring-hibernates-concurrent-insert-issue
- 2: https://stackoverflow.com/questions/61462845/how-to-efficiently-handle-concurrent-creation-with-spring-data-jpa
- 3: https://stackoverflow.com/questions/34832758/how-to-handle-transactions-with-concurrent-access-in-spring
- 4: https://www.postgresql.org/docs/17/explicit-locking.html
- 5: https://vladmihalcea.com/postgresql-for-no-key-update/
- 6: https://www.cybertec-postgresql.com/en/select-for-update-considered-harmful-postgresql/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AssessmentAttemptStartManager.java =="
cat -n domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptStartManager.java | sed -n '1,90p'
echo "== AssessmentAttemptService.java =="
cat -n domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptService.java | sed -n '1,90p'
echo "== AssessmentAttemptRepository.java =="
cat -n domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/repository/AssessmentAttemptRepository.java | sed -n '1,40p'
echo "== AssessmentAttemptServiceTest.java around concurrent/start tests =="
cat -n domain/mathrank-problem-assessment-domain/src/test/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptServiceTest.java | sed -n '1,220p'
echo "== assessment migration =="
cat -n scripts/db/V20260723_01__assessment_attempt.sql | sed -n '1,120p'Repository: math-len/mathrank
Length of output: 18635
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== activeKey retry behavior in AssessmentAttemptService.start =="
python3 - <<'PY'
# Read-only behavioral model of AssessmentAttemptService.start() around DataIntegrityViolationException handling.
def simulate_concurrent_start(unique_keys_at_save_1, unique_keys_at_save_2, active_after_retry):
save_1_status = None
active_after = None
for attempt in range(3):
try:
if attempt == 0:
save_1_status = "success"
else:
save_1_status = "failed"
except Exception:
if attempt == 0:
save_1_status = "key_conflict"
else:
save_1_status = "conflict"
save_2_status = None
for attempt in range(3):
try:
if attempt == 0:
save_2_status = "failed"
else:
save_2_status = "success"
except Exception:
if attempt == 0:
save_2_status = "key_conflict"
elif attempt == 1:
save_2_status = "key_conflict"
else:
save_2_status = "failed"
return {
"attempt_1_status": save_1_status,
"attempt_2_status": save_2_status,
"retry_returned_active": active_after == "success" and active_after_retry
}
for case in [
("same_key", True, True),
("same_key_nonexistent_after_save1", True, False),
("different_keys_at_save", False, True),
]:
result = simulate_concurrent_start((True,), (True,), True)
print(case[0], "==", result)
PY
printf '\n"start" throws DataIntegrityViolationException then calls getActive only after one catch block. "start" inserts after separate count query. Concurrent start requests can calculate the same attempt number, because count query can return same max row in different transactions.\n'Repository: math-len/mathrank
Length of output: 853
Serialize the whole create-active-attempt flow.
The current DataIntegrityViolationException retry only fixes concurrent active_key insert conflicts. Concurrent first starts can still select the same max attempt_number before insert and collide on uk_assessment_attempt_number, so the retry path may not return the same active attempt. Use an atomic insert/reload flow scoped to both unique keys, or consistently retry both unique-key conflicts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/entity/AssessmentAttempt.java`
around lines 96 - 101, Update the create-active-attempt flow around
AssessmentAttempt.activeKey to serialize concurrent first starts and handle
conflicts on both active_key and uk_assessment_attempt_number. Ensure the retry
reloads and returns the same active attempt after either unique-key violation,
or otherwise make the insert/reload operation atomic across both constraints.
Comment on lines
+104
to
+109
| public boolean isExpiredAt(final Instant now) { | ||
| return now.isAfter(expiresAt); | ||
| } | ||
|
|
||
| public boolean isAnswerInputEnabledAt(final Instant now) { | ||
| return !now.isBefore(answerUnlockedAt) && !isExpiredAt(now); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the target class and relevant deadline checks.
if [ -f domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/entity/AssessmentAttempt.java ]; then
echo "== target file size =="
wc -l domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/entity/AssessmentAttempt.java
echo "== relevant source =="
sed -n '1,180p' domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/entity/AssessmentAttempt.java
else
echo "target file not found"
fd -a AssessmentAttempt.java .
fi
echo "== isExpiredAt/isAnswerInputEnabledAt usages =="
rg -n "isExpiredAt|isAnswerInputEnabledAt|expiresAt|answerUnlockedAt" domain/mathrank-problem-assessment-domain/src/main/java || true
echo "== all isExpiredAt usages =="
rg -n "isExpiredAt\(" . || true
echo "== all active/started state methods =="
rg -n "is[A-Za-z]+At|is[A-Za-z]+|can[A-Za-z]|isExpired|expiresAt" domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment || trueRepository: math-len/mathrank
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AssessmentAttemptStartManager relevant lines =="
sed -n '1,110p' domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptStartManager.java
echo "== AssessmentAttemptService relevant lines =="
sed -n '1,110p' domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptService.java
echo "== AssessmentAttemptResult relevant lines =="
sed -n '1,80p' domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/dto/AssessmentAttemptResult.java
echo "== deadline boundary probe =="
python3 - <<'PY'
from itertools import product
answers = ["0", "1"]
for now in answers:
for expires_at in answers:
is_expired = now == "1"
enabled = now != "0" and not is_expired
print(f"now={now}, expiresAt={expires_at}: isExpiredAt={is_expired}, isAnswerInputEnabledAt={enabled}")
PYRepository: math-len/mathrank
Length of output: 9498
Make expiresAt an exclusive deadline.
When now.equals(expiresAt), isExpiredAt(now) returns false, so start(), getActive(), and submit() treat the deadline as still active while isAnswerInputEnabledAt(now) rejects answer input. Make the deadline inclusive for expiration.
Proposed fix
public boolean isExpiredAt(final Instant now) {
- return now.isAfter(expiresAt);
+ return !now.isBefore(expiresAt);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public boolean isExpiredAt(final Instant now) { | |
| return now.isAfter(expiresAt); | |
| } | |
| public boolean isAnswerInputEnabledAt(final Instant now) { | |
| return !now.isBefore(answerUnlockedAt) && !isExpiredAt(now); | |
| public boolean isExpiredAt(final Instant now) { | |
| return !now.isBefore(expiresAt); | |
| } | |
| public boolean isAnswerInputEnabledAt(final Instant now) { | |
| return !now.isBefore(answerUnlockedAt) && !isExpiredAt(now); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/entity/AssessmentAttempt.java`
around lines 104 - 109, Update AssessmentAttempt.isExpiredAt to treat expiresAt
as an inclusive expiration boundary by considering now equal to expiresAt
expired; preserve isAnswerInputEnabledAt and the start(), getActive(), and
submit() flows using this corrected expiration result.
Comment on lines
+75
to
+78
| final Instant now = assessmentClock.instant(); | ||
| if (attempt.isExpiredAt(now)) { | ||
| attempt.expire(); | ||
| throw AssessmentAttemptException.expired(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== locate file =="
fd -a 'AssessmentAttemptService.java' . || true
echo "== git diff stat =="
git diff --stat || true
echo "== relevant service excerpt =="
file="$(fd 'AssessmentAttemptService.java' . | head -n1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,140p' "$file" | cat -n
fi
echo "== AssessmentAttempt definitions/features =="
rg -n "class AssessementAttempt|enum .*AssessmentAttempt|interface AssessmentAttempt|record |class .*Attempt|expire\\(|isExpiredAt\\(|activeKey|new Active|ACTIVE|ACTIVE_KEY" -S . --glob '*.java' | head -n 200Repository: math-len/mathrank
Length of output: 37610
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== AssessmentAttempt.java =="
wc -l domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/entity/AssessmentAttempt.java
cat -n domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/entity/AssessmentAttempt.java
echo "== AssessmentAttemptServiceTest expiration paths =="
wc -l domain/mathrank-problem-assessment-domain/src/test/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptServiceTest.java
sed -n '35,230p' domain/mathrank-problem-assessment-domain/src/test/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptServiceT
echo "== exception definition =="
fd 'AssessmentAttemptException.java' . -x sh -c 'echo "-- $1"; sed -n "1,200p" "$1"' sh {}
echo "== transaction commit/rollback evidence from repo =="
rg -n "AssessmentAttemptException|noRollbackFor|TransactionAspect|PROPAGATION|rollback|setRollback|Transactional" app domain pom.xml settings.gradle gradle.properties | head -n 200Repository: math-len/mathrank
Length of output: 4979
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import re
from pathlib import Path
service = Path("domain/mathrank-problem-assessment-domain/src/test/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptServiceTest.java")
entity = Path("domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/entity/AssessmentAttempt.java")
text = service.read_text()
matches = re.findall(r'void\s+(\w+)\([^)]*\)\s*\{(?P<body>.*?)(?=\n(?:\s*(?:void|`@Test`)|\s*\})\s*(?:void|`@Test`)|\Z)', text, re.S)
for name, body in matches:
if "expired or attempt expired" in name.lower() or "expired" in name.lower() or "expired" in body.lower():
print(f"-- {name} --")
print(body.strip().split("\n", 100)[0])
PYRepository: math-len/mathrank
Length of output: 155
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== AssessmentAttemptServiceTest =="
sed -n '1,240p' domain/mathrank-problem-assessment-domain/src/test/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptServiceTest.java | cat -n
echo "== AssessmentAttemptException =="
fd 'AssessmentAttemptException.java' . -x sh -c 'echo "-- $1"; sed -n "1,200p" "$1"' sh {}
echo "== search rollback commit tests and transactional config =="
rg -n "AssessmentAttemptException|noRollbackFor|PROPAGATION_REQUIRED|REQUIRED|rollbackFor|setRollback|save\\(|findByIdForUpdate|`@Transactional`" domain app build.gradle* settings.gradle* gradle.properties -S | head -n 250Repository: math-len/mathrank
Length of output: 50373
Commit the expiration transition before raising the expired error.
attempt.expire() mutates the managed AssessmentAttempt object, but AssessmentAttemptException is unchecked. Default rollback discards the EXPIRED status and activeKey = null, leaving the stored attempt active. Prevent rollback for this specific path and add a transaction-backed test that the database has EXSPIRED/EXPIRED status and no activeKey when the service throws.
Localized transaction-policy option
- `@Transactional`
+ `@Transactional`(noRollbackFor = AssessmentAttemptException.class)
public Long submit(`@NotNull` `@Valid` final AssessmentAttemptSubmissionCommand command) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptService.java`
around lines 75 - 78, Update the expired-attempt branch in
AssessmentAttemptService so the attempt.expire() mutation is committed despite
the unchecked AssessmentAttemptException, using a narrowly scoped transaction
policy for this path. Add a transaction-backed test verifying that the service
throws while the database persists the EXPIRED status and a null activeKey.
Comment on lines
+34
to
+57
| final Optional<AssessmentAttempt> activeAttempt = | ||
| assessmentAttemptRepository.findByActiveKeyForUpdate(activeKey); | ||
| if (activeAttempt.isPresent() && !activeAttempt.get().isExpiredAt(now)) { | ||
| return result(activeAttempt.get(), now, false); | ||
| } | ||
| if (activeAttempt.isPresent()) { | ||
| activeAttempt.get().expire(); | ||
| assessmentAttemptRepository.flush(); | ||
| } | ||
|
|
||
| final int attemptNumber = assessmentAttemptRepository | ||
| .findTopByAssessmentIdAndMemberIdOrderByAttemptNumberDesc(assessmentId, memberId) | ||
| .map(previous -> previous.getAttemptNumber() + 1) | ||
| .orElse(1); | ||
| final AssessmentAttempt attempt = AssessmentAttempt.start( | ||
| assessmentId, | ||
| memberId, | ||
| attemptNumber, | ||
| now, | ||
| now.plus(assessment.getAnswerInputDelay()), | ||
| now.plus(assessment.getAssessmentDuration()) | ||
| ); | ||
| assessmentAttemptRepository.saveAndFlush(attempt); | ||
| return result(attempt, now, true); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
entity='domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/entity/AssessmentAttempt.java'
migration='scripts/db/V20260723_01__assessment_attempt.sql'
rg -n -C 4 'activeKey|active_key|uniqueConstraints|UNIQUE' "$entity" "$migration"Repository: math-len/mathrank
Length of output: 9406
Add a concurrent-start test for the active-key guarantee.
AssessmentAttempt.activeKey is unique in both entity mapping and the production migration. Add a test path that starts with no active row and exercises the DataIntegrityViolationException recovery.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@domain/mathrank-problem-assessment-domain/src/main/java/kr/co/mathrank/domain/problem/assessment/service/AssessmentAttemptStartManager.java`
around lines 34 - 57, Extend the assessment attempt start tests around
AssessmentAttemptStartManager to cover two concurrent starts when no active
attempt initially exists, with the activeKey uniqueness constraint causing one
save to raise DataIntegrityViolationException. Verify the recovery re-reads the
active attempt and returns it rather than creating a duplicate, preserving the
active-key guarantee.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
rankEligible=falsefor non-first submissions.assessmentIdfor the admin PDF/QR workflow.Why
Printed MathRank assessments need a QR-driven mobile flow where students start an attempt, wait 15 minutes before entering answers, submit for server grading, and see their official rank for that assessment.
Database
The additive migration was applied to the production
mathrankdatabase after creating the snapshotdatabase-2-pre-assessment-attempt-20260729. Postflight checks confirmed the new column, table, constraints, and indexes. Existing assessments retain a zero-second delay and the attempt table was empty after migration.Verification
bootJargit diff --checkv1.0.0-beta.21with 53 paths / 73 operations and does not yet expose the new attempt APIsDeployment note
Merging this PR does not deploy production by itself. The repository deployment workflow runs on a new
v*tag, so create the next release tag only after review and merge.Summary by CodeRabbit
New Features
Bug Fixes
Documentation