fix(server): resolve achievement system integrity issues and implement real evaluators#836
Open
FelixTJDietrich wants to merge 18 commits intomainfrom
Open
fix(server): resolve achievement system integrity issues and implement real evaluators#836FelixTJDietrich wants to merge 18 commits intomainfrom
FelixTJDietrich wants to merge 18 commits intomainfrom
Conversation
Fix 13 bugs discovered after merging the achievement system (#762): YAML/schema fixes (self-referencing parents, missing COMMIT_CREATED trigger, DummyEvaluator misconfiguration), service fixes (@transactional self-invocation, unconditional cache eviction, missing dirty-check), LazyInitializationException in async listener and EventContext, startup validation for evaluator classes and cycle detection. Adds 21 tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e events Replace all 14 DummyEvaluator placeholders with production-ready evaluators: - SpeedsterEvaluator: PR merged within 5 minutes of creation - ItsyBitsyEvaluator: commit with exactly 1 line changed - BruteForceEvaluator: 5 commits within a 5-minute window - AtomicChangesEvaluator: 10 consecutive small commits (≤3 lines, ≤2 files) - CrossBoundaryEvaluator: single commit touching 2+ languages - HiveMindEvaluator: issue closed with 10+ unique participants - OracleEvaluator: issue open for 6+ months before closing - NecromancerEvaluator: close own issue with zero comments from others - PolyglotEvaluator: commits across 3+ programming languages - NightOwlEvaluator: 5 commits between 01:00-05:00 UTC same day - LongTimeReturnEvaluator: return after 90+ days of inactivity - AllRarityEvaluator: collect all rare/epic/legendary tier achievements Add repository query methods to support evaluators and emit lifecycle events (PullRequestMerged, PullRequestClosed, IssueClosed) during sync for entities arriving in terminal state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dosubot
bot
added
bug
github-actions
bot
added
application-server
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces many DummyEvaluator entries with 12 concrete evaluators, adds time-window and paging repository queries, refactors achievement recalculation to TransactionTemplate with per-page transactions and explicit cache eviction, tightens registry validation, conditions lifecycle emissions in processors, and adds extensive unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Event as ActivityEvent
participant Listener as AchievementEventListener
participant Service as AchievementService
participant Evaluator as AchievementEvaluator
participant Repo as Repository
participant Cache as Cache
Event->>Listener: onApplicationEvent (AFTER_COMMIT, async)
Listener->>Service: checkAndUnlock(user, event)
Service->>Service: resolve triggered achievements
loop per achievement
Service->>Evaluator: updateProgress(userAchievement, event)
Evaluator->>Repo: query commits/issues/PRs/comments
Repo-->>Evaluator: return data
Evaluator-->>Service: progressUpdated (true/false)
alt progress changed or unlocked
Service->>Service: persist UserAchievement (transactional boundary)
Service->>Cache: evictAchievementCache(userId)
Cache-->>Service: evicted
end
end
Service-->>Listener: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the server-side achievement system by fixing configuration/service integrity problems, introducing production implementations for previously dummy evaluators, and ensuring lifecycle events are emitted during sync when entities arrive already closed/merged.
Changes:
- Replace DummyEvaluator placeholders with concrete evaluators and wire new DB queries needed for evaluation.
- Improve achievement processing semantics (dirty-check persistence + targeted cache eviction) and recalculation transaction boundaries.
- Emit missing lifecycle events during sync for already-terminal PRs/issues; extend YAML + schema + add unit tests.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/evaluator/StandardCountEvaluatorTest.java | Adds unit tests for the linear “count” evaluator behavior. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/evaluator/ReviewCountEvaluatorTest.java | Adds unit tests for review counting with self-review filtering. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/AchievementServiceTest.java | Adds unit tests for checkAndUnlock behavior and dirty-check optimization. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/AchievementRegistryTest.java | Adds YAML/registry validation tests (parents, cycles, triggers). |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/AchievementRecalculationServiceTest.java | Adds unit tests for recalculation replay logic. |
| server/application-server/src/main/resources/achievements/achievements.yml | Fixes triggers/parents and wires new evaluator classes + progress types. |
| server/application-server/src/main/resources/achievements/achievements-schema.json | Extends schema enum to include COMMIT_CREATED. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequest/github/GitHubPullRequestProcessor.java | Emits closed/merged lifecycle events for PRs created already terminal during sync. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/issuecomment/IssueCommentRepository.java | Adds author-count queries to support issue-based evaluators. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/issue/github/GitHubIssueProcessor.java | Emits IssueClosed event for issues that arrive already closed during sync. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/common/events/EventContext.java | Resolves providerType defensively to avoid LazyInitializationException in async handling. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/commit/CommitRepository.java | Adds commit queries for “recent commits”, “with file changes”, and extension aggregation. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityEventRepository.java | Adds time-window counting + “last activity before” queries for evaluators. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/SpeedsterEvaluator.java | Implements “merge within 5 minutes” evaluator. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/PolyglotEvaluator.java | Implements “3+ languages” evaluator. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/OracleEvaluator.java | Implements “issue open > 6 months” evaluator. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/NightOwlEvaluator.java | Implements “5 commits 01:00–05:00 UTC” evaluator. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/NecromancerEvaluator.java | Implements “close own issue with no outside comments” evaluator. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/LongTimeReturnEvaluator.java | Implements “return after 90+ days inactivity” evaluator. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/ItsyBitsyEvaluator.java | Implements “exactly 1 line changed” commit evaluator. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/HiveMindEvaluator.java | Implements “many participants/commenters” issue evaluator. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/CrossBoundaryEvaluator.java | Implements “single commit touches 2+ languages” evaluator. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/BruteForceEvaluator.java | Implements “5 commits in 5 minutes” evaluator. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AtomicChangesEvaluator.java | Implements “10 consecutive atomic commits” evaluator. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AllRarityEvaluator.java | Implements “collect all achievements of a rarity tier” evaluator. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/AchievementService.java | Adds dirty-check persistence and programmatic cache eviction. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/AchievementRegistry.java | Adds startup validation (self-parent, missing parent, cycles, evaluator existence, trigger sanity). |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/AchievementRecalculationService.java | Uses TransactionTemplate to avoid a single massive transaction; evicts cache after replay. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/AchievementEventListener.java | Avoids LazyInitializationException by logging with userId instead of login. |
...tion-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/commit/CommitRepository.java
Outdated
Show resolved
Hide resolved
Comment on lines
110
to
118
| try { | ||
| achievementService.checkAndUnlock(savedEvent); | ||
| } catch (ObjectOptimisticLockingFailureException e) { | ||
| log.warn( | ||
| "Optimistic locking conflict during recalculation for user {}, eventId={}: {}", | ||
| log.error( | ||
| "Optimistic locking conflict during recalculation (after retries exhausted) " + | ||
| "for user {}, eventId={}: {}. This event's progress increment was lost.", | ||
| LoggingUtils.sanitizeForLog(user.getLogin()), | ||
| event.getId(), | ||
| e.getMessage() |
...-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/HiveMindEvaluator.java
Outdated
Show resolved
Hide resolved
...erver/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/BruteForceEvaluator.java
Show resolved
Hide resolved
...er/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AtomicChangesEvaluator.java
Show resolved
Hide resolved
Comment on lines
+59
to
+80
| @Override | ||
| public boolean updateProgress(UserAchievement userAchievement, ActivitySavedEvent event) { | ||
| if (userAchievement.getUnlockedAt() != null) { | ||
| return false; | ||
| } | ||
|
|
||
| Long authorId = userAchievement.getUser().getId(); | ||
| var extensions = commitRepository.findDistinctFileExtensionsByAuthorId(authorId); | ||
|
|
||
| Set<String> languages = extensions | ||
| .stream() | ||
| .filter(Objects::nonNull) | ||
| .map(ext -> EXTENSION_TO_LANGUAGE.get(ext.toLowerCase())) | ||
| .filter(Objects::nonNull) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| if (languages.size() >= MIN_LANGUAGES) { | ||
| userAchievement.setProgressData(new BinaryAchievementProgress(true)); | ||
| return true; | ||
| } | ||
| return false; | ||
| } |
...-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/NightOwlEvaluator.java
Show resolved
Hide resolved
...-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/PolyglotEvaluator.java
Show resolved
Hide resolved
...ication-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityEventRepository.java
Show resolved
Hide resolved
...tion-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/commit/CommitRepository.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/application-server/src/main/resources/achievements/achievements-schema.json (1)
51-87:⚠️ Potential issue | 🟠 MajorKeep
evaluatorClassaligned with the new evaluator rollout.This schema still only permits the three legacy evaluators, so entries like
ItsyBitsyEvaluator,AtomicChangesEvaluator,PolyglotEvaluator,LongTimeReturnEvaluator, etc. remain invalid against it. That leavesachievements.ymland any schema-based validation/tooling out of sync with this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/resources/achievements/achievements-schema.json` around lines 51 - 87, The JSON schema's "evaluatorClass" enum only lists the three legacy evaluators; update that enum to include the new evaluator names (e.g. "ItsyBitsyEvaluator", "AtomicChangesEvaluator", "PolyglotEvaluator", "LongTimeReturnEvaluator") and, if you follow the same pattern, their fully qualified counterparts ("de.tum.in.www1.hephaestus.achievement.evaluator.ItsyBitsyEvaluator", etc.) so schema validation accepts the new evaluators referenced in achievements.yml; edit the "evaluatorClass" enum in the achievements-schema.json to add these identifiers (and any other new evaluator class names introduced in this PR).
🧹 Nitpick comments (8)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/common/events/EventContext.java (1)
36-49: Defensive handling for detached proxy is sound; consider tracking the TODO.The try-catch for
LazyInitializationExceptionis a reasonable workaround for detached JPA proxies. The DEBUG-level logging is appropriate since this is expected fallback behavior, not an error condition.The TODO comment (lines 36-37) documents legitimate technical debt. Consider opening an issue to track resolving
providerTypeeagerly duringProcessingContextconstruction, which would eliminate the need for exception-based control flow.Would you like me to open an issue to track refactoring the eager provider type resolution into
ProcessingContextconstruction?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/common/events/EventContext.java` around lines 36 - 49, Current code in EventContext.java catches LazyInitializationException when calling ctx.provider().getType(); create a tracking issue to refactor processing so provider type is resolved eagerly during ProcessingContext construction (as the TODO states), and update ProcessingContext to expose a non-lazy providerType so EventContext can remove the try-catch; reference ProcessingContext, provider(), providerType, EventContext, and the LazyInitializationException handling in your issue so the follow-up change is to resolve provider type in ProcessingContext constructor and drop the defensive exception-based resolution and debug log in EventContext.server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/evaluator/ReviewCountEvaluatorTest.java (1)
53-62: Consider adding test coverage for REVIEW_COMMENT target type.The
ReviewCountEvaluatorhandles bothREVIEWandREVIEW_COMMENTtarget types (per the evaluator's implementation), but tests currently only exercise theREVIEWpath. Consider adding a test case that usesActivityTargetType.REVIEW_COMMENTand stubsreviewCommentRepository.findByIdWithPullRequestAuthor()to ensure both branches are covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/evaluator/ReviewCountEvaluatorTest.java` around lines 53 - 62, Add a test in ReviewCountEvaluatorTest that mirrors the existing createReviewEvent flow but for REVIEW_COMMENT: create a helper like createReviewCommentEvent(Long targetId) that returns an ActivitySavedEvent with ActivityTargetType.REVIEW_COMMENT, stub reviewCommentRepository.findByIdWithPullRequestAuthor() to return a ReviewComment (with linked pull request author) when the evaluator queries it, and assert the evaluator behaves identically to the REVIEW case (increments counts / awards as expected); reference the existing createReviewEvent, ReviewCountEvaluatorTest class, and reviewCommentRepository.findByIdWithPullRequestAuthor() when adding the test and stubbing.server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/HiveMindEvaluator.java (1)
35-38: Participant count may over-count when issue author also commented.The current logic unconditionally adds +1 for the issue author, but if the author also commented on their own issue, they're already included in
distinctCommenters. This could award the achievement with only 9 unique participants.The comment acknowledges this ("may overlap") but labels it "conservative"—which is accurate from a product perspective (erring on the side of awarding). If strict accuracy is needed:
♻️ More accurate participant counting
- // Count distinct comment authors + the issue author = total unique participants - long distinctCommenters = issueCommentRepository.countDistinctAuthorIdsByIssueId(issue.getId()); - // Add 1 for the issue author (may overlap with commenters, but that's conservative) - long totalParticipants = distinctCommenters + 1; + // Count distinct comment authors + long distinctCommenters = issueCommentRepository.countDistinctAuthorIdsByIssueId(issue.getId()); + // Check if issue author commented (would be included in distinctCommenters) + Long authorId = issue.getAuthor() != null ? issue.getAuthor().getId() : null; + boolean authorCommented = authorId != null && + issueCommentRepository.countByIssueIdAndAuthorId(issue.getId(), authorId) > 0; + // Add issue author only if they didn't comment + long totalParticipants = distinctCommenters + (authorCommented ? 0 : 1);Note: This would require adding a
countByIssueIdAndAuthorIdmethod to the repository, which may not be worth the added complexity given the current design choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/HiveMindEvaluator.java` around lines 35 - 38, The code in HiveMindEvaluator currently computes totalParticipants by calling issueCommentRepository.countDistinctAuthorIdsByIssueId(issue.getId()) and then unconditionally adding 1 for the issue author, which double-counts when the author also commented; update the logic to determine whether the issue author is already included (e.g., add a new repository method countByIssueIdAndAuthorId(issueId, authorId) or extend countDistinctAuthorIdsByIssueId to include a flag) and only add 1 when the author did not comment — adjust the calculation around distinctCommenters and totalParticipants accordingly.server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AllRarityEvaluator.java (1)
29-42: Consider validating prerequisite IDs at startup.The
PREREQUISITESmap hardcodes achievement IDs. If these IDs change inachievements.yml, this evaluator will silently fail to match prerequisites.Given the PR already adds "startup registry validation" for cycles and evaluator existence, consider extending that validation to also verify that all IDs referenced in evaluator constants exist in the registry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AllRarityEvaluator.java` around lines 29 - 42, AllRarityEvaluator defines a hardcoded PREREQUISITES map that can go out of sync with achievements.yml; extend the existing startup registry validation to iterate every achievement ID in AllRarityEvaluator.PREREQUISITES (both keys and all values) and verify each ID exists in the achievement registry (e.g. registry.contains(id) or registry.getAchievement(id)), and if any are missing fail fast by logging and throwing an IllegalStateException (or otherwise preventing startup) so configuration mismatches are detected early.server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/evaluator/StandardCountEvaluatorTest.java (1)
58-143: Rename these tests toshould…When….The new method names don't follow the convention used for JUnit methods in this repository.
As per coding guidelines, "Use test naming convention:
should[ExpectedBehavior]When[Condition](e.g.,shouldReturnUserWhenValidIdProvided)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/evaluator/StandardCountEvaluatorTest.java` around lines 58 - 143, Rename the JUnit test methods in StandardCountEvaluatorTest to follow the repository convention should[ExpectedBehavior]When[Condition]; e.g., change incrementsFromZero() to shouldIncrementFromZeroWhenEventOccurs(), incrementsByOne() to shouldIncrementByOneWhenEventOccurs(), doesNotIncrementPastTarget() to shouldNotIncrementPastTargetWhenAtTarget(), returnsTrueAtTarget() to shouldReturnTrueWhenReachingTarget(), returnsFalseBeforeTarget() to shouldReturnFalseWhenNotAtTarget(), unlocksSingleTargetAchievement() to shouldReturnTrueWhenTargetIsOneFromFreshProgress(), and returnsFalseForWrongProgressType() to shouldReturnFalseWhenProgressTypeIsWrong(); keep the same DisplayName annotations and test bodies, only update the method identifiers in the StandardCountEvaluatorTest nested classes so names match the should…When… pattern.server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/AchievementRegistryTest.java (1)
33-209: Rename these tests toshould…When….The new method names are readable, but they don't match the repository convention used for JUnit tests under
src/test/java.As per coding guidelines, "Use test naming convention:
should[ExpectedBehavior]When[Condition](e.g.,shouldReturnUserWhenValidIdProvided)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/AchievementRegistryTest.java` around lines 33 - 209, Rename the test methods to follow the should[ExpectedBehavior]When[Condition] convention; locate the methods in the nested test classes (CommitTriggerTests: allCommitAchievementsHaveCommitCreatedTrigger -> shouldContainCommitCreatedTriggerWhenAchievementIsMainlineCommit, getByTriggerEventReturnsCommitAchievements -> shouldReturnCommitAchievementsWhenTriggerIsCommitCreated; ParentReferenceTests: noSelfReferencingParents -> shouldNotReferenceItselfWhenParentIsSet, allParentReferencesExist -> shouldHaveExistingParentWhenParentIsSpecified, noParentCycles -> shouldNotFormCyclesWhenFollowingParentChain; EvaluatorTriggerTests: dummyEvaluatorHasNoTriggers -> shouldHaveNoTriggersWhenEvaluatorIsDummy, realEvaluatorsHaveTriggers -> shouldHaveTriggersWhenEvaluatorIsReal; MilestoneFirstActionTests: includesPullRequestMergedTrigger -> shouldIncludePullRequestMergedWhenMilestoneFirstAction, includesCommitCreatedTrigger -> shouldIncludeCommitCreatedWhenMilestoneFirstAction), update the `@DisplayName` strings only if desired but ensure method names and references to these methods (e.g., in any reflection-based test tooling) are updated accordingly.server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/AchievementRecalculationServiceTest.java (2)
80-116: Consider adding verification for cache eviction.The test verifies
deleteByUserIdandcheckAndUnlockcalls, but doesn't verify thatachievementService.evictAchievementCache(user.getId())is called at the end of recalculation. This is part of the contract per line 138 ofAchievementRecalculationService.💡 Suggested addition
// Verify each event was replayed verify(achievementService, times(2)).checkAndUnlock(any(ActivitySavedEvent.class)); + + // Verify cache is evicted after replay + verify(achievementService).evictAchievementCache(1L);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/AchievementRecalculationServiceTest.java` around lines 80 - 116, The test wipesAndReplays in AchievementRecalculationServiceTest currently verifies deleteByUserId and checkAndUnlock but misses asserting that the achievement cache is evicted; update the test to also verify that achievementService.evictAchievementCache(testUser.getId() or 1L) is invoked once after service.recalculateUserInternal(testUser) by adding a verify(achievementService).evictAchievementCache(...) call so the test covers the full contract of AchievementRecalculationService.recalculateUserInternal.
118-145: Same suggestion: verify cache eviction in empty history test.💡 Suggested addition
verify(userAchievementRepository).deleteByUserId(1L); verify(achievementService, never()).checkAndUnlock(any()); + verify(achievementService).evictAchievementCache(1L);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/AchievementRecalculationServiceTest.java` around lines 118 - 145, In the handlesEmptyHistory test, also verify that the user's achievement cache is evicted after recalculateUserInternal runs: after calling service.recalculateUserInternal(testUser) and verifying userAchievementRepository.deleteByUserId(1L), add a verify(...) asserting the cache-eviction call for user id 1L (use whatever cache bean/method your code uses, e.g., cacheManager / achievementCache / userAchievementCache eviction method) so the test ensures recalculateUserInternal both clears the DB entries and evicts the cache.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/LongTimeReturnEvaluator.java`:
- Around line 36-37: The condition in LongTimeReturnEvaluator currently uses
gap.compareTo(MIN_ABSENCE) > 0 which excludes a gap exactly equal to MIN_ABSENCE
(the 90-day threshold); update the comparison to include equality (e.g., use
gap.compareTo(MIN_ABSENCE) >= 0) so a 90-day absence triggers the achievement,
touching the code around the Duration gap calculation and the if check that
references MIN_ABSENCE.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/commit/CommitRepository.java`:
- Around line 386-402: The CASE in the native query inside
findDistinctFileExtensionsByAuthorId incorrectly treats dotfiles in
subdirectories; remove the first WHEN (cf.filename LIKE '%/.%') so only the
pattern WHEN cf.filename LIKE '%.%' is used with SUBSTRING(cf.filename FROM
'\\.([^.]+)$') and LOWER(...) to extract extensions; update the `@Query` (still
referencing cf and gc) so dotfiles like src/.gitignore yield NULL and paths like
path/.eslintrc.json yield "json".
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/issue/github/GitHubIssueProcessor.java`:
- Around line 221-230: The synthetic DomainEvent.IssueClosed emission in
GitHubIssueProcessor should only run for sync-created issues to avoid a
duplicate close event (it collides with the DomainEvent.IssueClosed emitted
later in processClosed() after process(issueDto, context)). Wrap the existing
emission block (the if (issue.getState() == Issue.State.CLOSED) { ... new
DomainEvent.IssueClosed(...) ... }) with a guard that checks the processing
context is a sync run (e.g. only publish when context indicates sync:
context.isSync() or context.getOrigin() == EventContext.Origin.SYNC or a similar
flag available on EventContext), or conversely skip publishing when the context
indicates webhook/inbound processing, so only sync-originated paths emit this
synthetic close.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequest/github/GitHubPullRequestProcessor.java`:
- Around line 242-250: process(...) currently emits lifecycle events
DomainEvent.PullRequestClosed and DomainEvent.PullRequestMerged but
processClosed(...) also emits them after calling process(...), causing duplicate
publications; modify process(...) to accept a boolean flag (e.g.,
emitLifecycleEvents) or similar parameter to skip lifecycle emissions, update
all callers to pass true by default, then have processClosed(...) call
process(..., false) and publish the lifecycle events exactly once via
eventPublisher.publishEvent(new DomainEvent.PullRequestClosed(...)) /
PullRequestMerged(...); ensure you update the signature and all usages of
process(...) and keep EventPayload.PullRequestData.from(pr) /
EventContext.from(context) construction unchanged.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/AchievementRegistryTest.java`:
- Around line 157-180: The tests incorrectly treat fully-qualified dummy
evaluator names as non-dummy; update the checks in both tests (methods
dummyEvaluatorHasNoTriggers and realEvaluatorsHaveTriggers) to normalize
evaluatorClass() by checking its simple name (e.g., use def.evaluatorClass() !=
null && def.evaluatorClass().endsWith("DummyEvaluator") or extract the substring
after the last '.' and compare to "DummyEvaluator") so that both
"DummyEvaluator" and "… .DummyEvaluator" are treated as DummyEvaluator when
asserting triggerEvents on AchievementDefinition instances.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/evaluator/StandardCountEvaluatorTest.java`:
- Around line 80-88: The test doesNotIncrementPastTarget should also assert the
boolean contract of AchievementEvaluator.updateProgress: call
evaluator.updateProgress(ua, createEvent(...)) and assert that it returns false
when the UserAchievement is already complete (current == target). Update the
test method doesNotIncrementPastTarget to capture the boolean result from
StandardCountEvaluator.updateProgress and assertThat(result).isFalse(), while
keeping the existing progress counter assertion on
LinearAchievementProgress.current().
---
Outside diff comments:
In
`@server/application-server/src/main/resources/achievements/achievements-schema.json`:
- Around line 51-87: The JSON schema's "evaluatorClass" enum only lists the
three legacy evaluators; update that enum to include the new evaluator names
(e.g. "ItsyBitsyEvaluator", "AtomicChangesEvaluator", "PolyglotEvaluator",
"LongTimeReturnEvaluator") and, if you follow the same pattern, their fully
qualified counterparts
("de.tum.in.www1.hephaestus.achievement.evaluator.ItsyBitsyEvaluator", etc.) so
schema validation accepts the new evaluators referenced in achievements.yml;
edit the "evaluatorClass" enum in the achievements-schema.json to add these
identifiers (and any other new evaluator class names introduced in this PR).
---
Nitpick comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AllRarityEvaluator.java`:
- Around line 29-42: AllRarityEvaluator defines a hardcoded PREREQUISITES map
that can go out of sync with achievements.yml; extend the existing startup
registry validation to iterate every achievement ID in
AllRarityEvaluator.PREREQUISITES (both keys and all values) and verify each ID
exists in the achievement registry (e.g. registry.contains(id) or
registry.getAchievement(id)), and if any are missing fail fast by logging and
throwing an IllegalStateException (or otherwise preventing startup) so
configuration mismatches are detected early.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/HiveMindEvaluator.java`:
- Around line 35-38: The code in HiveMindEvaluator currently computes
totalParticipants by calling
issueCommentRepository.countDistinctAuthorIdsByIssueId(issue.getId()) and then
unconditionally adding 1 for the issue author, which double-counts when the
author also commented; update the logic to determine whether the issue author is
already included (e.g., add a new repository method
countByIssueIdAndAuthorId(issueId, authorId) or extend
countDistinctAuthorIdsByIssueId to include a flag) and only add 1 when the
author did not comment — adjust the calculation around distinctCommenters and
totalParticipants accordingly.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/common/events/EventContext.java`:
- Around line 36-49: Current code in EventContext.java catches
LazyInitializationException when calling ctx.provider().getType(); create a
tracking issue to refactor processing so provider type is resolved eagerly
during ProcessingContext construction (as the TODO states), and update
ProcessingContext to expose a non-lazy providerType so EventContext can remove
the try-catch; reference ProcessingContext, provider(), providerType,
EventContext, and the LazyInitializationException handling in your issue so the
follow-up change is to resolve provider type in ProcessingContext constructor
and drop the defensive exception-based resolution and debug log in EventContext.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/AchievementRecalculationServiceTest.java`:
- Around line 80-116: The test wipesAndReplays in
AchievementRecalculationServiceTest currently verifies deleteByUserId and
checkAndUnlock but misses asserting that the achievement cache is evicted;
update the test to also verify that
achievementService.evictAchievementCache(testUser.getId() or 1L) is invoked once
after service.recalculateUserInternal(testUser) by adding a
verify(achievementService).evictAchievementCache(...) call so the test covers
the full contract of AchievementRecalculationService.recalculateUserInternal.
- Around line 118-145: In the handlesEmptyHistory test, also verify that the
user's achievement cache is evicted after recalculateUserInternal runs: after
calling service.recalculateUserInternal(testUser) and verifying
userAchievementRepository.deleteByUserId(1L), add a verify(...) asserting the
cache-eviction call for user id 1L (use whatever cache bean/method your code
uses, e.g., cacheManager / achievementCache / userAchievementCache eviction
method) so the test ensures recalculateUserInternal both clears the DB entries
and evicts the cache.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/AchievementRegistryTest.java`:
- Around line 33-209: Rename the test methods to follow the
should[ExpectedBehavior]When[Condition] convention; locate the methods in the
nested test classes (CommitTriggerTests:
allCommitAchievementsHaveCommitCreatedTrigger ->
shouldContainCommitCreatedTriggerWhenAchievementIsMainlineCommit,
getByTriggerEventReturnsCommitAchievements ->
shouldReturnCommitAchievementsWhenTriggerIsCommitCreated; ParentReferenceTests:
noSelfReferencingParents -> shouldNotReferenceItselfWhenParentIsSet,
allParentReferencesExist -> shouldHaveExistingParentWhenParentIsSpecified,
noParentCycles -> shouldNotFormCyclesWhenFollowingParentChain;
EvaluatorTriggerTests: dummyEvaluatorHasNoTriggers ->
shouldHaveNoTriggersWhenEvaluatorIsDummy, realEvaluatorsHaveTriggers ->
shouldHaveTriggersWhenEvaluatorIsReal; MilestoneFirstActionTests:
includesPullRequestMergedTrigger ->
shouldIncludePullRequestMergedWhenMilestoneFirstAction,
includesCommitCreatedTrigger ->
shouldIncludeCommitCreatedWhenMilestoneFirstAction), update the `@DisplayName`
strings only if desired but ensure method names and references to these methods
(e.g., in any reflection-based test tooling) are updated accordingly.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/evaluator/ReviewCountEvaluatorTest.java`:
- Around line 53-62: Add a test in ReviewCountEvaluatorTest that mirrors the
existing createReviewEvent flow but for REVIEW_COMMENT: create a helper like
createReviewCommentEvent(Long targetId) that returns an ActivitySavedEvent with
ActivityTargetType.REVIEW_COMMENT, stub
reviewCommentRepository.findByIdWithPullRequestAuthor() to return a
ReviewComment (with linked pull request author) when the evaluator queries it,
and assert the evaluator behaves identically to the REVIEW case (increments
counts / awards as expected); reference the existing createReviewEvent,
ReviewCountEvaluatorTest class, and
reviewCommentRepository.findByIdWithPullRequestAuthor() when adding the test and
stubbing.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/evaluator/StandardCountEvaluatorTest.java`:
- Around line 58-143: Rename the JUnit test methods in
StandardCountEvaluatorTest to follow the repository convention
should[ExpectedBehavior]When[Condition]; e.g., change incrementsFromZero() to
shouldIncrementFromZeroWhenEventOccurs(), incrementsByOne() to
shouldIncrementByOneWhenEventOccurs(), doesNotIncrementPastTarget() to
shouldNotIncrementPastTargetWhenAtTarget(), returnsTrueAtTarget() to
shouldReturnTrueWhenReachingTarget(), returnsFalseBeforeTarget() to
shouldReturnFalseWhenNotAtTarget(), unlocksSingleTargetAchievement() to
shouldReturnTrueWhenTargetIsOneFromFreshProgress(), and
returnsFalseForWrongProgressType() to
shouldReturnFalseWhenProgressTypeIsWrong(); keep the same DisplayName
annotations and test bodies, only update the method identifiers in the
StandardCountEvaluatorTest nested classes so names match the should…When…
pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90a60a05-29b2-4789-93a3-73d83db020dc
📒 Files selected for processing (29)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/AchievementEventListener.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/AchievementRecalculationService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/AchievementRegistry.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/AchievementService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AllRarityEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AtomicChangesEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/BruteForceEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/CrossBoundaryEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/HiveMindEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/ItsyBitsyEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/LongTimeReturnEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/NecromancerEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/NightOwlEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/OracleEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/PolyglotEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/SpeedsterEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityEventRepository.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/commit/CommitRepository.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/common/events/EventContext.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/issue/github/GitHubIssueProcessor.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/issuecomment/IssueCommentRepository.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequest/github/GitHubPullRequestProcessor.javaserver/application-server/src/main/resources/achievements/achievements-schema.jsonserver/application-server/src/main/resources/achievements/achievements.ymlserver/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/AchievementRecalculationServiceTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/AchievementRegistryTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/AchievementServiceTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/evaluator/ReviewCountEvaluatorTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/evaluator/StandardCountEvaluatorTest.java
...r/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/LongTimeReturnEvaluator.java
Outdated
Show resolved
Hide resolved
...tion-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/commit/CommitRepository.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/de/tum/in/www1/hephaestus/gitprovider/issue/github/GitHubIssueProcessor.java
Outdated
Show resolved
Hide resolved
...ava/de/tum/in/www1/hephaestus/gitprovider/pullrequest/github/GitHubPullRequestProcessor.java
Show resolved
Hide resolved
Comment on lines
+157
to
+180
| @Test | ||
| @DisplayName("DummyEvaluator achievements have no trigger events") | ||
| void dummyEvaluatorHasNoTriggers() { | ||
| for (AchievementDefinition def : registry.values()) { | ||
| if ("DummyEvaluator".equals(def.evaluatorClass())) { | ||
| assertThat(def.triggerEvents()) | ||
| .as( | ||
| "DummyEvaluator achievement '%s' should have empty triggerEvents to avoid wasted work", | ||
| def.id() | ||
| ) | ||
| .satisfiesAnyOf( | ||
| triggers -> assertThat(triggers).isNull(), | ||
| triggers -> assertThat(triggers).isEmpty() | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("non-DummyEvaluator achievements have at least one trigger event") | ||
| void realEvaluatorsHaveTriggers() { | ||
| for (AchievementDefinition def : registry.values()) { | ||
| if (!"DummyEvaluator".equals(def.evaluatorClass())) { | ||
| assertThat(def.triggerEvents()) |
Contributor
There was a problem hiding this comment.
Normalize DummyEvaluator before asserting trigger behavior.
evaluatorClass can be configured as either DummyEvaluator or its fully qualified name, but these branches only handle the short form. A fully qualified dummy evaluator would be treated as a real evaluator and fail the second test for the wrong reason.
💡 Handle both allowed `DummyEvaluator` spellings
+ private static final Set<String> DUMMY_EVALUATOR_NAMES = Set.of(
+ "DummyEvaluator",
+ "de.tum.in.www1.hephaestus.achievement.evaluator.DummyEvaluator"
+ );
+
...
- if ("DummyEvaluator".equals(def.evaluatorClass())) {
+ if (DUMMY_EVALUATOR_NAMES.contains(def.evaluatorClass())) {
...
- if (!"DummyEvaluator".equals(def.evaluatorClass())) {
+ if (!DUMMY_EVALUATOR_NAMES.contains(def.evaluatorClass())) {📝 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
| @Test | |
| @DisplayName("DummyEvaluator achievements have no trigger events") | |
| void dummyEvaluatorHasNoTriggers() { | |
| for (AchievementDefinition def : registry.values()) { | |
| if ("DummyEvaluator".equals(def.evaluatorClass())) { | |
| assertThat(def.triggerEvents()) | |
| .as( | |
| "DummyEvaluator achievement '%s' should have empty triggerEvents to avoid wasted work", | |
| def.id() | |
| ) | |
| .satisfiesAnyOf( | |
| triggers -> assertThat(triggers).isNull(), | |
| triggers -> assertThat(triggers).isEmpty() | |
| ); | |
| } | |
| } | |
| } | |
| @Test | |
| @DisplayName("non-DummyEvaluator achievements have at least one trigger event") | |
| void realEvaluatorsHaveTriggers() { | |
| for (AchievementDefinition def : registry.values()) { | |
| if (!"DummyEvaluator".equals(def.evaluatorClass())) { | |
| assertThat(def.triggerEvents()) | |
| private static final Set<String> DUMMY_EVALUATOR_NAMES = Set.of( | |
| "DummyEvaluator", | |
| "de.tum.in.www1.hephaestus.achievement.evaluator.DummyEvaluator" | |
| ); | |
| `@Test` | |
| `@DisplayName`("DummyEvaluator achievements have no trigger events") | |
| void dummyEvaluatorHasNoTriggers() { | |
| for (AchievementDefinition def : registry.values()) { | |
| if (DUMMY_EVALUATOR_NAMES.contains(def.evaluatorClass())) { | |
| assertThat(def.triggerEvents()) | |
| .as( | |
| "DummyEvaluator achievement '%s' should have empty triggerEvents to avoid wasted work", | |
| def.id() | |
| ) | |
| .satisfiesAnyOf( | |
| triggers -> assertThat(triggers).isNull(), | |
| triggers -> assertThat(triggers).isEmpty() | |
| ); | |
| } | |
| } | |
| } | |
| `@Test` | |
| `@DisplayName`("non-DummyEvaluator achievements have at least one trigger event") | |
| void realEvaluatorsHaveTriggers() { | |
| for (AchievementDefinition def : registry.values()) { | |
| if (!DUMMY_EVALUATOR_NAMES.contains(def.evaluatorClass())) { | |
| assertThat(def.triggerEvents()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/AchievementRegistryTest.java`
around lines 157 - 180, The tests incorrectly treat fully-qualified dummy
evaluator names as non-dummy; update the checks in both tests (methods
dummyEvaluatorHasNoTriggers and realEvaluatorsHaveTriggers) to normalize
evaluatorClass() by checking its simple name (e.g., use def.evaluatorClass() !=
null && def.evaluatorClass().endsWith("DummyEvaluator") or extract the substring
after the last '.' and compare to "DummyEvaluator") so that both
"DummyEvaluator" and "… .DummyEvaluator" are treated as DummyEvaluator when
asserting triggerEvents on AchievementDefinition instances.
Comment on lines
+80
to
+88
| @Test | ||
| @DisplayName("does not increment past target") | ||
| void doesNotIncrementPastTarget() { | ||
| UserAchievement ua = createUserAchievement(5, 5); | ||
| evaluator.updateProgress(ua, createEvent(ActivityEventType.COMMIT_CREATED)); | ||
|
|
||
| LinearAchievementProgress progress = (LinearAchievementProgress) ua.getProgressData(); | ||
| assertThat(progress.current()).isEqualTo(5); // Not incremented beyond target | ||
| } |
Contributor
There was a problem hiding this comment.
Assert the boolean contract when progress is already complete.
This case only verifies the counter. AchievementEvaluator defines updateProgress() as "true if unlocked during update", and the current StandardCountEvaluator logic returns true even when current == target before the call. Adding the result assertion here will catch that regression.
💡 Tighten this test
- evaluator.updateProgress(ua, createEvent(ActivityEventType.COMMIT_CREATED));
+ boolean result = evaluator.updateProgress(ua, createEvent(ActivityEventType.COMMIT_CREATED));
LinearAchievementProgress progress = (LinearAchievementProgress) ua.getProgressData();
assertThat(progress.current()).isEqualTo(5); // Not incremented beyond target
+ assertThat(result).isFalse();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/achievement/evaluator/StandardCountEvaluatorTest.java`
around lines 80 - 88, The test doesNotIncrementPastTarget should also assert the
boolean contract of AchievementEvaluator.updateProgress: call
evaluator.updateProgress(ua, createEvent(...)) and assert that it returns false
when the UserAchievement is already complete (current == target). Update the
test method doesNotIncrementPastTarget to capture the boolean result from
StandardCountEvaluator.updateProgress and assertThat(result).isFalse(), while
keeping the existing progress counter assertion on
LinearAchievementProgress.current().
When processClosed() calls process()/processInternal(), both the internal lifecycle emission and processClosed()'s own emission fire, causing duplicate IssueClosed/PullRequestClosed/PullRequestMerged events. Add emitLifecycleOnCreate parameter to processInternal() so the sync path (process()) emits lifecycle events for new terminal entities while the webhook path (processClosed()) skips them to avoid double-emission. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequest/github/GitHubPullRequestProcessor.java (1)
359-367:⚠️ Potential issue | 🟠 MajorUse the persisted merge state when publishing close events.
After the stale short-circuit at Lines 160-174,
processInternal()can return a newer persisted PR whiledto.isMerged()still reflects an older payload. That can emitPullRequestClosed(false)for a PR that is already merged, which then drives the wrong downstream handling inserver/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityEventListener.java:163-226. Usepr.isMerged()here instead.Proposed fix
- boolean wasMerged = dto.isMerged(); + boolean wasMerged = pr.isMerged();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequest/github/GitHubPullRequestProcessor.java` around lines 359 - 367, The code uses the incoming payload flag dto.isMerged() when publishing the PullRequestClosed event, which can be stale after processInternal(...) returns a newer persisted PullRequest; change the logic to derive the merged state from the persisted object instead: replace the use of dto.isMerged() with pr.isMerged() (i.e. compute boolean wasMerged = pr.isMerged()) and use that value when constructing DomainEvent.PullRequestClosed and when deciding to publish PullRequestMerged.
♻️ Duplicate comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/issue/github/GitHubIssueProcessor.java (1)
224-236:⚠️ Potential issue | 🟠 MajorSynthetic
IssueClosedis still not scoped to sync-only create paths.
process(...)passesemitLifecycleOnCreate=true(Line 101) and is used by webhook handlers in this class (e.g., labeled/unlabeled/typed/untyped paths). If a closed issue is first seen through those handlers, this block can emitIssueClosedoutside sync-origin processing, which can still duplicate lifecycle events under replay/out-of-order delivery.#!/bin/bash set -euo pipefail echo "1) Show all handlers in GitHubIssueProcessor that call process(issueDto, context):" rg -n -C2 'Issue issue = process\(' server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/issue/github/GitHubIssueProcessor.java echo echo "2) Show where emitLifecycleOnCreate is set at call sites:" rg -n -C2 'processInternal\(.+, .+, .+, (true|false)\)' server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/issue/github/GitHubIssueProcessor.java echo echo "3) Inspect ProcessingContext for sync-origin markers usable for gating:" fd 'ProcessingContext.java' --exec sed -n '1,240p' {}Expected result: verify whether non-sync webhook paths currently enable synthetic close emission and whether an explicit sync-origin check is available for this block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/issue/github/GitHubIssueProcessor.java` around lines 224 - 236, The synthetic IssueClosed emission currently only checks emitLifecycleOnCreate and can fire for webhook paths; change the guard in GitHubIssueProcessor (the block that publishes new DomainEvent.IssueClosed) to require the processing context to indicate a sync-origin (use the sync/origin marker on ProcessingContext) in addition to emitLifecycleOnCreate and the CLOSED state, so only sync-created paths emit this synthetic close; keep the existing skip for processClosed() intact (do not modify processClosed()), update the condition that currently references emitLifecycleOnCreate to something like emitLifecycleOnCreate && context.isSyncOrigin() (or the actual sync-check accessor on ProcessingContext) before calling eventPublisher.publishEvent.
🧹 Nitpick comments (2)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AtomicChangesEvaluator.java (1)
19-21: Use@Serviceinstead of@Componentfor evaluator business logic.This class encapsulates application business logic and should use the service stereotype for consistency with server conventions.
♻️ Proposed refactor
-import org.springframework.stereotype.Component; +import org.springframework.stereotype.Service; @@ -@Component +@Service `@RequiredArgsConstructor` public class AtomicChangesEvaluator implements AchievementEvaluator {As per coding guidelines, "Use
@Serviceannotation for business logic classes,@Repositoryfor data access classes extending JpaRepository, and@RestControllerfor REST endpoints".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AtomicChangesEvaluator.java` around lines 19 - 21, The class AtomicChangesEvaluator is marked with `@Component` but implements business logic and should use the service stereotype; replace the `@Component` annotation with `@Service` on the AtomicChangesEvaluator class (retain `@RequiredArgsConstructor`) and update imports to use org.springframework.stereotype.Service to follow project conventions for service-layer beans.server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/issue/github/GitHubIssueProcessor.java (1)
142-147: Document the new internal flag in method Javadoc.
emitLifecycleOnCreatewas added toprocessInternal(...)but isn’t described in the Javadoc params, which makes behavior harder to reason about for future callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/issue/github/GitHubIssueProcessor.java` around lines 142 - 147, Update the Javadoc for the method processInternal(GitHubIssueDTO, ProcessingContext, boolean publishEvents, boolean emitLifecycleOnCreate) to include a `@param` entry for emitLifecycleOnCreate that clearly states this boolean controls whether lifecycle events are emitted when a new issue is created (true = emit lifecycle events on creation, false = suppress creation lifecycle events), and note any important interactions with publishEvents or expected behavior for callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AtomicChangesEvaluator.java`:
- Around line 36-45: The current approach fetches a fixed page
(commitRepository.findTopNByAuthorIdOrderByAuthoredAtDesc) then filters by
event.getOccurredAt(), which can miss older valid commits for historical/replay
events; change the query to fetch commits bounded by the event timestamp instead
of post-filtering—add or use a repository method that selects the top N commits
for authorId with authoredAt <= event.getOccurredAt() (keeping
REQUIRED_CONSECUTIVE as the limit) and replace the recentCommits/relevantCommits
logic with that time-bounded query to ensure the streak check sees the correct
commits.
---
Outside diff comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequest/github/GitHubPullRequestProcessor.java`:
- Around line 359-367: The code uses the incoming payload flag dto.isMerged()
when publishing the PullRequestClosed event, which can be stale after
processInternal(...) returns a newer persisted PullRequest; change the logic to
derive the merged state from the persisted object instead: replace the use of
dto.isMerged() with pr.isMerged() (i.e. compute boolean wasMerged =
pr.isMerged()) and use that value when constructing
DomainEvent.PullRequestClosed and when deciding to publish PullRequestMerged.
---
Duplicate comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/issue/github/GitHubIssueProcessor.java`:
- Around line 224-236: The synthetic IssueClosed emission currently only checks
emitLifecycleOnCreate and can fire for webhook paths; change the guard in
GitHubIssueProcessor (the block that publishes new DomainEvent.IssueClosed) to
require the processing context to indicate a sync-origin (use the sync/origin
marker on ProcessingContext) in addition to emitLifecycleOnCreate and the CLOSED
state, so only sync-created paths emit this synthetic close; keep the existing
skip for processClosed() intact (do not modify processClosed()), update the
condition that currently references emitLifecycleOnCreate to something like
emitLifecycleOnCreate && context.isSyncOrigin() (or the actual sync-check
accessor on ProcessingContext) before calling eventPublisher.publishEvent.
---
Nitpick comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AtomicChangesEvaluator.java`:
- Around line 19-21: The class AtomicChangesEvaluator is marked with `@Component`
but implements business logic and should use the service stereotype; replace the
`@Component` annotation with `@Service` on the AtomicChangesEvaluator class (retain
`@RequiredArgsConstructor`) and update imports to use
org.springframework.stereotype.Service to follow project conventions for
service-layer beans.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/issue/github/GitHubIssueProcessor.java`:
- Around line 142-147: Update the Javadoc for the method
processInternal(GitHubIssueDTO, ProcessingContext, boolean publishEvents,
boolean emitLifecycleOnCreate) to include a `@param` entry for
emitLifecycleOnCreate that clearly states this boolean controls whether
lifecycle events are emitted when a new issue is created (true = emit lifecycle
events on creation, false = suppress creation lifecycle events), and note any
important interactions with publishEvents or expected behavior for callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 458f0ca5-4c86-45f4-b8fb-4455a0ba4ee8
📒 Files selected for processing (3)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AtomicChangesEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/issue/github/GitHubIssueProcessor.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/pullrequest/github/GitHubPullRequestProcessor.java
...er/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AtomicChangesEvaluator.java
Outdated
Show resolved
Hide resolved
…erabbitai[bot] Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/LongTimeReturnEvaluator.java`:
- Around line 12-15: The Javadoc for the LongTimeReturnEvaluator mismatch:
update the class/method comment that currently reads "more than 90 days" to say
"at least 90 days" (or equivalent inclusive wording) so it matches the evaluator
logic (the >= 90 days check in LongTimeReturnEvaluator).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ba76abf-a97b-4521-830d-ac07c37ba446
📒 Files selected for processing (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/LongTimeReturnEvaluator.java
Comment on lines
+12
to
+15
| /** | ||
| * Evaluator for the "The Ancient One" achievement: | ||
| * return to activity after more than 90 days of inactivity. | ||
| */ |
Contributor
There was a problem hiding this comment.
Align Javadoc with the inclusive 90-day threshold.
Line 14 says “more than 90 days,” but Line 37 unlocks on exactly 90 days (>=). Please update the doc to “at least 90 days” to match behavior.
✏️ Suggested doc fix
/**
* Evaluator for the "The Ancient One" achievement:
- * return to activity after more than 90 days of inactivity.
+ * return to activity after at least 90 days of inactivity.
*/📝 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
| /** | |
| * Evaluator for the "The Ancient One" achievement: | |
| * return to activity after more than 90 days of inactivity. | |
| */ | |
| /** | |
| * Evaluator for the "The Ancient One" achievement: | |
| * return to activity after at least 90 days of inactivity. | |
| */ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/LongTimeReturnEvaluator.java`
around lines 12 - 15, The Javadoc for the LongTimeReturnEvaluator mismatch:
update the class/method comment that currently reads "more than 90 days" to say
"at least 90 days" (or equivalent inclusive wording) so it matches the evaluator
logic (the >= 90 days check in LongTimeReturnEvaluator).
- Technically we have no way to verify that the change happened on exactly the same line
…' into achievement-events-integrity-fix
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/ItsyBitsyEvaluator.java (5)
3-9: Unused import:BinaryAchievementProgress.The import on line 4 is not referenced anywhere in the active code (only in the commented-out block). Remove it to keep imports clean.
🧹 Proposed fix
import de.tum.in.www1.hephaestus.achievement.UserAchievement; -import de.tum.in.www1.hephaestus.achievement.progress.BinaryAchievementProgress; import de.tum.in.www1.hephaestus.activity.ActivitySavedEvent;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/ItsyBitsyEvaluator.java` around lines 3 - 9, Remove the unused import BinaryAchievementProgress from the top of ItsyBitsyEvaluator (the import line referencing de.tum.in.www1.hephaestus.achievement.progress.BinaryAchievementProgress) since it is not used in the active code; update the imports in the ItsyBitsyEvaluator class to eliminate this unused symbol and keep imports clean.
43-83: Dead code:isExactSingleLineModificationis never called.This private helper method has no callers within the class. Since the evaluator is intentionally stubbed due to technical limitations, this unused method should be removed to avoid maintenance burden.
🧹 Proposed removal
return false; - -// return commitRepository -// .findById(event.targetId()) -// .map(commit -> { -// int totalChanges = commit.getAdditions() + commit.getDeletions(); -// if (totalChanges == 1) { -// userAchievement.setProgressData(new BinaryAchievementProgress(true)); -// return true; -// } -// return false; -// }) -// .orElse(false); } - - /** - * Helper method to resolve if a git diff patch String shows only a one and the same line modification in a file - * which is technically one deletion (-) followed by one addition (+) of the same line in the git diff string - * `@param` patch The actual git diff change text to parse for checking only one line modification - * `@return` true when only one and the same line was changed - */ - private boolean isExactSingleLineModification(String patch) { - if (patch == null || patch.isEmpty()) { - return false; - } - - int additions = 0; - int deletions = 0; - boolean isAdjacent = false; - - String[] lines = patch.split("\n"); - - for (int i = 0; i < lines.length; i++) { - // Ignore file headers - if (lines[i].startsWith("---") || lines[i].startsWith("+++")) { - continue; - } - - if (lines[i].startsWith("-")) { - deletions++; - // Check if the very next line is an addition - if (i + 1 < lines.length && lines[i + 1].startsWith("+")) { - isAdjacent = true; - } - } else if (lines[i].startsWith("+")) { - additions++; - // Check if the very next line is a deletion (some git clients reverse order) - if (i + 1 < lines.length && lines[i + 1].startsWith("-")) { - isAdjacent = true; - } - } - } - - // Must be exactly 1 of each, and they must be touching - return additions == 1 && deletions == 1 && isAdjacent; - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/ItsyBitsyEvaluator.java` around lines 43 - 83, Remove the dead private helper method isExactSingleLineModification from the ItsyBitsyEvaluator class: since it has no callers and the evaluator is intentionally stubbed, delete the entire method declaration (including its JavaDoc and body) to avoid unused code and maintenance burden.
30-40: Remove commented-out code or track it in an issue.Leaving commented-out implementation in production code adds noise. If this logic is intended for future use, consider tracking it in an issue and removing the commented block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/ItsyBitsyEvaluator.java` around lines 30 - 40, Remove the commented-out commitRepository lookup and mapping block in ItsyBitsyEvaluator (the multi-line comment that references commitRepository.findById(...), commit.getAdditions(), commit.getDeletions(), and BinaryAchievementProgress) — either delete it entirely or move the logic into a tracked issue/ticket and reference that ticket ID in a short TODO comment so the production code stays clean and the intended behavior is tracked.
17-19: Injected dependencycommitRepositoryis unused.Since the logic using
commitRepositoryis commented out, this field is never accessed. If the evaluator is intentionally stubbed, consider removing the unused dependency to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/ItsyBitsyEvaluator.java` around lines 17 - 19, The private field commitRepository in ItsyBitsyEvaluator is declared but never used; either remove the unused dependency or restore the commented logic that uses it. Locate the ItsyBitsyEvaluator class and either (a) remove the commitRepository field, its constructor parameter and any related import/usages, or (b) uncomment and re-enable the code that queries commitRepository (ensuring methods like whateverCommitQueryMethod are available) so the dependency is actually used; pick one approach and update the constructor and class members accordingly to eliminate the unused field warning.
22-28: Redundant unlocked check.The check on lines 23-25 duplicates logic already in
AchievementService.updateUserAchievementsForEvent(), which skips achievements wheregetUnlockedAt() != nullbefore calling evaluators. This guard is harmless but unnecessary.🧹 Proposed simplification
`@Override` public boolean updateProgress(UserAchievement userAchievement, ActivitySavedEvent event) { - if (userAchievement.getUnlockedAt() != null) { - return false; - } - // Currently false implementation of the achievement evaluator for Itsy Bitsy that cannot be resolved due to technical limitations return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/ItsyBitsyEvaluator.java` around lines 22 - 28, Remove the redundant unlocked check from ItsyBitsyEvaluator.updateProgress: the guard if (userAchievement.getUnlockedAt() != null) duplicates logic already performed in AchievementService.updateUserAchievementsForEvent(), so delete that if-block and let updateProgress simply perform its evaluator logic (currently returning false due to technical limitations) without re-checking unlockedAt; keep the existing comment explaining the evaluator is a no-op.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/ItsyBitsyEvaluator.java`:
- Around line 3-9: Remove the unused import BinaryAchievementProgress from the
top of ItsyBitsyEvaluator (the import line referencing
de.tum.in.www1.hephaestus.achievement.progress.BinaryAchievementProgress) since
it is not used in the active code; update the imports in the ItsyBitsyEvaluator
class to eliminate this unused symbol and keep imports clean.
- Around line 43-83: Remove the dead private helper method
isExactSingleLineModification from the ItsyBitsyEvaluator class: since it has no
callers and the evaluator is intentionally stubbed, delete the entire method
declaration (including its JavaDoc and body) to avoid unused code and
maintenance burden.
- Around line 30-40: Remove the commented-out commitRepository lookup and
mapping block in ItsyBitsyEvaluator (the multi-line comment that references
commitRepository.findById(...), commit.getAdditions(), commit.getDeletions(),
and BinaryAchievementProgress) — either delete it entirely or move the logic
into a tracked issue/ticket and reference that ticket ID in a short TODO comment
so the production code stays clean and the intended behavior is tracked.
- Around line 17-19: The private field commitRepository in ItsyBitsyEvaluator is
declared but never used; either remove the unused dependency or restore the
commented logic that uses it. Locate the ItsyBitsyEvaluator class and either (a)
remove the commitRepository field, its constructor parameter and any related
import/usages, or (b) uncomment and re-enable the code that queries
commitRepository (ensuring methods like whateverCommitQueryMethod are available)
so the dependency is actually used; pick one approach and update the constructor
and class members accordingly to eliminate the unused field warning.
- Around line 22-28: Remove the redundant unlocked check from
ItsyBitsyEvaluator.updateProgress: the guard if (userAchievement.getUnlockedAt()
!= null) duplicates logic already performed in
AchievementService.updateUserAchievementsForEvent(), so delete that if-block and
let updateProgress simply perform its evaluator logic (currently returning false
due to technical limitations) without re-checking unlockedAt; keep the existing
comment explaining the evaluator is a no-op.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98856230-f056-4ed5-8e96-4ca08878e621
📒 Files selected for processing (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/ItsyBitsyEvaluator.java
- The descriptions for the two achievements were swapped accidentally
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
...-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/PolyglotEvaluator.java
Fixed
Show fixed
Hide fixed
…' into achievement-events-integrity-fix
github-actions
bot
added
the
webapp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webapp/src/components/achievements/definitions.ts (1)
203-207:⚠️ Potential issue | 🟡 MinorTypo: "issues" should be singular.
The description says "Close an issues" but should read "Close an issue".
📝 Proposed fix
"issue.close.common.1": { name: "Take That, Bug!", - description: "Close an issues", + description: "Close an issue", icon: IssueClosedIcon, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/achievements/definitions.ts` around lines 203 - 207, Fix the typo in the achievement definition for "issue.close.common.1": update the description value from "Close an issues" to "Close an issue" in the object where name is "Take That, Bug!" (look for the "issue.close.common.1" entry in definitions.ts and edit its description string).
♻️ Duplicate comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AtomicChangesEvaluator.java (1)
36-45:⚠️ Potential issue | 🟠 MajorUse a time-bounded query instead of fetch-then-filter.
Line 36 loads the latest commits globally, and Line 43 filters by
event.occurredAt()afterward. For historical/replay events, this can miss older valid commits and incorrectly fail the 10-commit streak.🔧 Proposed fix
- List<Commit> recentCommits = commitRepository.findTopNByAuthorIdOrderByAuthoredAtDesc( - authorId, - PageRequest.of(0, REQUIRED_CONSECUTIVE) - ); - - List<Commit> relevantCommits = recentCommits - .stream() - .filter(commit -> commit.getAuthoredAt() != null && !commit.getAuthoredAt().isAfter(event.occurredAt())) - .limit(REQUIRED_CONSECUTIVE) - .toList(); + List<Commit> relevantCommits = + commitRepository.findTopNByAuthorIdAndAuthoredAtLessThanEqualOrderByAuthoredAtDesc( + authorId, + event.occurredAt(), + PageRequest.of(0, REQUIRED_CONSECUTIVE) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AtomicChangesEvaluator.java` around lines 36 - 45, The code currently fetches recentCommits via commitRepository.findTopNByAuthorIdOrderByAuthoredAtDesc and then filters by event.occurredAt(), which can miss older commits for historical events; replace this fetch-then-filter with a time-bounded repository query that requests commits authoredAt <= event.occurredAt() and ordered desc, limited to REQUIRED_CONSECUTIVE (e.g. add or use a method like commitRepository.findTopNByAuthorIdAndAuthoredAtLessThanEqualOrderByAuthoredAtDesc(authorId, event.occurredAt(), PageRequest.of(0, REQUIRED_CONSECUTIVE)) and assign directly to relevantCommits), so you no longer need the stream filter/limit on recentCommits.
🧹 Nitpick comments (6)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/OracleEvaluator.java (1)
9-9: Unused@Slf4jannotation.The logger is imported via
@Slf4jbut never used in the class. Consider removing it or adding debug/trace logging for diagnostics.🔧 Proposed fix
-import lombok.extern.slf4j.Slf4j;-@Slf4j `@Component` `@RequiredArgsConstructor` public class OracleEvaluator implements AchievementEvaluator {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/OracleEvaluator.java` at line 9, The class-level Lombok logger annotation/import is unused: remove the `@Slf4j` annotation and its corresponding import (lombok.extern.slf4j.Slf4j) from the OracleEvaluator class to eliminate the dead dependency; alternatively, if you prefer to keep logging, add at least one use of the generated "log" logger (e.g., log.debug or log.trace) inside OracleEvaluator methods to justify the annotation—update either the class declaration (remove `@Slf4j`) or add a log statement in methods like evaluate(...) to fix the unused logger.server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AtomicChangesEvaluator.java (1)
19-21: Prefer@Servicefor this evaluator bean.This class contains business logic and should use
@Servicerather than@Componentfor clearer service-layer semantics.♻️ Proposed refactor
import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.data.domain.PageRequest; -import org.springframework.stereotype.Component; +import org.springframework.stereotype.Service; @@ `@Slf4j` -@Component +@Service `@RequiredArgsConstructor` public class AtomicChangesEvaluator implements AchievementEvaluator {As per coding guidelines, "Use
@Serviceannotation for business logic classes,@Repositoryfor data access classes extending JpaRepository, and@RestControllerfor REST endpoints".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AtomicChangesEvaluator.java` around lines 19 - 21, The class AtomicChangesEvaluator implements business logic as an AchievementEvaluator but is annotated with `@Component`; change the annotation to `@Service` (replace `@Component` with `@Service`) and keep `@RequiredArgsConstructor` to reflect service-layer semantics per guidelines, ensuring the class remains a Spring-managed bean with the same constructor injection behavior.server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/ItsyBitsyEvaluator.java (3)
3-6: Unused imports and injected dependency.
BinaryAchievementProgress(line 4) and the injectedCommitRepository(line 19) are not used in active code. If the evaluator is intentionally disabled, consider removing these to avoid confusion about unused dependencies.Also applies to: 19-19
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/ItsyBitsyEvaluator.java` around lines 3 - 6, Remove the unused import BinaryAchievementProgress and the unused injected dependency CommitRepository from the ItsyBitsyEvaluator class; locate the import line for BinaryAchievementProgress and delete it, then remove the CommitRepository field and its constructor or `@Autowired` parameter (and any related import) so the evaluator no longer references an unused commit repository; if the evaluator is intentionally disabled, also consider deleting or commenting any other unused fields/methods in ItsyBitsyEvaluator to avoid confusion.
27-41: Commented-out implementation should be removed or tracked separately.The evaluator is intentionally disabled per the commit note, but leaving commented-out code creates maintenance burden. The comment on line 27 mentions "technical limitations" without specifics, making it unclear when/if this can be resolved.
Consider either:
- Removing the commented-out block and documenting the limitation in a TODO with an issue link, or
- Adding a more descriptive comment explaining the specific technical limitation for future reference.
♻️ Suggested improvement
`@Override` public boolean updateProgress(UserAchievement userAchievement, ActivitySavedEvent event) { if (userAchievement.getUnlockedAt() != null) { return false; } - // Currently false implementation of the achievement evaluator for Itsy Bitsy that cannot be resolved due to technical limitations + // TODO: Implement once commit additions/deletions data is reliably available from GitHub API sync. + // See: https://github.com/ls1intum/Hephaestus/issues/XXX + // Achievement requires: commit with exactly 1 line changed (additions + deletions == 1) return false; - - // return commitRepository - // .findById(event.targetId()) - // .map(commit -> { - // int totalChanges = commit.getAdditions() + commit.getDeletions(); - // if (totalChanges == 1) { - // userAchievement.setProgressData(new BinaryAchievementProgress(true)); - // return true; - // } - // return false; - // }) - // .orElse(false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/ItsyBitsyEvaluator.java` around lines 27 - 41, The commented-out implementation in the ItsyBitsyEvaluator class (the block using commitRepository.findById(event.targetId()), commit.getAdditions()/getDeletions(), and BinaryAchievementProgress) should not remain inline: either remove the commented code and replace it with a single-line TODO that references a tracking issue/PR and briefly states the concrete technical limitation, or keep the logic in a separate referenced ticket and add a clear explanatory comment inside the evaluate method mentioning the exact limitation and the issue link; update the class comment accordingly so future readers can locate the reason and the work item to re-enable the logic.
49-83: Dead code: helper method is never called.
isExactSingleLineModificationis defined but never invoked—neither in the active code nor in the commented-out block. If the evaluator is intentionally disabled, remove this method to reduce confusion and maintenance overhead.🗑️ Remove unused helper
- /** - * Helper method to resolve if a git diff patch String shows only a one and the same line modification in a file - * which is technically one deletion (-) followed by one addition (+) of the same line in the git diff string - * `@param` patch The actual git diff change text to parse for checking only one line modification - * `@return` true when only one and the same line was changed - */ - private boolean isExactSingleLineModification(String patch) { - if (patch == null || patch.isEmpty()) { - return false; - } - - int additions = 0; - int deletions = 0; - boolean isAdjacent = false; - - String[] lines = patch.split("\n"); - - for (int i = 0; i < lines.length; i++) { - // Ignore file headers - if (lines[i].startsWith("---") || lines[i].startsWith("+++")) { - continue; - } - - if (lines[i].startsWith("-")) { - deletions++; - // Check if the very next line is an addition - if (i + 1 < lines.length && lines[i + 1].startsWith("+")) { - isAdjacent = true; - } - } else if (lines[i].startsWith("+")) { - additions++; - // Check if the very next line is a deletion (some git clients reverse order) - if (i + 1 < lines.length && lines[i + 1].startsWith("-")) { - isAdjacent = true; - } - } - } - - // Must be exactly 1 of each, and they must be touching - return additions == 1 && deletions == 1 && isAdjacent; - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/ItsyBitsyEvaluator.java` around lines 49 - 83, isExactSingleLineModification in ItsyBitsyEvaluator is dead code and should be removed to avoid confusion; delete the private method isExactSingleLineModification(String patch) from the ItsyBitsyEvaluator class if no callers are intended, or if the functionality is required, replace/remove the stub and integrate a call to isExactSingleLineModification from the evaluator entrypoint (e.g., the class's evaluation method) where patch parsing occurs so the helper is actually used; prefer removal when the evaluator is disabled to reduce maintenance overhead.server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/NecromancerEvaluator.java (1)
12-12: Unused@Slf4jannotation.The
@Slf4jannotation is present but no logging statements are used in this class. Consider either removing the annotation or adding debug/trace logging for troubleshooting (e.g., when an issue is not found or when the 6-month threshold check is performed).♻️ Option 1: Remove unused annotation
-import lombok.extern.slf4j.Slf4j; import org.springframework.stereotype.Component; /** * Evaluator for the "Necromancer" achievement: * Close an issue that has been open for over 6 months. */ -@Slf4j `@Component`♻️ Option 2: Add logging for observability
return issueRepository .findById(event.targetId()) .map(issue -> { Instant createdAt = issue.getCreatedAt(); if (createdAt == null) { + log.debug("Issue {} has no createdAt timestamp, skipping Necromancer evaluation", event.targetId()); return false; } // calculate duration between creation and the event's occurrence (closure) Instant closedAt = event.occurredAt(); ZonedDateTime created = createdAt.atZone(ZoneOffset.UTC); ZonedDateTime closed = closedAt.atZone(ZoneOffset.UTC); long months = ChronoUnit.MONTHS.between(created, closed); + log.debug("Issue {} open duration: {} months (threshold: 6)", event.targetId(), months); if (months >= 6) { userAchievement.setProgressData(new BinaryAchievementProgress(true)); return true; } return false; }) - .orElse(false); + .orElseGet(() -> { + log.debug("Issue {} not found for Necromancer evaluation", event.targetId()); + return false; + });Also applies to: 19-19
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/NecromancerEvaluator.java` at line 12, Remove the unused Lombok logging annotation or add meaningful logs: either delete the `@Slf4j` annotation from the NecromancerEvaluator class if no logging is needed, or keep it and add concise log statements (e.g., log.debug/log.info) inside methods like evaluate(...) and any helper methods to record key events such as "issue not found" checks and the 6-month threshold evaluation, using the class name NecromancerEvaluator to locate where to instrument logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/OracleEvaluator.java`:
- Around line 12-48: The OracleEvaluator.updateProgress currently checks
comments but should instead verify the issue was open at least 6 months before
close: in OracleEvaluator.updateProgress (inside the issueRepository.findById
lambda) replace the othersComments/countByIssueIdAndAuthorIdNot logic with a
time-based check that both issue.getCreatedAt() and issue.getClosedAt() are
non-null and compute monthsOpen =
ChronoUnit.MONTHS.between(issue.getCreatedAt().toInstant(),
issue.getClosedAt().toInstant()) (or equivalent ZonedDateTime conversion) and if
monthsOpen >= 6 call userAchievement.setProgressData(new
BinaryAchievementProgress(true)) and return true; otherwise return false; also
update the Javadoc to describe the 6-month-open requirement.
---
Outside diff comments:
In `@webapp/src/components/achievements/definitions.ts`:
- Around line 203-207: Fix the typo in the achievement definition for
"issue.close.common.1": update the description value from "Close an issues" to
"Close an issue" in the object where name is "Take That, Bug!" (look for the
"issue.close.common.1" entry in definitions.ts and edit its description string).
---
Duplicate comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AtomicChangesEvaluator.java`:
- Around line 36-45: The code currently fetches recentCommits via
commitRepository.findTopNByAuthorIdOrderByAuthoredAtDesc and then filters by
event.occurredAt(), which can miss older commits for historical events; replace
this fetch-then-filter with a time-bounded repository query that requests
commits authoredAt <= event.occurredAt() and ordered desc, limited to
REQUIRED_CONSECUTIVE (e.g. add or use a method like
commitRepository.findTopNByAuthorIdAndAuthoredAtLessThanEqualOrderByAuthoredAtDesc(authorId,
event.occurredAt(), PageRequest.of(0, REQUIRED_CONSECUTIVE)) and assign directly
to relevantCommits), so you no longer need the stream filter/limit on
recentCommits.
---
Nitpick comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AtomicChangesEvaluator.java`:
- Around line 19-21: The class AtomicChangesEvaluator implements business logic
as an AchievementEvaluator but is annotated with `@Component`; change the
annotation to `@Service` (replace `@Component` with `@Service`) and keep
`@RequiredArgsConstructor` to reflect service-layer semantics per guidelines,
ensuring the class remains a Spring-managed bean with the same constructor
injection behavior.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/ItsyBitsyEvaluator.java`:
- Around line 3-6: Remove the unused import BinaryAchievementProgress and the
unused injected dependency CommitRepository from the ItsyBitsyEvaluator class;
locate the import line for BinaryAchievementProgress and delete it, then remove
the CommitRepository field and its constructor or `@Autowired` parameter (and any
related import) so the evaluator no longer references an unused commit
repository; if the evaluator is intentionally disabled, also consider deleting
or commenting any other unused fields/methods in ItsyBitsyEvaluator to avoid
confusion.
- Around line 27-41: The commented-out implementation in the ItsyBitsyEvaluator
class (the block using commitRepository.findById(event.targetId()),
commit.getAdditions()/getDeletions(), and BinaryAchievementProgress) should not
remain inline: either remove the commented code and replace it with a
single-line TODO that references a tracking issue/PR and briefly states the
concrete technical limitation, or keep the logic in a separate referenced ticket
and add a clear explanatory comment inside the evaluate method mentioning the
exact limitation and the issue link; update the class comment accordingly so
future readers can locate the reason and the work item to re-enable the logic.
- Around line 49-83: isExactSingleLineModification in ItsyBitsyEvaluator is dead
code and should be removed to avoid confusion; delete the private method
isExactSingleLineModification(String patch) from the ItsyBitsyEvaluator class if
no callers are intended, or if the functionality is required, replace/remove the
stub and integrate a call to isExactSingleLineModification from the evaluator
entrypoint (e.g., the class's evaluation method) where patch parsing occurs so
the helper is actually used; prefer removal when the evaluator is disabled to
reduce maintenance overhead.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/NecromancerEvaluator.java`:
- Line 12: Remove the unused Lombok logging annotation or add meaningful logs:
either delete the `@Slf4j` annotation from the NecromancerEvaluator class if no
logging is needed, or keep it and add concise log statements (e.g.,
log.debug/log.info) inside methods like evaluate(...) and any helper methods to
record key events such as "issue not found" checks and the 6-month threshold
evaluation, using the class name NecromancerEvaluator to locate where to
instrument logging.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/OracleEvaluator.java`:
- Line 9: The class-level Lombok logger annotation/import is unused: remove the
`@Slf4j` annotation and its corresponding import (lombok.extern.slf4j.Slf4j) from
the OracleEvaluator class to eliminate the dead dependency; alternatively, if
you prefer to keep logging, add at least one use of the generated "log" logger
(e.g., log.debug or log.trace) inside OracleEvaluator methods to justify the
annotation—update either the class declaration (remove `@Slf4j`) or add a log
statement in methods like evaluate(...) to fix the unused logger.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02b988f1-1a39-4b60-9429-b0bffc5b6953
📒 Files selected for processing (6)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/AtomicChangesEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/ItsyBitsyEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/NecromancerEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/OracleEvaluator.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/PolyglotEvaluator.javawebapp/src/components/achievements/definitions.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/PolyglotEvaluator.java
Comment on lines
+12
to
+48
| /** | ||
| * Evaluator for the "Oracle" achievement: | ||
| * Close your own issue that had zero interaction from others. | ||
| */ | ||
| @Slf4j | ||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class OracleEvaluator implements AchievementEvaluator { | ||
|
|
||
| private final IssueRepository issueRepository; | ||
| private final IssueCommentRepository issueCommentRepository; | ||
|
|
||
| @Override | ||
| public boolean updateProgress(UserAchievement userAchievement, ActivitySavedEvent event) { | ||
| if (userAchievement.getUnlockedAt() != null) { | ||
| return false; | ||
| } | ||
|
|
||
| Long userId = userAchievement.getUser().getId(); | ||
|
|
||
| return issueRepository | ||
| .findById(event.targetId()) | ||
| .map(issue -> { | ||
| // Must be the issue author | ||
| if (issue.getAuthor() == null || !issue.getAuthor().getId().equals(userId)) { | ||
| return false; | ||
| } | ||
| // Must have zero comments from others | ||
| long othersComments = issueCommentRepository.countByIssueIdAndAuthorIdNot(issue.getId(), userId); | ||
| if (othersComments == 0) { | ||
| userAchievement.setProgressData(new BinaryAchievementProgress(true)); | ||
| return true; | ||
| } | ||
| return false; | ||
| }) | ||
| .orElse(false); | ||
| } |
Contributor
There was a problem hiding this comment.
Implementation logic does not match the Oracle achievement requirement.
According to the PR objectives, the Oracle achievement should unlock when an "issue open ≥6 months before close." However, this implementation checks for "zero comments from others," which is the condition for the Necromancer achievement (issue.special.necromancer: close own issue with 0 comments from others).
The Javadoc also describes the wrong achievement. The relevant code snippet confirms that countByIssueIdAndAuthorIdNot is documented as "Used by Necromancer achievement evaluator."
The correct logic should calculate the duration between issue creation and close, verifying it is at least 6 months:
// Expected Oracle logic (pseudo-code):
if (issue.getClosedAt() != null && issue.getCreatedAt() != null) {
long monthsOpen = ChronoUnit.MONTHS.between(
issue.getCreatedAt().toInstant(),
issue.getClosedAt().toInstant()
);
if (monthsOpen >= 6) {
userAchievement.setProgressData(new BinaryAchievementProgress(true));
return true;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/achievement/evaluator/OracleEvaluator.java`
around lines 12 - 48, The OracleEvaluator.updateProgress currently checks
comments but should instead verify the issue was open at least 6 months before
close: in OracleEvaluator.updateProgress (inside the issueRepository.findById
lambda) replace the othersComments/countByIssueIdAndAuthorIdNot logic with a
time-based check that both issue.getCreatedAt() and issue.getClosedAt() are
non-null and compute monthsOpen =
ChronoUnit.MONTHS.between(issue.getCreatedAt().toInstant(),
issue.getClosedAt().toInstant()) (or equivalent ZonedDateTime conversion) and if
monthsOpen >= 6 call userAchievement.setProgressData(new
BinaryAchievementProgress(true)) and return true; otherwise return false; also
update the Javadoc to describe the 6-month-open requirement.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
qpxxl-tum
reviewed
Mar 14, 2026
Contributor
qpxxl-tum
left a comment
qpxxl-tum
left a comment
There was a problem hiding this comment.
somethings are missing, some are fixed by me
-mainly the AchievementRegistry is somehow not that good
- ItsyBitsyEvaluator was deactivated by me because of missing intended functionality and technical limitations
- self reference is not a bug in achievements but inteded. I fixed it.
- haven't had the time to look through the tests thorougly and local testing is still open
| @RequiredArgsConstructor | ||
| public class AllRarityEvaluator implements AchievementEvaluator { | ||
|
|
||
| private static final Map<String, Set<String>> PREREQUISITES = Map.of( |
Contributor
There was a problem hiding this comment.
Is fine for now but for better conditions we could think of the user having either issue.opened.rare or issue.closed.rare and for the other rarities respectively
| @Slf4j | ||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class NecromancerEvaluator implements AchievementEvaluator { |
Contributor
There was a problem hiding this comment.
This functionality is wrong. I will correct it
| @Slf4j | ||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class OracleEvaluator implements AchievementEvaluator { |
Contributor
There was a problem hiding this comment.
Also wrong but will also be corrected. It appears I accidentally swapped the descriptions for both achievements
| String parentId = record.parent(); | ||
| if (parentId != null && !parentId.isEmpty()) { | ||
| // Self-reference check | ||
| if (parentId.equals(record.id())) { |
Contributor
There was a problem hiding this comment.
self-referencing achievements are NOT an error but intended. Those achievements are standalone and not connected to the avatar node (unlike achievements without a parent which are automatically connected to the avatar node.
- ItsyBitsyEvaluator: fix single-line edit detection. A modification is 1 add + 1 delete in git, so `totalChanges == 1` never matched. Now checks `changedFiles == 1 && additions <= 1 && deletions <= 1`. - HiveMindEvaluator: fix participant double-counting. The old `distinctCommenters + 1` overcounted when the issue author also commented. Now uses UNION query for exact distinct participant count. - Extract shared LanguageExtensions utility from duplicate maps in CrossBoundaryEvaluator and PolyglotEvaluator. PolyglotEvaluator was missing sql, zsh, yml, yaml, html, css, scss extensions. - AllRarityEvaluator: add @PostConstruct startup validation that all hardcoded prerequisite achievement IDs exist in the registry. App fails fast if achievements.yml drifts. - StandardCountEvaluator: add unlock guard for consistency with all other evaluators. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sues - Fix 3 self-referencing parent entries in achievements.yml that broke noSelfReferencingParents and noParentCycles tests: - issue.special.necromancer → parent: issue.close.common.2 - milestone.long_time_return → parent: milestone.first_action - milestone.all_rare → parent: milestone.first_action - Fix AtomicChangesEvaluator to pass event.occurredAt() to updated findTopNByAuthorIdOrderByAuthoredAtDesc(authorId, asOf, pageable) - Fix PolyglotEvaluator method name to match actual repository method Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…arden achievement retry Two systemic bugs caused achievement evaluation failures: 1. CommitCreated events were never emitted when commits were created via GitHubPullRequestProcessor.upsertMergeCommit() or GitLabCommitSyncService, causing all commit-triggered achievements to remain stuck at low counts despite hundreds of commits in the database. 2. AchievementService @retryable only caught ObjectOptimisticLockingFailureException but not DataIntegrityViolationException from INSERT race conditions, causing silent permanent loss of achievement increments under concurrent processing. Fix 1: Add check-first-then-publish pattern to both commit creation paths, emitting DomainEvent.CommitCreated for genuinely new commits. Add REST_SYNC to DataSource enum for GitLab sync context. Fix 2: Expand @retryable to catch DataIntegrityViolationException (INSERT races), increase max attempts from 3 to 5, and use exponential backoff (100ms base, 2x multiplier, 2s max) instead of fixed 100ms delay. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📚 Documentation Preview
|
qpxxl-tum
requested changes
Mar 16, 2026
Contributor
qpxxl-tum
left a comment
qpxxl-tum
left a comment
There was a problem hiding this comment.
Self-reference should be preserved as this is not a bug but an intended feature that is correctly handled by the frontend
|
|
||
| - id: "issue.special.necromancer" | ||
| parent: "issue.special.necromancer" | ||
| parent: "issue.close.common.2" |
Contributor
There was a problem hiding this comment.
Still not a bug but a feature!!!
Self-referencing achievements appear as standalone in the frontend without beeing connected to the avatar node (this connection is implizit when parent == null).
They are used as standalone milestones (and often hidden ones) that can be unlocked through special conditions and serve as a separate motivational factor!
|
|
||
| - id: "milestone.long_time_return" | ||
| parent: "milestone.long_time_return" | ||
| parent: "milestone.first_action" |
Contributor
There was a problem hiding this comment.
also wrong, should be self-referencing
|
|
||
| - id: "milestone.all_rare" | ||
| parent: "milestone.all_rare" | ||
| parent: "milestone.first_action" |
Contributor
There was a problem hiding this comment.
here also an intended self-reference
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
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.
Summary
New Evaluators
pr.special.speedstercommit.special.itsy_bitsycommit.special.brute_forcecommit.special.atomic_changescommit.special.cross_boundaryissue.special.hive_mindissue.special.oracleissue.special.necromancermilestone.polyglotmilestone.night_owlmilestone.long_time_returnmilestone.all_rare/epic/legendaryBugs Fixed (from initial commit)
COMMIT_CREATEDto commit achievement triggers (were empty[])triggerEventson DummyEvaluator achievementsPULL_REQUEST_MERGED,COMMIT_CREATEDtomilestone.first_actiontriggerspr.special.speedsterfrom Linear to BinaryAchievementProgressCOMMIT_CREATEDtoachievements-schema.jsontriggerEvents enum@Transactionalself-invocation (replaced withTransactionTemplate)@CacheEvictwith programmatic cache evictioncheckAndUnlock()— only save when progress changesLazyInitializationException— useuser.getId()instead ofuser.getLogin()on detached proxyLive Verification
Full sync of
ls1intum/Hephaestus— 49/49 achievements verified for FelixTJDietrich:Test plan
cd server/application-server && ./mvnw test -P'!quick'— 1253 tests, 0 failures/users/{login}/achievementsAPI/workspaces/{slug}/users/{login}/achievements/recalculateLazyInitializationExceptionin logs during sync🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores