Bugfix - Minor relevance algorithm changes #9282
Conversation
…o posted news for the last month
WalkthroughRefactors EcoNews counting from a single-date check to date-range queries, expands cached user-relevant-news with a minimumAvailableDate, updates cache and relevance services to use date windows and new error conditions, adds pool-aware weight normalization, and updates tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant RelevanceService as EcoNewsRelevanceService
participant Cache
participant CacheService
participant Repo as EcoNewsRepository
Client->>RelevanceService: findRelevantEcoNews(userId, page)
RelevanceService->>Cache: getCachedUserRelevantNews(userId)
alt cache miss
RelevanceService->>CacheService: computeCachedData(userId)
CacheService->>Repo: countEcoNewsBetweenDates(startDate, endDate)
Repo-->>CacheService: count
CacheService->>Cache: store CachedUserRelevantNews(minDate, maxDate, counts...)
Cache-->>RelevanceService: CachedUserRelevantNews
else cache hit
Cache-->>RelevanceService: CachedUserRelevantNews
end
RelevanceService->>RelevanceService: validate cache (totalNewsCount, lastGeneratedPage, page)
alt no interactions
RelevanceService-->>Client: throw EcoNewsRelevanceCalculationException (USER_HAS_NO_INTERACTIONS_YET)
else invalid page or zero-count
RelevanceService-->>Client: throw NotFoundException (RELEVANT_NEWS_FOR_MONTH_NOT_FOUND / NO_MORE_RELEVANT_NEWS)
else valid page
RelevanceService->>Repo: countEcoNewsBetweenDates(minimumAvailableDate, lastRequestedDate)
Repo-->>RelevanceService: date-bounded count
RelevanceService->>RelevanceService: normalizeWeights(weights, pools)
RelevanceService-->>Client: relevant news DTO
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
dao/src/main/java/greencity/repository/EcoNewsRepo.java(1 hunks)service-api/src/main/java/greencity/constant/ErrorMessage.java(1 hunks)service-api/src/main/java/greencity/dto/cache/CachedUserRelevantNews.java(1 hunks)service/src/main/java/greencity/service/CacheServiceImpl.java(2 hunks)service/src/main/java/greencity/service/EcoNewsRelevanceServiceImpl.java(3 hunks)service/src/main/java/greencity/utils/RelevanceWeightUtils.java(2 hunks)service/src/test/java/greencity/ModelUtils.java(1 hunks)service/src/test/java/greencity/service/CacheServiceImplTest.java(1 hunks)service/src/test/java/greencity/service/EcoNewsRelevanceServiceImplTest.java(9 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-03-14T16:22:58.043Z
Learnt from: maks741
Repo: ita-social-projects/GreenCity PR: 8244
File: service/src/main/java/greencity/service/UserNotificationServiceImpl.java:78-123
Timestamp: 2025-03-14T16:22:58.043Z
Learning: In UserNotificationServiceImpl, the use of .limit(2L * page.getPageSize()) when merging notifications from GreenCity and UBS is intentional. This ensures that all notifications from both sources can be accommodated without losing any data during the merging process.
Applied to files:
service/src/main/java/greencity/service/EcoNewsRelevanceServiceImpl.java
📚 Learning: 2025-04-30T21:07:48.800Z
Learnt from: ChernenkoVitaliy
Repo: ita-social-projects/GreenCity PR: 8372
File: core/src/main/java/greencity/controller/ExportSettingsController.java:0-0
Timestamp: 2025-04-30T21:07:48.800Z
Learning: The GreenCity application has a custom Pageable configuration that sets default values (page 0, size 20) when not explicitly specified, making PageableDefault annotation unnecessary.
Applied to files:
service/src/main/java/greencity/service/EcoNewsRelevanceServiceImpl.java
📚 Learning: 2025-08-22T15:53:22.505Z
Learnt from: RostyslavZadyraichuk
Repo: ita-social-projects/GreenCity PR: 8931
File: dao/src/main/java/greencity/repository/EcoNewsRelevanceRepo.java:12-22
Timestamp: 2025-08-22T15:53:22.505Z
Learning: When limiting JPQL query results to a fixed number (like "last 10 items"), use Pageable parameter with PageRequest.of(0, N) instead of JPQL LIMIT clause, which is not supported and causes runtime exceptions.
Applied to files:
service/src/main/java/greencity/service/EcoNewsRelevanceServiceImpl.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
service/src/main/java/greencity/service/EcoNewsRelevanceServiceImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
service/src/main/java/greencity/utils/RelevanceWeightUtils.java(2 hunks)service/src/test/java/greencity/service/EcoNewsRelevanceServiceImplTest.java(9 hunks)service/src/test/java/greencity/utils/RelevanceWeightUtilsTest.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
service/src/test/java/greencity/utils/RelevanceWeightUtilsTest.java (2)
170-199: Well-structured test with flexible validation.The parameterized test design is excellent, providing three validation modes (exact match, index-specific assertions, and general properties). This approach offers comprehensive coverage for the pool-aware normalization logic.
228-229: Test case will fail due to implementation bug.This test case with all empty pools will trigger the division by zero issue identified in the implementation. Once the guard condition is added to Line 105 of
RelevanceWeightUtils.java, this test should pass correctly with the expected result{0.0, 0.0, 0.0}.service/src/test/java/greencity/service/EcoNewsRelevanceServiceImplTest.java (5)
173-174: Exception handling for empty user profiles aligns with PR objectives.The updated test correctly reflects the new behavior where users without interaction history receive an exception rather than falling back to generic news. This matches the PR's goal of handling cases "when a user preferences profile is not yet created."
200-200: Appropriate exception type for non-existent pages.Using
NotFoundExceptioninstead ofEcoNewsRelevanceCalculationExceptionprovides clearer semantics for the case where a requested page doesn't exist. This aligns with the PR objective of handling overstated page requests with a specific exception.
236-239: Enhanced cache state mocking supports expanded data model.The additional mock returns for
lastGeneratedPage,totalNewsCount,totalPagesCount,minimumAvailableDate, andlastRequestedDateproperly support the expandedCachedUserRelevantNewsmodel. These changes align with the PR's shift to date-windowed caching and pagination tracking.Also applies to: 282-288
328-328: Method rename reflects date-range query architecture.The change from
countEcoNewsBeforeDatetocountEcoNewsBetweenDatesaligns with the PR objective of narrowing the relevance query scope to a specific time window (last month). This provides more precise control over the news pool used for relevance calculations.
388-389: Enhanced verifications support dynamic cache behavior.The additional verifications for
totalNewsCountandtotalPagesCount(Lines 388-389) ensure the cache state is properly utilized. The relaxed verification at Line 401 usinganyInt()withatLeastOnce()appropriately handles dynamic page generation scenarios where the exact generated page number may vary during test execution.Also applies to: 401-401
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
service/src/test/java/greencity/utils/RelevanceWeightUtilsTest.java (2)
185-185: Add braces for consistency and safety.Single-statement if blocks should use braces to prevent future errors and maintain consistency with project style.
Apply this diff:
- if (result.length > 2) + if (result.length > 2) { assertEquals(assertions[2], result[2], PRECISION); + }
179-198: Consider simplifying the assertion logic for better maintainability.The test combines three different assertion strategies in conditional branches, making it harder to understand which test case exercises which logic. The null-input check (lines 194-197) occurs after all other assertions, which means null inputs still execute lines 179-192 before being validated.
Consider refactoring into separate focused test methods or at minimum, handle the null case first:
- One test for exact expected values
- One test for indexed assertions
- One test for sum and ordering validation
- One dedicated test for null inputs
This approach would improve test clarity and make failures easier to diagnose.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
service/src/test/java/greencity/utils/RelevanceWeightUtilsTest.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
service/src/test/java/greencity/utils/RelevanceWeightUtilsTest.java (2)
10-12: LGTM! Imports are necessary and well-organized.The new imports support the parameterized test for pool-aware weight normalization.
219-237: Test data is mathematically correct; consider adding documentation and explicit assertions.The redistribution logic for line 223-224 is actually correct—when the weak pool is empty, its weight (0.3) is divided equally between the two populated pools (2 pools): 0.2 + 0.15 = 0.35 and 0.5 + 0.15 = 0.65. The algorithm redistributes empty pools' weights proportionally to remaining pools, which your test data validates well.
However, one remaining concern: line 233-234 passes
nullfor bothexpectedandassertionsparameters. This forces the test to rely on implicit sum and ordering checks rather than explicit expected values, which can obscure intent for maintainers. Consider either providing explicit expected output or adding a test comment clarifying why this case uses implicit validation.Adding a brief JavaDoc comment above the data provider explaining the redistribution formula would help future reviewers understand what these test cases validate.
|
Changed:
Summary by CodeRabbit
New Features
Bug Fixes