Remove use of recurrence in opfab (#7499) #7759
Conversation
vlo-rte
commented
Dec 26, 2024
•
vlo-rte
commented
- In release notes :
- In chapter : Tasks
- Text : Remove use of recurrence in opfab #7499 : Remove field "timespans.recurrence" in opfab
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive removal of recurrence-related functionality across multiple services and components in the OperatorFabric system. The changes span various files in client, node-services, services, and UI modules, systematically eliminating Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Nitpick comments (8)
services/cards-publication/src/test/java/org/opfab/cards/publication/kafka/integration/KafkaSerializeDeserializeShould.java (1)
58-58: Confirm reliance onInstant.now().
This test code usesInstant.now()in a test scenario, which can sometimes introduce flaky behaviors if the test environment experiences timing variations or if the system clock changes. You might consider using a fixed timestamp or mockingInstant.now()to ensure deterministic results.ui/main/src/app/services/reminder/RruleReminderUtils.ts (2)
42-43: Clearer early return.
Rather than using an if statement that checks the condition and returns-1, consider an explicit early return to reduce nesting and improve readability.-function getNextTimeForRepeatingFromTimeSpan(timeSpan: TimeSpan, card: Card, startingDate: number): number { - if (timeSpan) { - if (timeSpan.start + MAX_MILLISECONDS_FOR_REMINDING_AFTER_EVENT_STARTS < startingDate) { - return -1; - } else { - return timeSpan.start; - } - } - return -1; +function getNextTimeForRepeatingFromTimeSpan(timeSpan: TimeSpan, card: Card, startingDate: number): number { + if (!timeSpan) { + return -1; + } + if (timeSpan.start + MAX_MILLISECONDS_FOR_REMINDING_AFTER_EVENT_STARTS < startingDate) { + return -1; + } + return timeSpan.start; +}
42-45: Validate assumption on 15-minute threshold.
Confirm thatMAX_MILLISECONDS_FOR_REMINDING_AFTER_EVENT_STARTSis set to 15 minutes intentionally. If changes are needed in the future, consider extracting this constant into a shared configuration for easy adjustability.services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java (1)
105-105: Replicate time span coverage for additional test data.This snippet repeats the same usage of
TimeSpan(Instant.ofEpochMilli(123l), null). If modified behavior or additional edge cases are expected, ensure these test data points capture a range of scenarios (e.g., start date far in the future, or boundary conditions).ui/main/src/app/business/view/timeline/circles.spec.ts (1)
224-225: Ensure correct handling of multipleTimeSpanentries.When creating multiple time spans, confirm that all intervals are displayed as intended on the timeline. Additional tests may be beneficial to verify correct rendering/aggregation of events.
ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts (1)
315-315: Consider returning an empty array instead ofundefined.Returning an empty array might simplify downstream logic. Check any places that assume
card.timeSpansis always defined and avoid possible null checks.ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts (1)
477-477: Further testing of singleTimeSpan.When
viewCardInCalendaris true, the code sets a singleTimeSpan. Confirm that downstream consumption (UI rendering, reminder logic, etc.) properly handles this single-interval scenario.services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java (1)
453-453: Consider standardizing the Instant construction approach for consistency
Here,Instant.ofEpochMilli(123l)is used, whereas later lines employInstant.ofEpochSecond(123l). This discrepancy in time units might cause confusion and complicate maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
client/cards/src/main/avro/timeSpan.avdl(0 hunks)node-services/cards-reminder/src/domain/application/reminderUtils.ts(2 hunks)node-services/cards-reminder/src/domain/model/card.model.ts(1 hunks)node-services/cards-reminder/src/tests/cardsReminderControl.test.ts(3 hunks)node-services/cards-reminder/src/tests/reminderUtils.test.ts(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/HoursAndMinutes.java(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/Recurrence.java(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/TimeSpan.java(1 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/HoursAndMinutes.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/Recurrence.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java(1 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardValidationService.java(0 hunks)services/cards-publication/src/main/modeling/swagger.yaml(0 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/kafka/integration/KafkaSerializeDeserializeShould.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardPermissionControlServiceShould.java(7 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java(4 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/ExternalAppServiceShould.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java(2 hunks)src/docs/asciidoc/reference_doc/card_notification.adoc(0 hunks)src/docs/asciidoc/reference_doc/user_cards.adoc(0 hunks)ui/main/src/app/business/model/card.model.ts(1 hunks)ui/main/src/app/business/view/timeline/circles.spec.ts(2 hunks)ui/main/src/app/business/view/timeline/circles.ts(1 hunks)ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts(3 hunks)ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts(1 hunks)ui/main/src/app/modules/calendar/calendar.component.ts(1 hunks)ui/main/src/app/services/reminder/ReminderUtils.spec.ts(1 hunks)ui/main/src/app/services/reminder/ReminderUtils.ts(2 hunks)ui/main/src/app/services/reminder/RruleReminderUtils.ts(1 hunks)
💤 Files with no reviewable changes (10)
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/HoursAndMinutes.java
- src/docs/asciidoc/reference_doc/card_notification.adoc
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/Recurrence.java
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/HoursAndMinutes.java
- client/cards/src/main/avro/timeSpan.avdl
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/Recurrence.java
- src/docs/asciidoc/reference_doc/user_cards.adoc
- node-services/cards-reminder/src/tests/reminderUtils.test.ts
- services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardValidationService.java
- services/cards-publication/src/main/modeling/swagger.yaml
✅ Files skipped from review due to trivial changes (1)
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardPermissionControlServiceShould.java
🔇 Additional comments (24)
node-services/cards-reminder/src/domain/application/reminderUtils.ts (2)
10-10: Imports look good.
No issues found; the removal of recurrence-related imports aligns with the PR objective of deprecating recurrence usage.
33-36: Verify edge cases where timeSpan.start equals startingDate.
Currently, if timeSpan.start < startingDate, the function returns -1; otherwise, it returns timeSpan.start. Consider whether timeSpan.start === startingDate warrants special handling (e.g., returning -1 or timeSpan.start) based on the desired behavior.
ui/main/src/app/services/reminder/ReminderUtils.ts (2)
10-10: Imports appear consistent with the new architecture.
No recurrence imports remain, which is consistent with the PR’s objective.
32-35: Double-check the else branch logic.
When timeSpan.start + MAX_MILLISECONDS_FOR_REMINDING_AFTER_EVENT_STARTS is still >= startingDate, the function returns timeSpan.start. Ensure no off-by-one or boundary condition issues exist if timeSpan.start is very close to startingDate.
services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java (1)
21-21: Simplified TimeSpan constructor.
Removing the recurrence parameter ensures consistency with the rest of the codebase. Confirm there are no stray references to recurrence in the publication package.
✅ Verification successful
TimeSpan constructor change is safe to proceed
The verification shows that:
- There are no references to
recurrencewithin the cards-publication package - All TimeSpan constructor calls in the publication package are using the simplified version with just start and end parameters
The removal of the recurrence parameter from the TimeSpan constructor is consistent with the codebase usage, and no further changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify there are no lingering references to `recurrence` within the publication package.
fd --glob '*.java' | xargs rg 'recurrence'
Length of output: 148
Script:
#!/bin/bash
# Let's check if there are any references to recurrence specifically in the publication package
rg -g '*.java' -l 'recurrence' services/cards-publication/
# Also check for any usages of TimeSpan constructor to ensure all calls are updated
ast-grep --pattern 'new TimeSpan($$$)'
Length of output: 5648
node-services/cards-reminder/src/domain/model/card.model.ts (1)
25-25: RRule usage after removing recurrence.
Given the PR aims to remove recurrence, ensure RRule is not duplicating the old logic or left as unused. Verify if RRule ties into new code paths.
✅ Verification successful
Let me gather more information about the RRule usage and changes.
Let me check one more aspect to understand the RRule service implementation better.
RRule is still actively used for reminder functionality
Based on the verification, RRule is still an integral part of the reminder system and not a vestige of old recurrence logic. The RRuleReminderService actively uses it to:
- Calculate next reminder times via
getNextTimeForRepeating - Handle card operations (ADD/UPDATE/DELETE)
- Manage reminder persistence and scheduling
The optional rRule field in the card model is intentional and used by the current reminder implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Confirm that references to `RRule` remain purposeful and not vestiges of recurrence usage.
rg 'new RRule' -A 5
Length of output: 4331
Script:
#!/bin/bash
# Check for RRule imports and type definitions
rg "import.*RRule"
# Check for RRule-related interfaces and types
ast-grep --pattern 'interface $_ {
$$$
rRule?: $_;
$$$
}'
# Look for RRule service implementations
rg "class.*RRule.*Service" -A 10
Length of output: 2959
Script:
#!/bin/bash
# Check the implementation of RRuleReminderService
cat node-services/cards-reminder/src/domain/application/rruleReminderService.ts
# Look for recent changes in RRule related files
rg "rRule" node-services/cards-reminder/src/domain/model/card.model.ts -A 2
Length of output: 7007
ui/main/src/app/services/reminder/ReminderUtils.spec.ts (2)
10-10: Check global import usage.
The import statements for Card and TimeSpan come from @ofModel/card.model. Verify that this file is still the correct source of truth after the removal of recurrence code.
Line range hint 12-28: Tests successfully verifying a non-recurrent scenario.
The logic in each test checks that if the current date is not too far beyond timeSpan.start, the function returns the start; otherwise, it returns -1. These tests appear accurate and consistent with the updated non-recurrence scenario.
services/cards-publication/src/test/java/org/opfab/cards/publication/services/ExternalAppServiceShould.java (1)
100-100: Validate TimeSpan instantiation.
Calling new TimeSpan(Instant.ofEpochMilli(123l), null) is consistent with the removal of recurrence. Confirm whether an end date of null is safe in all usage contexts.
services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java (1)
91-91: Confirm null usage for end date in TimeSpan.
Using null for the end date is allowed, but ensure that the logic and any subsequent usage correctly handle open-ended time spans. Consider adding a test case explicitly covering TimeSpan(…, null) if not already present.
ui/main/src/app/business/model/card.model.ts (1)
116-116: Constructor parameter removal validated.
Removing the third parameter from TimeSpan (formerly recurrence) aligns with the overall PR goal of eliminating recurrence logic. Verify all references or calls within the codebase no longer rely on that parameter.
ui/main/src/app/modules/calendar/calendar.component.ts (1)
101-112: Event creation scoped to explicit start/end times.
Events are now added solely based on startDate and endDate from the timeSpan. Since recurrence logic has been removed, confirm that downstream functionalities requiring repeating events have been updated accordingly. Also verify that setting allDay: false meets the intended calendar behavior for these events.
ui/main/src/app/business/view/timeline/circles.ts (1)
51-51: Consistently handle time spans.
The code now funnels all time spans through computeCardForTimeLine, removing any recurrence-specific logic. This simplification is appropriate, but double-check that no residual references to removed recurrence methods exist elsewhere in this class.
ui/main/src/app/business/view/timeline/circles.spec.ts (1)
14-14: Confirm usage of TimeSpan import.
Ensure that references to any removed recurrence functionality have been fully excised. Verify that no lingering code depends on the old recurrence parameter.
ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts (1)
311-311: Handle potential null or undefined date values for new TimeSpan.
Ensure that timeSpan.startDate and timeSpan.endDate are present and valid. If either is undefined, consider how this might affect reminders or front-end displays.
node-services/cards-reminder/src/tests/cardsReminderControl.test.ts (3)
14-14: Confirm complete removal of Recurrence.
import {RRule, TimeSpan, Day, Frequency}... indicates the old Recurrence reference is gone, which is aligned with the PR objective. Perform a quick scan to ensure no orphaned references or code paths remain.
302-302: Check null handling in TimeSpan creation.
Creating a TimeSpan with a null endDate is valid, but ensure that logic around open-ended time spans is thoroughly tested (e.g. indefinite event durations).
416-416: Typographical fix.
Changing the comment to “1 minute later” clarifies the test scenario. This is a small but helpful improvement.
ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts (3)
25-25: Import cleanup
Removing Recurrence from imports is consistent with the rest of the PR. No issues detected.
465-468: Verify usage of new TimeSpan(10, 20).
Ensure that the test checks the correct interpretation of start/end boundaries. If the user’s logic expects inclusive or exclusive boundaries, make sure the test coverage matches real usage.
497-500: Ignore viewCardInCalendar if timeSpans exist.
Make sure the developer’s intention is for timeSpans to override viewCardInCalendar. Double-check user experience to avoid unexpected overshadowing of the viewCardInCalendar option.
services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java (3)
491-491: Validate that null end date for TimeSpan is handled as intended
Creating TimeSpan with a null second parameter may be an intended scenario. Confirm that downstream logic properly handles null as an open-ended or unspecified time.
512-512: Consistency check on using Instant.ofEpochSecond(123l)
This is a valid approach, aligning with the same pattern in other lines. Ensure the test scenario distinctly verifies second-based epochs versus millisecond-based to avoid confusion.
525-525: Ensure adequate coverage for second-based epoch
Reusing Instant.ofEpochSecond(123l) consistently helps maintain clarity compared to mixing seconds and milliseconds, especially when verifying time-based logic.
| @@ -14,6 +14,5 @@ | |||
|
|
|||
| public record TimeSpan( | |||
| Instant start, | |||
| Instant end, | |||
| Recurrence recurrence) { | |||
| Instant end) { | |||
There was a problem hiding this comment.
💡 Codebase verification
The recurrence field has been replaced by rRule field, but there are still references to the old field
The codebase shows that the recurrence field has been deprecated in favor of the rRule field, but there are still several files containing references to the old field:
src/tooling/migration-rrule-recurrence/migration-rrule-recurrence.js: Migration script that converts oldrecurrenceformat to newrRuleformatui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts: Contains deprecated field warning messagessrc/test/api/karate/cards/cardsWithTimespans.feature: Test cases still using the oldrecurrencefieldnode-services/cards-reminder/src/tests/cardsReminderControl.test.ts: Tests using the oldrecurrencefield
These files should be updated to use the new rRule field for consistency, even though they might be test or migration-related files.
🔗 Analysis chain
Removal of the recurrence field looks correct.
If this field was previously used in other classes or avro schemas, confirm no references remain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify no references to the removed `recurrence` field remain in the codebase.
rg -A 3 '[. ]recurrence'
Length of output: 29828
| viewCardInCalendar: true | ||
| }); | ||
| cardBuilder.setFieldVisible(InputFieldName.StartDate, true); | ||
| cardBuilder.setStartDate(50); | ||
| cardBuilder.setFieldVisible(InputFieldName.EndDate, true); | ||
| cardBuilder.setEndDate(100); | ||
| const card = await cardBuilder.getCard(); | ||
| expect(card.timeSpans).toEqual([new TimeSpan(50, 100, new Recurrence(new HourAndMinutes(1, 0)))]); | ||
| expect(card.timeSpans).toEqual([new TimeSpan(50, 100)]); |
There was a problem hiding this comment.
Recurrence removal in the view logic.
Though the code references the deprecated field, it only warns about it. Keep track of any external calls or data flows that still rely on recurrence logic and clean them up soon.
Would you like assistance in further removing references to the deprecated recurrence field?
vlo-rte
force-pushed
the
FE-7499-RemoveUseOfRecurrenceInOpfab
branch
from
c971278 to
5d3a563
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/test/api/karate/cards/cardsWithTimespans.feature (2)
108-109: Confirm the correctness of multiple identical time spans.Two time spans with the same
startandendmight be redundant. Check if duplication has any side effects on the reminder logic or validation.
139-139: Typo consideration: "timepans" vs. "timespans".If “timepans” is intentional, no action needed. Otherwise, consider renaming to “timespans” for consistency and clarity.
node-services/cards-reminder/src/tests/cardsReminderControl.test.ts (1)
416-416: LGTM: Improved comment clarityThe comment has been corrected from "1 minutes" to "1 minute", improving documentation clarity.
ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts (1)
Line range hint
465-500: Consider adding negative test cases.While the existing test cases cover the basic functionality, consider adding negative test cases to verify proper handling of invalid time spans (e.g., endDate before startDate).
Example test case to add:
it('Should handle invalid time spans gracefully', async () => { setSpecificCardInformation({ valid: true, card: {}, timeSpans: [{startDate: 20, endDate: 10}] // Invalid: end before start }); const card = await cardBuilder.getCard(); expect(card.timeSpans).toEqual(undefined); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
client/cards/src/main/avro/timeSpan.avdl(0 hunks)node-services/cards-reminder/src/domain/application/reminderUtils.ts(2 hunks)node-services/cards-reminder/src/domain/model/card.model.ts(1 hunks)node-services/cards-reminder/src/tests/cardsReminderControl.test.ts(3 hunks)node-services/cards-reminder/src/tests/reminderUtils.test.ts(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/HoursAndMinutes.java(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/Recurrence.java(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/TimeSpan.java(1 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/HoursAndMinutes.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/Recurrence.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java(1 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardValidationService.java(0 hunks)services/cards-publication/src/main/modeling/swagger.yaml(0 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/kafka/integration/KafkaSerializeDeserializeShould.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardPermissionControlServiceShould.java(7 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java(4 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/ExternalAppServiceShould.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java(2 hunks)src/docs/asciidoc/reference_doc/card_notification.adoc(0 hunks)src/docs/asciidoc/reference_doc/user_cards.adoc(0 hunks)src/test/api/karate/cards/cardsWithTimespans.feature(7 hunks)ui/main/src/app/business/model/card.model.ts(1 hunks)ui/main/src/app/business/view/timeline/circles.spec.ts(2 hunks)ui/main/src/app/business/view/timeline/circles.ts(1 hunks)ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts(3 hunks)ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts(1 hunks)ui/main/src/app/modules/calendar/calendar.component.ts(1 hunks)ui/main/src/app/services/reminder/ReminderUtils.spec.ts(1 hunks)ui/main/src/app/services/reminder/ReminderUtils.ts(2 hunks)ui/main/src/app/services/reminder/RruleReminderUtils.ts(1 hunks)
💤 Files with no reviewable changes (10)
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/HoursAndMinutes.java
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/Recurrence.java
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/Recurrence.java
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/HoursAndMinutes.java
- node-services/cards-reminder/src/tests/reminderUtils.test.ts
- services/cards-publication/src/main/modeling/swagger.yaml
- src/docs/asciidoc/reference_doc/card_notification.adoc
- services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardValidationService.java
- src/docs/asciidoc/reference_doc/user_cards.adoc
- client/cards/src/main/avro/timeSpan.avdl
🚧 Files skipped from review as they are similar to previous changes (11)
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/ExternalAppServiceShould.java
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/TimeSpan.java
- services/cards-publication/src/test/java/org/opfab/cards/publication/kafka/integration/KafkaSerializeDeserializeShould.java
- ui/main/src/app/services/reminder/RruleReminderUtils.ts
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardPermissionControlServiceShould.java
- ui/main/src/app/services/reminder/ReminderUtils.spec.ts
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java
- ui/main/src/app/business/view/timeline/circles.ts
- ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts
- node-services/cards-reminder/src/domain/model/card.model.ts
🔇 Additional comments (21)
src/test/api/karate/cards/cardsWithTimespans.feature (6)
43-43: Consider explicitly handling open-ended TimeSpan entries.
Here, "end" is defined in the first object but is missing in the second. If the intent is to allow open-ended time spans, please ensure the downstream logic correctly handles timeSpans without an "end" property. Otherwise, consider validating or rejecting incomplete time spans.
64-64: Double-check timeSpans consistency.
Mirroring the logic from line 43, ensure the system clearly distinguishes between open-ended and closed-ended time spans here as well, and that it doesn’t allow ambiguous or malformed ranges.
85-85: Validate repeated pattern.
Similar to previous changes, ensure there's a consistent mechanism to handle the second time span missing the "end" property.
[approve]
132-133: Validate negative secondsBeforeTimeSpanForReminder usage.
Ensure the logic properly handles secondsBeforeTimeSpanForReminder: -1000. The scenario states it should fail the request, but verify that all other time spans remain unaffected and the system produces an appropriate error message.
157-158: Double-check repeated timeSpan blocks.
As with lines 108-109, confirm that multiple time spans with identical values are either expected or intentionally tested for corner cases.
202-202: Scenario title correctly updated to remove recurrence context.
The title now matches the updated validation logic that focuses solely on TimeSpan constraints.
node-services/cards-reminder/src/domain/application/reminderUtils.ts (2)
10-10: Ensure comprehensive removal of recurrence references.
By removing recurrence logic, confirm there are no orphaned methods or references to recurrence left in card.model or related modules that might cause runtime errors.
33-36: Logic simplification looks correct.
The time span is rejected if timeSpan.start is already past startingDate, otherwise returned. Validate that edge cases (e.g., timeSpan.start === startingDate) behave as intended.
ui/main/src/app/services/reminder/ReminderUtils.ts (2)
10-10: Check for leftover imports or references post-removal.
After deleting all recurrence logic, verify that no import is left behind accidentally. Confirm that @ofModel/card.model no longer references any removed structures.
32-35: TimeSpan-based reminder check is clear.
Using MAX_MILLISECONDS_FOR_REMINDING_AFTER_EVENT_STARTS is a straightforward threshold. Revisit default durations if user feedback suggests different defaults.
ui/main/src/app/business/model/card.model.ts (1)
116-116: Constructor change aligns with the removal of recurrence.
Removing the recurrence parameter simplifies the model. Ensure that any downstream usage (including tests) is updated to avoid passing extra parameters.
ui/main/src/app/modules/calendar/calendar.component.ts (1)
101-112: LGTM: Clean implementation of calendar event creation
The simplified event creation logic correctly handles basic timespan events with proper validation of end dates. The code maintains good readability with clear property assignments and consistent class naming.
ui/main/src/app/business/view/timeline/circles.spec.ts (1)
224-225: LGTM: Correct usage of simplified TimeSpan constructor
The TimeSpan constructor is properly used with only start and end date parameters, consistent with the removal of recurrence functionality.
node-services/cards-reminder/src/tests/cardsReminderControl.test.ts (2)
14-14: LGTM: Clean up of imports
The import statement has been correctly updated to remove unused recurrence-related types.
302-302: LGTM: Simplified test card creation
The getTestCard function has been properly simplified to use only TimeSpan without recurrence, maintaining test coverage for the core functionality.
services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java (1)
453-453: LGTM! TimeSpan instantiations updated correctly.
All TimeSpan instantiations have been properly updated to remove the recurrence parameter, aligning with the PR objective to remove recurrence functionality.
Also applies to: 468-468, 477-477, 483-490, 497-497, 500-500, 512-512, 525-525
ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts (5)
25-25: LGTM: Import statement updated correctly.
The removal of the Recurrence import aligns with the PR objective of removing recurrence functionality.
465-468: LGTM: TimeSpan test case simplified.
The test case has been correctly updated to use only startDate and endDate parameters, removing recurrence-related complexity.
477-477: LGTM: TimeSpan instantiation simplified.
The TimeSpan constructor call has been appropriately updated to use only the essential parameters (startDate, endDate).
483-490: LGTM: Recurrence-related test case updated.
The test case has been properly modified to focus on basic TimeSpan functionality without recurrence, while maintaining the viewCardInCalendar behavior.
497-500: LGTM: TimeSpan array handling simplified.
The test case correctly verifies TimeSpan creation with only startDate and endDate parameters when provided through timeSpans configuration.
vlo-rte
force-pushed
the
FE-7499-RemoveUseOfRecurrenceInOpfab
branch
from
5d3a563 to
55c8a73
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
node-services/cards-reminder/src/domain/application/reminderUtils.ts (1)
33-36: Consider edge case handling in time comparisonThe simplified time comparison logic might benefit from additional validation:
- Handling of null/undefined timeSpan.start
- Validation of timeSpan.start being a valid timestamp
if (timeSpan.start < startingDate) { return -1; } else { + if (!timeSpan.start || isNaN(timeSpan.start)) { + return -1; + } return timeSpan.start; }ui/main/src/app/services/reminder/ReminderUtils.spec.ts (1)
Line range hint
16-40: Add test cases for edge casesWhile the current tests cover the basic functionality well, consider adding tests for:
- Null/undefined timeSpans
- Invalid start dates (NaN, null)
- Different timezones
Example test cases to add:
it('Should handle null timeSpans', () => { testCard.timeSpans = null; const dateForRepeating = getNextTimeForRepeating(testCard); expect(dateForRepeating).toEqual(-1); }); it('Should handle invalid start date in timeSpan', () => { testCard.timeSpans = [new TimeSpan(NaN)]; const dateForRepeating = getNextTimeForRepeating(testCard); expect(dateForRepeating).toEqual(-1); });ui/main/src/app/modules/calendar/calendar.component.ts (1)
101-112: Consider optimizing calendar event creationThe event creation logic has been simplified correctly to handle only basic time spans. However, we could optimize the array concatenation operation.
Consider using array push or spread operator instead of concat for better performance:
- this.calendarEvents = this.calendarEvents.concat({ + this.calendarEvents.push({ id: card.id, title: card.titleTranslated, start: startDate, end: endDate, className: [ 'opfab-calendar-event', 'opfab-calendar-event-' + card.severity.toLowerCase() ], allDay: false });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
client/cards/src/main/avro/timeSpan.avdl(0 hunks)node-services/cards-reminder/src/domain/application/reminderUtils.ts(2 hunks)node-services/cards-reminder/src/domain/model/card.model.ts(1 hunks)node-services/cards-reminder/src/tests/cardsReminderControl.test.ts(1 hunks)node-services/cards-reminder/src/tests/reminderUtils.test.ts(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/HoursAndMinutes.java(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/Recurrence.java(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/TimeSpan.java(1 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/HoursAndMinutes.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/Recurrence.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java(1 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardValidationService.java(0 hunks)services/cards-publication/src/main/modeling/swagger.yaml(0 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/kafka/integration/KafkaSerializeDeserializeShould.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardPermissionControlServiceShould.java(7 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java(4 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/ExternalAppServiceShould.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java(2 hunks)src/docs/asciidoc/reference_doc/card_notification.adoc(0 hunks)src/docs/asciidoc/reference_doc/user_cards.adoc(0 hunks)src/test/api/karate/cards/cardsWithTimespans.feature(7 hunks)ui/main/src/app/business/model/card.model.ts(1 hunks)ui/main/src/app/business/view/timeline/circles.spec.ts(2 hunks)ui/main/src/app/business/view/timeline/circles.ts(1 hunks)ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts(3 hunks)ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts(1 hunks)ui/main/src/app/modules/calendar/calendar.component.ts(1 hunks)ui/main/src/app/services/reminder/ReminderUtils.spec.ts(1 hunks)ui/main/src/app/services/reminder/ReminderUtils.ts(2 hunks)ui/main/src/app/services/reminder/RruleReminderUtils.ts(1 hunks)
💤 Files with no reviewable changes (10)
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/HoursAndMinutes.java
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/Recurrence.java
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/HoursAndMinutes.java
- src/docs/asciidoc/reference_doc/card_notification.adoc
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/Recurrence.java
- node-services/cards-reminder/src/tests/reminderUtils.test.ts
- src/docs/asciidoc/reference_doc/user_cards.adoc
- client/cards/src/main/avro/timeSpan.avdl
- services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardValidationService.java
- services/cards-publication/src/main/modeling/swagger.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/ExternalAppServiceShould.java
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/TimeSpan.java
- services/cards-publication/src/test/java/org/opfab/cards/publication/kafka/integration/KafkaSerializeDeserializeShould.java
- node-services/cards-reminder/src/domain/model/card.model.ts
- ui/main/src/app/services/reminder/RruleReminderUtils.ts
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java
- ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardPermissionControlServiceShould.java
- ui/main/src/app/business/view/timeline/circles.ts
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java
- ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts
🔇 Additional comments (11)
services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java (3)
453-453: LGTM: TimeSpan constructor updated correctly.
The removal of the recurrence parameter from the TimeSpan constructor aligns with the PR's objective of removing recurrence functionality.
491-491: LGTM: TimeSpan constructor updated correctly in child card test.
The TimeSpan constructor has been properly updated while maintaining the test's core purpose of validating parent-child card relationships.
512-512: LGTM: TimeSpan constructors updated correctly in parent UID validation test.
Both TimeSpan constructor calls have been properly updated while maintaining the test's core purpose of validating parent card UID relationships.
Also applies to: 525-525
ui/main/src/app/services/reminder/ReminderUtils.ts (1)
32-35:
Synchronize reminder timeout behavior across services
The UI implementation includes a 15-minute timeout (MAX_MILLISECONDS_FOR_REMINDING_AFTER_EVENT_STARTS) that's not present in the node-services implementation. This could lead to inconsistent behavior.
Let's verify if this constant exists in node-services:
Consider synchronizing the implementations to ensure consistent behavior across the application.
src/test/api/karate/cards/cardsWithTimespans.feature (2)
43-43: LGTM: TimeSpans structure simplified correctly
The timeSpans structure has been correctly simplified to only include start and end properties, aligning with the removal of recurrence functionality.
Also applies to: 64-64, 85-85
139-139: Verify test coverage after recurrence removal
While the scenario titles have been updated correctly to reflect the removal of recurrence functionality, we should ensure that the remaining test cases provide adequate coverage for the simplified time spans functionality.
Also applies to: 202-202
✅ Verification successful
Let me gather more information about the timespan test coverage.
Test coverage for time spans functionality is adequate
The test coverage for time spans is comprehensive and properly maintained after the recurrence removal:
- The
cardsWithTimespans.featurefile contains essential test scenarios:- Basic time span card creation and validation
- Verification of time span properties (start/end dates)
- Validation of secondsBeforeTimeSpanForReminder
- Error handling for invalid time span configurations
- Card deletion with time spans
Additional time span related tests are present in:
fetchArchivedCardsWithFilter.featurefor time span filteringgetCardSubscription.featurefor time range queriescardsReminder.featurefor reminder functionality with time spans
While there are some remaining references to recurrence in test files, they are mostly in template files or legacy test files (postCardWithRRuleRecurrence.feature) that should be removed separately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to recurrence in test files
# and verify test coverage for time spans
# Check for any remaining recurrence references in test files
echo "Checking for remaining recurrence references in tests..."
rg -i "recurrence" "src/test" || true
# List all test scenarios related to time spans
echo "Listing time span related test scenarios..."
rg -A 1 "Scenario.*time[Ss]pan" "src/test" || true
Length of output: 2896
Script:
#!/bin/bash
# Search for all test scenarios in cardsWithTimespans.feature to understand the test coverage
echo "Checking test scenarios in cardsWithTimespans.feature..."
cat src/test/api/karate/cards/cardsWithTimespans.feature || true
# Search for any other timespan-related tests that might use different naming
echo "Checking for other timespan-related tests..."
rg -i "timespan|time span|start.*end" "src/test/api/karate/cards/" || true
Length of output: 11241
ui/main/src/app/business/model/card.model.ts (1)
116-116: Verify usage of TimeSpan class after signature change
The TimeSpan constructor has been correctly simplified to remove the recurrence parameter. However, we should verify that all instantiations of TimeSpan have been updated accordingly.
✅ Verification successful
TimeSpan constructor changes have been properly updated across the codebase
Based on the verification results, all instances of TimeSpan instantiation across the codebase are using the simplified constructor correctly, without the recurrence parameter. The instances found are primarily in test files and builders, and they all follow the pattern of using either:
- Single parameter:
new TimeSpan(startDate) - Two parameters:
new TimeSpan(startDate, endDate)
The remaining references to "Recurrence" are in:
- Migration scripts (
migration-rrule-recurrence.js) - Template files (
.handlebars) - Test files (
.spec.ts,.test.ts) - Translation files (
en.json,fr.json,nl.json) - RRule-related utilities
These references are expected as they are part of the migration from Recurrence to RRule functionality and don't indicate any missed updates to TimeSpan usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all TimeSpan instantiations to verify they've been updated
# Search for TimeSpan instantiations
echo "Checking TimeSpan instantiations..."
ast-grep --pattern 'new TimeSpan($$$)'
# Search for any remaining references to Recurrence
echo "Checking for remaining Recurrence references..."
rg -l "Recurrence" || true
Length of output: 6544
ui/main/src/app/modules/calendar/calendar.component.ts (1)
101-112: Verify calendar event display after recurrence removal
While the basic event creation looks correct, we should verify that the calendar still displays events properly and that there are no visual regressions.
✅ Verification successful
Let me gather more information about the calendar tests to verify the event display functionality.
Calendar event display verification is well-covered by tests
The calendar implementation and tests show that:
- The calendar component correctly handles event display with proper styling and severity classes
- Cypress tests verify calendar events display in all views (month, week, day)
- Tests confirm event interaction (clicking) and content verification
- The event creation code is properly structured with required properties (id, title, start, end, className)
The removal of recurrence functionality doesn't impact the basic event display as:
- Basic event creation is independent of recurrence logic
- Event styling and display properties remain intact
- Test coverage validates the core calendar functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for calendar-related test coverage
# Search for calendar component tests
echo "Checking calendar component tests..."
fd -e spec.ts -e spec.js calendar
# Check for any remaining recurrence-related calendar functionality
echo "Checking for remaining recurrence references in calendar code..."
rg -l "recurrence" "ui/main/src/app/modules/calendar" || true
Length of output: 370
Script:
#!/bin/bash
# Check calendar test implementation and calendar component code
echo "Checking calendar test implementation..."
cat src/test/cypress/cypress/integration/Calendar.spec.js
echo -e "\nChecking calendar component implementation..."
cat ui/main/src/app/modules/calendar/calendar.component.ts
Length of output: 10656
ui/main/src/app/business/view/timeline/circles.spec.ts (1)
224-225: LGTM! TimeSpan constructor updates are consistent.
The removal of the recurrence parameter from the TimeSpan constructor calls aligns with the PR objective of removing recurrence functionality.
node-services/cards-reminder/src/tests/cardsReminderControl.test.ts (2)
14-14: LGTM! Import statement updated correctly.
The removal of HourAndMinutes and Recurrence from imports aligns with the PR objective of removing recurrence functionality.
Line range hint 1-500: Test coverage looks comprehensive.
The test suite thoroughly covers both RRule and timespan-based reminder scenarios, including:
- Timing conditions for sending/not sending reminders
- Multiple cards handling
- Database reset scenarios
- Card updates and deletions
- Edge cases (null/negative secondsBeforeTimeSpanForReminder)
vlo-rte
force-pushed
the
FE-7499-RemoveUseOfRecurrenceInOpfab
branch
2 times, most recently
from
8bfd96c to
dce3ba6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ui/main/src/app/modules/calendar/calendar.component.ts (1)
Line range hint
1-1: Consider complete recurrence removal strategyThe current implementation shows an inconsistent approach to recurrence removal:
- TimeSpan-based recurrence has been removed
- RRule-based recurrence remains in the codebase
This partial removal might lead to confusion about which recurrence mechanism should be used. Consider either:
- Complete removal of all recurrence mechanisms (including RRule)
- Documentation clarifying why RRule-based recurrence is retained while TimeSpan-based recurrence is removed
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
client/cards/src/main/avro/timeSpan.avdl(0 hunks)node-services/cards-reminder/src/domain/application/reminderUtils.ts(2 hunks)node-services/cards-reminder/src/domain/model/card.model.ts(1 hunks)node-services/cards-reminder/src/tests/cardsReminderControl.test.ts(1 hunks)node-services/cards-reminder/src/tests/reminderUtils.test.ts(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/HoursAndMinutes.java(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/Recurrence.java(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/TimeSpan.java(1 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/HoursAndMinutes.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/Recurrence.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java(1 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardValidationService.java(0 hunks)services/cards-publication/src/main/modeling/swagger.yaml(0 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/kafka/integration/KafkaSerializeDeserializeShould.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardPermissionControlServiceShould.java(7 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java(4 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/ExternalAppServiceShould.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java(2 hunks)src/docs/asciidoc/reference_doc/card_notification.adoc(0 hunks)src/docs/asciidoc/reference_doc/user_cards.adoc(0 hunks)src/test/api/karate/cards/cardsWithTimespans.feature(7 hunks)ui/main/src/app/business/model/card.model.ts(1 hunks)ui/main/src/app/business/view/timeline/circles.spec.ts(2 hunks)ui/main/src/app/business/view/timeline/circles.ts(1 hunks)ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts(3 hunks)ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts(1 hunks)ui/main/src/app/modules/calendar/calendar.component.ts(1 hunks)ui/main/src/app/services/reminder/ReminderUtils.spec.ts(0 hunks)ui/main/src/app/services/reminder/ReminderUtils.ts(0 hunks)ui/main/src/app/services/reminder/RruleReminderUtils.ts(1 hunks)
💤 Files with no reviewable changes (12)
- src/docs/asciidoc/reference_doc/card_notification.adoc
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/HoursAndMinutes.java
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/HoursAndMinutes.java
- client/cards/src/main/avro/timeSpan.avdl
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/Recurrence.java
- ui/main/src/app/services/reminder/ReminderUtils.spec.ts
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/Recurrence.java
- node-services/cards-reminder/src/tests/reminderUtils.test.ts
- services/cards-publication/src/main/modeling/swagger.yaml
- src/docs/asciidoc/reference_doc/user_cards.adoc
- ui/main/src/app/services/reminder/ReminderUtils.ts
- services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardValidationService.java
🚧 Files skipped from review as they are similar to previous changes (12)
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/TimeSpan.java
- ui/main/src/app/services/reminder/RruleReminderUtils.ts
- services/cards-publication/src/test/java/org/opfab/cards/publication/kafka/integration/KafkaSerializeDeserializeShould.java
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/ExternalAppServiceShould.java
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardPermissionControlServiceShould.java
- node-services/cards-reminder/src/domain/model/card.model.ts
- ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java
- ui/main/src/app/business/view/timeline/circles.ts
- ui/main/src/app/business/view/timeline/circles.spec.ts
- src/test/api/karate/cards/cardsWithTimespans.feature
🔇 Additional comments (12)
services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java (2)
491-491: LGTM: TimeSpan constructor calls consistently updated.
All TimeSpan constructor calls in the test cases have been consistently updated to remove the recurrence parameter. The changes maintain the test coverage while adapting to the simplified time span model.
Let's verify that test coverage remains comprehensive after removing recurrence-related tests:
Also applies to: 512-512, 525-525
✅ Verification successful
TimeSpan usage is consistent and recurrence-related code has been properly removed
The verification confirms:
- No references to "recurrence" remain in any test files
- All TimeSpan constructor calls consistently use the new two-parameter format (start, end)
- The TimeSpan class is now a simple record with just start and end Instant fields
- Test coverage remains comprehensive with multiple test cases using the simplified TimeSpan model
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining recurrence-related test methods or assertions
# Test: Search for test methods or assertions that might still reference recurrence
rg -l 'test.*recurrence|assert.*recurrence' --type java
Length of output: 57
Script:
#!/bin/bash
# Let's check for any remaining references to recurrence in test files
rg -i "recurrence" --type java services/cards-publication/src/test/
# Let's also check the TimeSpan usage in test files to ensure consistency
rg "new TimeSpan\(" --type java services/cards-publication/src/test/
# Let's examine the TimeSpan class to understand its current structure
fd TimeSpan.java --type f --exec cat {}
Length of output: 4203
453-453: LGTM: TimeSpan constructor updated correctly.
The TimeSpan constructor call has been updated to remove the recurrence parameter, aligning with the broader changes to remove recurrence functionality.
Let's verify that all TimeSpan usages have been updated consistently:
✅ Verification successful
TimeSpan constructor calls consistently updated across the codebase
Based on the verification results:
- All TimeSpan constructor calls in Java code now use the simplified constructor without the recurrence parameter
- The only remaining recurrence-related Java file is
FreqEnum.javawhich appears to be a model class that can be safely removed in a future cleanup - The changes in the test file are consistent with other TimeSpan usage across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining recurrence-related code in TimeSpan usage
# Test: Search for TimeSpan constructor calls that might still include recurrence
ast-grep --pattern 'new TimeSpan($$$, recurrence)'
Length of output: 52
Script:
#!/bin/bash
# Search for TimeSpan constructor calls to verify consistent usage
rg "new TimeSpan\(" -A 2
# Also check for any remaining recurrence-related code
rg "recurrence" --type java
Length of output: 12039
node-services/cards-reminder/src/domain/application/reminderUtils.ts (1)
33-37: Verify edge case handling in simplified time span logic
The simplified logic correctly handles the basic case but should be verified for edge cases:
- When
startingDateis undefined (uses current time) - When dealing with timezone differences
✅ Verification successful
Edge cases are properly handled in the simplified time span logic
The code inspection reveals that the edge cases are well-handled:
- The
startingDateparameter is optional and has a proper fallback:
if (startingDate == null) {
startingDate = new Date().valueOf();
}-
The function uses millisecond timestamps (
.valueOf()) which are timezone-independent, avoiding any timezone-related issues. -
The simplified logic is actually cleaner and safer than before, as it:
- Properly handles undefined startingDate
- Uses universal timestamp comparisons
- Returns consistent results (-1 for past dates, timeSpan.start for future dates)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for timezone-related code that might be affected
rg -l "(?i)(timezone|tz|utc)" --type ts
# Look for tests covering these scenarios
rg -l "getNextTimeForRepeatingFromTimeSpan.*undefined" --type ts
Length of output: 1682
Script:
#!/bin/bash
# Let's check the implementation and test coverage of the function
ast-grep --pattern 'function getNextTimeForRepeatingFromTimeSpan' -A 10
rg "getNextTimeForRepeatingFromTimeSpan" -A 5 --type ts
# Check test files for this function
fd ".*\.test\.ts$|.*\.spec\.ts$" -x cat {} | rg -A 10 "getNextTimeForRepeatingFromTimeSpan"
Length of output: 2887
ui/main/src/app/business/model/card.model.ts (2)
115-117: TimeSpan class simplified correctly
The TimeSpan class has been correctly simplified to remove the recurrence property.
Line range hint 127-141: Verify if RRule class should be removed
The RRule class appears to handle recurrence-related functionality (frequency, intervals, weekdays, etc.). Since the PR objective is to remove recurrence, should this class also be removed?
✅ Verification successful
The RRule class should be removed along with its related utilities
Based on the search results, the RRule class is deeply integrated into the reminder functionality of the application. The class is used in:
ui/main/src/app/services/reminder/RruleReminderUtils.ts- Core utility for handling recurrence rulesnode-services/cards-reminder/- Backend reminder service implementation- Several test files that verify recurrence functionality
The evidence shows this class is specifically designed for recurrence functionality, with no other uses found. Since the PR's objective is to remove recurrence, this class and its related components should be removed:
ui/main/src/app/business/model/card.model.ts: Remove the RRule classui/main/src/app/services/reminder/RruleReminderUtils.ts: Remove the entire filenode-services/cards-reminder/src/domain/application/rrule-reminderUtils.ts: Remove the entire filenode-services/cards-reminder/src/domain/application/rruleReminderService.ts: Remove or refactor to handle non-recurring reminders only- Update all related test files to remove recurrence-related test cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for RRule usage in the codebase
ast-grep --pattern 'new RRule($$$)'
rg "RRule" --type ts -A 5
Length of output: 52188
ui/main/src/app/modules/calendar/calendar.component.ts (2)
101-112: Event creation logic simplified correctly
The calendar event creation logic has been correctly simplified to handle only start and end dates.
Line range hint 124-146: Verify if computeRRuleCalendarEvents should be removed
While timespan-based recurrence has been removed, the component still handles RRule-based recurrence. This seems inconsistent with the PR objective of removing recurrence functionality.
node-services/cards-reminder/src/tests/cardsReminderControl.test.ts (4)
14-14: LGTM: Import statement updated to remove recurrence-related imports.
The changes align with the PR objective of removing recurrence functionality.
Line range hint 465-468: LGTM: TimeSpan test case simplified.
The test case has been updated to use basic time spans with start and end dates, removing recurrence-related complexity.
Line range hint 483-490: Verify test coverage for recurrence removal.
The test case title still mentions "recurrence deprecated field" but the implementation no longer tests recurrence functionality. Consider updating the test case title to reflect the current behavior.
Let's verify if there are any remaining references to recurrence:
#!/bin/bash
# Description: Search for remaining references to recurrence in test files.
# Search for "recurrence" in test file names
fd -t f ".*\.test\.(ts|js)" -x rg -l "recurrence"
# Search for recurrence-related test case descriptions
rg -i "describe.*recurrence" "**/*test*"Line range hint 497-500: LGTM: TimeSpan test case handles calendar view correctly.
The test case correctly verifies that timeSpans from specific card information take precedence over calendar view settings.
ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts (1)
25-25: LGTM: Import statement updated to remove recurrence import.
The changes align with the PR objective of removing recurrence functionality.
vlo-rte
force-pushed
the
FE-7499-RemoveUseOfRecurrenceInOpfab
branch
from
dce3ba6 to
6353518
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
ui/main/src/app/modules/calendar/calendar.component.ts (1)
RRule functionality should be removed as part of recurrence removal
Based on the search results, RRule functionality is deeply integrated into the codebase for handling recurring events:
- The
RRuleclass is imported and used in multiple files across the UI and node services- There are extensive test files specifically for RRule functionality
- The calendar component uses RRule for displaying recurring events
- The reminder service heavily relies on RRule for scheduling recurring reminders
- There's even a migration tool specifically for RRule recurrence at
src/tooling/migration-rrule-recurrence/Since the PR's objective is to remove recurrence functionality, all RRule-related code should be removed, including:
- The
computeRRuleCalendarEventsmethod in the calendar component- RRule-related imports and models
- RRule reminder utilities and services
- Associated tests and migration tools
🔗 Analysis chain
Line range hint
122-146: Verify if RRule functionality should be retained.Since the PR objective is to remove recurrence, we should verify if RRule-based recurring events should be kept.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for RRule usage across the codebase echo "Searching for RRule imports..." rg "import.*RRule" echo "Searching for RRule instantiation..." ast-grep --pattern "new RRule" echo "Searching for rRule property usage..." rg "\.rRule"Length of output: 31748
🧹 Nitpick comments (3)
node-services/cards-reminder/src/domain/application/reminderUtils.ts (1)
33-37: Simplify the time comparison logic and consider renaming the function.The function no longer handles repeating events, so its name could be misleading. Also, the logic can be more concise.
-function getNextTimeForRepeatingFromTimeSpan(timeSpan: TimeSpan, startingDate?: number): number { +function getNextValidTimeFromTimeSpan(timeSpan: TimeSpan, startingDate: number = new Date().valueOf()): number { - if (startingDate == null) { - startingDate = new Date().valueOf(); - } - - if (timeSpan.start < startingDate) { - return -1; - } else { - return timeSpan.start; - } + return timeSpan.start < startingDate ? -1 : timeSpan.start; }ui/main/src/app/modules/calendar/calendar.component.ts (1)
101-112: Add defensive null checks for card properties.While the event creation logic is correct, it could benefit from more defensive programming.
this.calendarEvents = this.calendarEvents.concat({ id: card.id, - title: card.titleTranslated, + title: card.titleTranslated ?? card.id, start: startDate, end: endDate, className: [ 'opfab-calendar-event', - 'opfab-calendar-event-' + card.severity.toLowerCase() + 'opfab-calendar-event-' + (card.severity?.toLowerCase() ?? 'default') ], allDay: false });services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java (1)
Line range hint
1-924: Consider adding migration-related test cases.The test suite comprehensively covers the updated TimeSpan behavior. However, consider adding test cases to verify:
- Backward compatibility with stored cards that might have recurrence data
- Migration scenarios where old cards with recurrence are updated
- Error handling when receiving cards with legacy recurrence data through APIs
This will ensure smooth transition for existing deployments.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
client/cards/src/main/avro/timeSpan.avdl(0 hunks)node-services/cards-reminder/src/domain/application/reminderUtils.ts(2 hunks)node-services/cards-reminder/src/domain/model/card.model.ts(1 hunks)node-services/cards-reminder/src/tests/cardsReminderControl.test.ts(1 hunks)node-services/cards-reminder/src/tests/reminderUtils.test.ts(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/HoursAndMinutes.java(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/Recurrence.java(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/TimeSpan.java(1 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/HoursAndMinutes.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/Recurrence.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java(1 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardValidationService.java(0 hunks)services/cards-publication/src/main/modeling/swagger.yaml(0 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/kafka/integration/KafkaSerializeDeserializeShould.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardPermissionControlServiceShould.java(7 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java(4 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/ExternalAppServiceShould.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java(2 hunks)src/docs/asciidoc/reference_doc/card_notification.adoc(0 hunks)src/docs/asciidoc/reference_doc/user_cards.adoc(0 hunks)src/test/api/karate/cards/cardsReminder.feature(0 hunks)src/test/api/karate/cards/cardsWithTimespans.feature(7 hunks)ui/main/src/app/business/model/card.model.ts(1 hunks)ui/main/src/app/business/view/timeline/circles.spec.ts(2 hunks)ui/main/src/app/business/view/timeline/circles.ts(1 hunks)ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts(3 hunks)ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts(1 hunks)ui/main/src/app/modules/calendar/calendar.component.ts(1 hunks)ui/main/src/app/services/reminder/ReminderUtils.spec.ts(0 hunks)ui/main/src/app/services/reminder/ReminderUtils.ts(0 hunks)ui/main/src/app/services/reminder/RruleReminderUtils.ts(1 hunks)
💤 Files with no reviewable changes (13)
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/Recurrence.java
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/HoursAndMinutes.java
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/Recurrence.java
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/HoursAndMinutes.java
- src/docs/asciidoc/reference_doc/card_notification.adoc
- src/docs/asciidoc/reference_doc/user_cards.adoc
- client/cards/src/main/avro/timeSpan.avdl
- ui/main/src/app/services/reminder/ReminderUtils.spec.ts
- services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardValidationService.java
- src/test/api/karate/cards/cardsReminder.feature
- services/cards-publication/src/main/modeling/swagger.yaml
- node-services/cards-reminder/src/tests/reminderUtils.test.ts
- ui/main/src/app/services/reminder/ReminderUtils.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/TimeSpan.java
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/ExternalAppServiceShould.java
- node-services/cards-reminder/src/domain/model/card.model.ts
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardPermissionControlServiceShould.java
- ui/main/src/app/services/reminder/RruleReminderUtils.ts
- ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts
- services/cards-publication/src/test/java/org/opfab/cards/publication/kafka/integration/KafkaSerializeDeserializeShould.java
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java
- src/test/api/karate/cards/cardsWithTimespans.feature
- ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts
🔇 Additional comments (8)
ui/main/src/app/business/model/card.model.ts (2)
116-116: LGTM! TimeSpan simplification aligns with recurrence removal.
The simplified TimeSpan constructor correctly removes the recurrence property.
Line range hint 120-124: Verify if HourAndMinutes class is still needed.
Since recurrence functionality has been removed, this class might be unused.
✅ Verification successful
Let me check the actual usage of HourAndMinutes in these files to determine if it's still needed.
The HourAndMinutes class is still actively used in task-related templates
The class is still needed as it's being used in several task-related template files for time handling:
- Used in task card templates for displaying and managing time values
- Referenced in template view files (
taskCardTemplateView.ts,taskUserCardTemplateView.ts) - Used in template tests for time-related assertions
- Imported and used in
cardBuilder.spec.ts
While the recurrence functionality has been removed, this class appears to serve a broader purpose in handling time representations for tasks in the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of HourAndMinutes class
rg -l "HourAndMinutes"
ast-grep --pattern "new HourAndMinutes"
Length of output: 608
Script:
#!/bin/bash
# Check the actual usage context of HourAndMinutes in the files
rg "HourAndMinutes" -B2 -A2
Length of output: 6101
ui/main/src/app/business/view/timeline/circles.ts (1)
51-51: LGTM! Simplified card processing logic.
The direct call to computeCardForTimeLine aligns with the removal of recurrence functionality, making the code more maintainable by consolidating the card processing logic.
ui/main/src/app/business/view/timeline/circles.spec.ts (1)
14-14: LGTM! Test cases updated to remove recurrence.
The changes correctly align the test cases with the removal of recurrence functionality by:
- Removing the
Recurrenceimport - Updating
TimeSpanconstructor calls to remove the recurrence parameter
Also applies to: 224-225
node-services/cards-reminder/src/tests/cardsReminderControl.test.ts (1)
14-14: LGTM! Test suite updated to focus on core reminder functionality.
The changes maintain comprehensive test coverage while removing recurrence-related dependencies, ensuring the reminder system works correctly with both RRule and timespan-based reminders.
Also applies to: 16-16
services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java (3)
491-491: TimeSpan constructor usage is consistent.
The TimeSpan instantiation in the child card test case has been properly updated to match the new signature.
512-512: TimeSpan constructor usage in parent-child card tests is correct.
Both parent and child card TimeSpan instantiations have been updated consistently to use the simplified constructor without recurrence.
Let's verify the test coverage for the updated TimeSpan behavior in parent-child scenarios:
Also applies to: 525-525
✅ Verification successful
TimeSpan usage in parent-child card tests is properly verified
Based on the search results, I can confirm that the test coverage for TimeSpan in parent-child scenarios is comprehensive. The tests include:
- Tests for child cards with TimeSpan when parent card doesn't exist
- Tests for child cards with TimeSpan when initialParentCardUid doesn't exist
- Tests verifying correct start/end date handling between parent and child cards
- Tests for updating parent cards with KEEP_CHILD_CARDS action and its impact on child card dates
The TimeSpan constructor usage is consistent across these test cases, using the simplified constructor without recurrence.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for parent-child card scenarios with TimeSpan
# Test: Search for test methods involving both parent-child relationships and TimeSpan.
# Expect: Comprehensive test coverage of TimeSpan in parent-child scenarios.
rg -l 'parentCard.*TimeSpan|TimeSpan.*parentCard' --type java
Length of output: 63
Script:
#!/bin/bash
# Let's try a broader search to understand the test coverage
# First, let's find all files containing TimeSpan usage
rg -l "TimeSpan" --type java
# Then, let's look at the actual test content in the specific file
rg -A 10 -B 10 "parentCard" services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java
# Also search for child card tests
rg -A 10 -B 10 "childCard" services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java
Length of output: 20163
453-453: TimeSpan constructor usage looks good.
The TimeSpan instantiation has been correctly updated to remove the recurrence parameter, aligning with the broader changes to remove recurrence functionality.
Let's verify that all TimeSpan instantiations in the test suite have been updated consistently:
✅ Verification successful
All TimeSpan instantiations have been updated correctly
Based on the search results, all TimeSpan instantiations across the codebase are using the correct two-parameter constructor (start, end) without the recurrence parameter. The changes are consistent across:
- Test files in cards-publication service
- Test files in cards-reminder service
- UI components and tests
- Integration tests
Let's verify the TimeSpan class definition to confirm the constructor signature:
Let's find the Java TimeSpan class since we're reviewing Java code:
TimeSpan usage is correctly aligned with the class definition
The verification confirms that:
- The TimeSpan class is now a Java record with just two parameters:
start(required) andend(optional) - All TimeSpan instantiations across the codebase use the correct two-parameter constructor
- The test class
CardProcessServiceShould.javaand all other test files are using the correct constructor signature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining TimeSpan instantiations that might still use recurrence
# Test: Search for TimeSpan constructor calls. Expect: Only two-parameter constructor calls (start, end).
ast-grep --pattern 'new TimeSpan($$$)'
Length of output: 5101
Script:
#!/bin/bash
# Find and show the TimeSpan class definition
ast-grep --pattern 'class TimeSpan {
$$$
}'
Length of output: 954
Script:
#!/bin/bash
# Find the Java TimeSpan class definition
fd TimeSpan.java
Length of output: 198
Script:
#!/bin/bash
# Show the Java TimeSpan class definition
cat services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java
Length of output: 740
vlo-rte
force-pushed
the
FE-7499-RemoveUseOfRecurrenceInOpfab
branch
from
6353518 to
366cfd4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
node-services/cards-reminder/src/domain/application/reminderUtils.ts (1)
33-37: Consider renaming the function to better reflect its current behavior.The function name
getNextTimeForRepeatingFromTimeSpansuggests repetition handling, but after removing recurrence, it simply checks if the time span is in the future. Consider renaming to something likeisTimeSpanInFutureorgetValidTimeSpanStart.-function getNextTimeForRepeatingFromTimeSpan(timeSpan: TimeSpan, startingDate?: number): number { +function getValidTimeSpanStart(timeSpan: TimeSpan, startingDate?: number): number {node-services/cards-reminder/src/tests/cardsReminderControl.test.ts (1)
Line range hint
67-308: Consider documenting the transition from recurrence to time spans.While the test suite thoroughly validates RRule-based reminders, it would be helpful to add a comment explaining that this test suite represents the legacy behavior being replaced by time spans. This documentation would help future developers understand the evolution of the reminder system.
Add a comment at the start of the test suite:
+// Legacy test suite for RRule-based reminders. +// Note: This functionality is being replaced by simpler time span-based reminders +// as part of the recurrence removal initiative. describe('Cards reminder with rrule structure', function () {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
client/cards/src/main/avro/timeSpan.avdl(0 hunks)node-services/cards-reminder/src/domain/application/reminderUtils.ts(2 hunks)node-services/cards-reminder/src/domain/model/card.model.ts(1 hunks)node-services/cards-reminder/src/tests/cardsReminderControl.test.ts(1 hunks)node-services/cards-reminder/src/tests/reminderUtils.test.ts(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/HoursAndMinutes.java(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/Recurrence.java(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/TimeSpan.java(1 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/HoursAndMinutes.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/Recurrence.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java(1 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardValidationService.java(0 hunks)services/cards-publication/src/main/modeling/swagger.yaml(0 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/kafka/integration/KafkaSerializeDeserializeShould.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardPermissionControlServiceShould.java(7 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java(4 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/ExternalAppServiceShould.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java(2 hunks)src/docs/asciidoc/reference_doc/card_notification.adoc(0 hunks)src/docs/asciidoc/reference_doc/user_cards.adoc(0 hunks)src/test/api/karate/cards/cardsReminder.feature(0 hunks)src/test/api/karate/cards/cardsWithTimespans.feature(7 hunks)ui/main/src/app/business/model/card.model.ts(1 hunks)ui/main/src/app/business/view/timeline/circles.spec.ts(2 hunks)ui/main/src/app/business/view/timeline/circles.ts(1 hunks)ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts(3 hunks)ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts(1 hunks)ui/main/src/app/modules/calendar/calendar.component.ts(1 hunks)ui/main/src/app/services/reminder/ReminderUtils.spec.ts(0 hunks)ui/main/src/app/services/reminder/ReminderUtils.ts(0 hunks)ui/main/src/app/services/reminder/RruleReminderUtils.ts(1 hunks)
💤 Files with no reviewable changes (13)
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/HoursAndMinutes.java
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/Recurrence.java
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/Recurrence.java
- src/docs/asciidoc/reference_doc/card_notification.adoc
- ui/main/src/app/services/reminder/ReminderUtils.spec.ts
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/HoursAndMinutes.java
- client/cards/src/main/avro/timeSpan.avdl
- src/test/api/karate/cards/cardsReminder.feature
- node-services/cards-reminder/src/tests/reminderUtils.test.ts
- services/cards-publication/src/main/modeling/swagger.yaml
- services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardValidationService.java
- src/docs/asciidoc/reference_doc/user_cards.adoc
- ui/main/src/app/services/reminder/ReminderUtils.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/TimeSpan.java
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/ExternalAppServiceShould.java
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardPermissionControlServiceShould.java
- node-services/cards-reminder/src/domain/model/card.model.ts
- ui/main/src/app/services/reminder/RruleReminderUtils.ts
- services/cards-publication/src/test/java/org/opfab/cards/publication/kafka/integration/KafkaSerializeDeserializeShould.java
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java
- ui/main/src/app/business/view/timeline/circles.ts
- ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts
- ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts
- src/test/api/karate/cards/cardsWithTimespans.feature
- ui/main/src/app/business/view/timeline/circles.spec.ts
🔇 Additional comments (10)
node-services/cards-reminder/src/domain/application/reminderUtils.ts (1)
10-10: LGTM: Import statement is correctly maintained.
The import statement has been correctly updated to only include the necessary types after removing recurrence.
ui/main/src/app/business/model/card.model.ts (2)
116-116: LGTM: TimeSpan class correctly simplified.
The TimeSpan class has been correctly simplified to remove the recurrence property, aligning with the PR objective.
Line range hint 120-125: Verify if HourAndMinutes class is still needed.
The HourAndMinutes class might be obsolete after removing recurrence from TimeSpan. Let's verify if it's still used elsewhere in the codebase.
✅ Verification successful
Let me check the actual usage of HourAndMinutes in these files to determine if it's still needed.
The HourAndMinutes class is still needed and actively used
The class is still being used in the task card templates for time-related functionality:
- Used in
taskCardTemplate.tsfor displaying task timing - Used in
taskCardTemplateView.tsand its spec for time formatting - Used in
taskUserCardTemplateView.tsand its spec for handling task schedules
While the class might have been originally created with recurrence functionality in mind, it's still serving an important role in time representation and formatting across the task card templates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for HourAndMinutes usage in the codebase
echo "Searching for HourAndMinutes class usage..."
rg "HourAndMinutes" -l
# Search for any hour and minute related patterns that might use this class
echo "Searching for hour/minute patterns..."
ast-grep --pattern 'new HourAndMinutes($_, $_)'
Length of output: 727
Script:
#!/bin/bash
# Check the actual usage of HourAndMinutes in the found files
echo "Checking HourAndMinutes usage in taskUserCardTemplateView.ts..."
rg "HourAndMinutes" ui/main/src/app/builtInTemplates/task/usercard/taskUserCardTemplateView.ts -B 2 -A 2
echo -e "\nChecking HourAndMinutes usage in taskCardTemplate.ts..."
rg "HourAndMinutes" ui/main/src/app/builtInTemplates/task/card/taskCardTemplate.ts -B 2 -A 2
echo -e "\nChecking HourAndMinutes usage in taskCardTemplateView.ts..."
rg "HourAndMinutes" ui/main/src/app/builtInTemplates/task/card/taskCardTemplateView.ts -B 2 -A 2
echo -e "\nChecking HourAndMinutes usage in test files..."
rg "HourAndMinutes" ui/main/src/app/builtInTemplates/task/card/taskCardTemplateView.spec.ts -B 2 -A 2
rg "HourAndMinutes" ui/main/src/app/builtInTemplates/task/usercard/taskUserCardTemplateView.spec.ts -B 2 -A 2
Length of output: 2834
ui/main/src/app/modules/calendar/calendar.component.ts (2)
101-112: LGTM: Event creation logic correctly simplified.
The event creation logic has been properly simplified to handle only basic time spans without recurrence. The code maintains clean separation between time span and RRule handling.
101-112: Verify integration between time span and RRule events.
The code maintains two separate paths for creating calendar events: one for time spans and another for RRule. Let's verify that this separation doesn't cause any duplicate events or conflicts.
Also applies to: 124-147
✅ Verification successful
Let me gather more information about the calendar event handling logic.
No conflict between timeSpans and RRule event handling
The code inspection reveals that timeSpans and RRule events are handled in separate, mutually exclusive code blocks. The implementation shows:
- TimeSpans events are created with explicit start and end dates from the timespan array
- RRule events are created with different properties (duration, rrule configuration) and don't overlap with timespan events
- Each type of event maintains its own unique properties and structure
- The events are added to the same calendar array using concat, but their creation logic ensures they represent different types of occurrences
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any potential overlap between timeSpans and rRule handling
echo "Searching for timeSpans usage with rRule..."
ast-grep --pattern 'if ($card.timeSpans) {
$$$
if ($card.rRule) {
$$$
}
}'
# Search for any safeguards against duplicate events
echo "Searching for duplicate event prevention..."
rg "calendarEvents.*concat" -A 5
Length of output: 1679
Script:
#!/bin/bash
# Search for the complete calendar event handling logic
echo "Searching for timeSpans and rRule handling..."
rg "timeSpans|rRule" -A 10 ui/main/src/app/modules/calendar/calendar.component.ts
# Search for calendar event initialization
echo "Searching for calendarEvents initialization..."
ast-grep --pattern 'calendarEvents = $_;'
Length of output: 2053
node-services/cards-reminder/src/tests/cardsReminderControl.test.ts (2)
14-16: LGTM! Import statements correctly updated.
The removal of HourAndMinutes and CardOperation imports aligns with the PR's objective of removing recurrence functionality.
Line range hint 309-467: LGTM! Well-structured test suite for time span-based reminders.
The new test suite effectively validates the simplified reminder system using time spans. It maintains feature parity with the previous RRule-based implementation while being more straightforward and easier to understand.
services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java (3)
453-453: LGTM! TimeSpan constructor updated correctly.
The removal of the recurrence parameter from the TimeSpan constructor is consistent with the PR's objective of removing recurrence functionality.
491-491: LGTM! TimeSpan constructor updated consistently.
The TimeSpan constructor call is correctly updated to remove the recurrence parameter while maintaining the test's purpose of validating parent card relationships.
512-512: LGTM! TimeSpan constructors updated consistently.
The TimeSpan constructor calls are correctly updated to remove the recurrence parameter while maintaining the test's purpose of validating initial parent card relationships.
Also applies to: 525-525
vlo-rte
force-pushed
the
FE-7499-RemoveUseOfRecurrenceInOpfab
branch
from
366cfd4 to
a32f19a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
node-services/cards-reminder/src/tests/cardsReminderControl.test.ts (1)
Test coverage for edge cases needs improvement
Based on the code analysis, while there are tests for basic TimeSpan and RRule scenarios separately, the following edge cases are not adequately covered:
- Multiple time spans with overlapping reminder times - only basic multiple time spans testing is present
- Time spans with very small gaps between them - no specific tests found
- Cards with both RRule and time spans defined - no tests found for this mixed scenario
These edge cases could potentially reveal timing-related bugs or race conditions in the reminder system.
Locations needing test coverage:
node-services/cards-reminder/src/tests/cardsReminderControl.test.ts: Add test suite section for mixed RRule and TimeSpan scenariosnode-services/cards-reminder/src/tests/cardsReminderControl.test.ts: Enhance "Cards reminder with timespans and no recurrence" section with overlapping and small-gap scenarios🔗 Analysis chain
Line range hint
14-300: Verify test coverage for edge cases.The test suite comprehensively covers the basic scenarios for both RRule and TimeSpan-based reminders. However, consider adding tests for the following edge cases:
- Multiple time spans with overlapping reminder times
- Time spans with very small gaps between them
- Cards with both RRule and time spans defined
Let me check for more specific test coverage related to the edge cases.
Let me try one more search to find test cases with multiple time spans.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check existing test coverage for edge cases rg -A 5 "describe\(" "node-services/cards-reminder/src/tests/"Length of output: 5054
Script:
#!/bin/bash # Check for tests involving multiple time spans and mixed RRule/timespan scenarios rg -B 2 -A 5 "timespans.*=.*\[" "node-services/cards-reminder/src/tests/"Length of output: 1788
Script:
#!/bin/bash # Search for test cases involving both RRule and timespans ast-grep --pattern 'const $_ = { $$$ rRule: $_, timespans: $_, $$$ }'Length of output: 79
Script:
#!/bin/bash # Look for test cases with overlapping time spans rg -A 5 "TimeSpan.*new.*Date.*" "node-services/cards-reminder/src/tests/"Length of output: 73
Script:
#!/bin/bash # Search for test cases with multiple time spans rg -B 5 -A 10 "timeSpans.*=.*\[.*TimeSpan.*,.*TimeSpan" "node-services/cards-reminder/src/tests/"Length of output: 97
🧹 Nitpick comments (1)
ui/main/src/app/modules/calendar/calendar.component.ts (1)
101-112: Add validation and type safety for calendar event creation.While the event creation logic is correct, consider adding the following improvements:
- Validate that startDate is before endDate
- Add type safety by creating interfaces for the card and timespan objects
- Add null/undefined checks for the date values
Here's a suggested implementation:
interface CardTimespan { start: number; end: number; } interface CalendarCard { id: string; titleTranslated: string; severity: string; timeSpans?: CardTimespan[]; } private createCalendarEvent(card: CalendarCard, timespan: CardTimespan): EventInput { const startDate = new Date(timespan.start); const endDate = new Date(timespan.end); if (isNaN(startDate.getTime()) || isNaN(endDate.getTime())) { console.warn(`Invalid dates for card ${card.id}`); return null; } if (startDate >= endDate) { console.warn(`Start date is not before end date for card ${card.id}`); return null; } return { id: card.id, title: card.titleTranslated, start: startDate, end: endDate, className: [ 'opfab-calendar-event', 'opfab-calendar-event-' + card.severity.toLowerCase() ], allDay: false }; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
client/cards/src/main/avro/timeSpan.avdl(0 hunks)node-services/cards-reminder/src/domain/application/reminderUtils.ts(2 hunks)node-services/cards-reminder/src/domain/model/card.model.ts(1 hunks)node-services/cards-reminder/src/tests/cardsReminderControl.test.ts(1 hunks)node-services/cards-reminder/src/tests/reminderUtils.test.ts(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/HoursAndMinutes.java(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/Recurrence.java(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/TimeSpan.java(1 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/HoursAndMinutes.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/Recurrence.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java(1 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardValidationService.java(0 hunks)services/cards-publication/src/main/modeling/swagger.yaml(0 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/kafka/integration/KafkaSerializeDeserializeShould.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardPermissionControlServiceShould.java(7 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java(4 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/ExternalAppServiceShould.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java(2 hunks)src/docs/asciidoc/reference_doc/card_notification.adoc(0 hunks)src/docs/asciidoc/reference_doc/user_cards.adoc(0 hunks)src/test/api/karate/cards/cardsReminder.feature(1 hunks)src/test/api/karate/cards/cardsWithTimespans.feature(7 hunks)ui/main/src/app/business/model/card.model.ts(1 hunks)ui/main/src/app/business/view/timeline/circles.spec.ts(2 hunks)ui/main/src/app/business/view/timeline/circles.ts(1 hunks)ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts(3 hunks)ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts(1 hunks)ui/main/src/app/modules/calendar/calendar.component.ts(1 hunks)ui/main/src/app/services/reminder/ReminderUtils.spec.ts(0 hunks)ui/main/src/app/services/reminder/ReminderUtils.ts(0 hunks)ui/main/src/app/services/reminder/RruleReminderUtils.ts(1 hunks)
💤 Files with no reviewable changes (12)
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/HoursAndMinutes.java
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/Recurrence.java
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/HoursAndMinutes.java
- ui/main/src/app/services/reminder/ReminderUtils.spec.ts
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/Recurrence.java
- src/docs/asciidoc/reference_doc/card_notification.adoc
- client/cards/src/main/avro/timeSpan.avdl
- node-services/cards-reminder/src/tests/reminderUtils.test.ts
- src/docs/asciidoc/reference_doc/user_cards.adoc
- ui/main/src/app/services/reminder/ReminderUtils.ts
- services/cards-publication/src/main/modeling/swagger.yaml
- services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardValidationService.java
🚧 Files skipped from review as they are similar to previous changes (15)
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/TimeSpan.java
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/ExternalAppServiceShould.java
- services/cards-publication/src/test/java/org/opfab/cards/publication/kafka/integration/KafkaSerializeDeserializeShould.java
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java
- node-services/cards-reminder/src/domain/model/card.model.ts
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardPermissionControlServiceShould.java
- ui/main/src/app/services/reminder/RruleReminderUtils.ts
- src/test/api/karate/cards/cardsWithTimespans.feature
- src/test/api/karate/cards/cardsReminder.feature
- ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts
- ui/main/src/app/business/view/timeline/circles.ts
- ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts
- node-services/cards-reminder/src/domain/application/reminderUtils.ts
- ui/main/src/app/business/view/timeline/circles.spec.ts
🔇 Additional comments (7)
services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java (3)
453-453: LGTM: TimeSpan constructor updated correctly.
The removal of the recurrence parameter from the TimeSpan constructor aligns with the PR's objective of removing recurrence functionality.
491-491: LGTM: TimeSpan constructor updated correctly in parent-child validation test.
The removal of the recurrence parameter from the TimeSpan constructor maintains the test's integrity while aligning with the recurrence removal objective.
512-512: LGTM: TimeSpan constructors updated correctly in parent UID validation test.
Both TimeSpan constructor calls have been properly updated to remove the recurrence parameter while maintaining the test's purpose of validating parent-child relationships through UIDs.
Also applies to: 525-525
ui/main/src/app/modules/calendar/calendar.component.ts (1)
Line range hint 138-161: Clarify the architectural approach to recurrence handling.
While the regular recurrence functionality has been removed from processCards, the computeRRuleCalendarEvents method still handles RRule-based recurrence. This creates an architectural inconsistency where some forms of recurrence are removed while others remain.
Please clarify:
- Is RRule-based recurrence intended to replace the removed recurrence functionality?
- Should
computeRRuleCalendarEventsalso be removed as part of this change? - If both mechanisms are needed, what are the specific use cases for each?
Let's check for any remaining recurrence-related code:
✅ Verification successful
RRule-based recurrence is the intended replacement for the removed recurrence functionality
Based on the search results, it's clear that this is part of a planned migration from the old recurrence system to RRule-based recurrence:
- The presence of
src/tooling/migration-rrule-recurrence/directory with migration scripts shows this is an intentional architectural change. - The old recurrence system used
timeSpans[0].recurrenceformat, while the new system uses therRuleproperty. - The RRule functionality is well integrated across the codebase:
- Calendar component uses
@fullcalendar/rruleplugin - Dedicated reminder utilities in
RruleReminderUtils.ts - RRule-specific database collections and services in the cards-reminder service
- Calendar component uses
The architectural approach is consistent, and computeRRuleCalendarEvents should be kept as it's part of the new RRule-based recurrence system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for recurrence-related code in TypeScript/JavaScript files
rg -t ts -t js -i "recurr|rrule" --glob '!{*.test.*,*.spec.*}'
Length of output: 26576
ui/main/src/app/business/model/card.model.ts (2)
116-116: LGTM! TimeSpan class simplified.
The TimeSpan class has been correctly simplified by removing the recurrence parameter, aligning with the PR objective of removing recurrence functionality.
Line range hint 119-124: Consider removing unused HourAndMinutes class.
Since recurrence functionality has been removed from the codebase, the HourAndMinutes class might no longer be needed. Consider removing it if it's not used elsewhere.
✅ Verification successful
Let me check how HourAndMinutes is being used in these files to determine if it's actually needed.
The HourAndMinutes class appears to be unused and can be safely removed
The search results show that while there are methods named getHourAndMinutes() and getByHourAndMinutes() in various template files, these methods only return string values in the format "HH:mm". The actual HourAndMinutes class defined in card.model.ts is never instantiated (no new HourAndMinutes() was found in the codebase). The time-related functionality in the templates is handled using native JavaScript Date objects and string formatting.
- Remove the
HourAndMinutesclass fromui/main/src/app/business/model/card.model.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if HourAndMinutes class is used anywhere in the codebase
ast-grep --pattern 'new HourAndMinutes($$$)'
rg -l "HourAndMinutes"
Length of output: 542
Script:
#!/bin/bash
# Check the actual usage of HourAndMinutes in the files
rg "HourAndMinutes" -A 3 -B 3
Length of output: 7357
node-services/cards-reminder/src/tests/cardsReminderControl.test.ts (1)
14-14: LGTM! Imports correctly updated.
The import statements have been properly updated to remove recurrence-related imports while maintaining the necessary imports for RRule and TimeSpan functionality.
Also applies to: 16-16
vlo-rte
force-pushed
the
FE-7499-RemoveUseOfRecurrenceInOpfab
branch
from
a32f19a to
cf2ad1c
Compare
Signed-off-by: vlo-rte <[email protected]>
vlo-rte
force-pushed
the
FE-7499-RemoveUseOfRecurrenceInOpfab
branch
from
cf2ad1c to
bd9c67b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
ui/main/src/app/business/view/timeline/circles.spec.ts (1)
224-225: LGTM! TimeSpan constructor calls updated correctly.The TimeSpan constructor calls have been properly updated to remove the recurrence parameter, aligning with the architectural changes.
However, consider improving the test case name and description for better clarity:
- it('set two circle with if one card with two timespans', () => { + it('should display two circles when a card has two time spans', () => {src/docs/asciidoc/resources/migration_guide_to_4.6.adoc (3)
Line range hint
9-11: Enhance the deprecated field migration stepsThe section about
toNotifydeprecation would benefit from:
- Example showing the old vs new configuration
- Clear steps for migrating existing cards
- Information about backward compatibility during migration
Consider adding a code block like this:
- "toNotify": false + "actions": ["STORE_ONLY_IN_ARCHIVES"]
Line range hint
43-63: Clarify email configuration migration detailsPlease enhance this section by:
- Specifying if existing daily email configurations need manual updates
- Explaining that
dayOfWeekToSendWeeklyRecapEmail: 1represents Monday (1) through Sunday (7)- Adding migration steps for users with custom email configurations
114-114: Remove stray line number from email template exampleThere's a stray line number
114~in the email template example that should be removed.ui/main/src/app/modules/calendar/calendar.component.ts (1)
Line range hint
134-157: Document the architectural decision regarding recurrence handling.While custom recurrence handling has been removed, the code retains support for iCalendar RRule-based recurring events. This is a good architectural decision as it:
- Leverages a widely adopted standard (iCalendar RRule)
- Improves maintainability by removing custom implementation
- Provides robust recurrence handling through the FullCalendar library
Consider adding a comment in the code to document this architectural decision:
private computeRRuleCalendarEvents(card: any) { + // Note: Custom recurrence implementation has been replaced with standard iCalendar RRule support + // for better maintainability and standardization if (card.rRule) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
client/cards/src/main/avro/timeSpan.avdl(0 hunks)node-services/cards-reminder/src/domain/application/reminderUtils.ts(2 hunks)node-services/cards-reminder/src/domain/model/card.model.ts(1 hunks)node-services/cards-reminder/src/tests/cardsReminderControl.test.ts(1 hunks)node-services/cards-reminder/src/tests/reminderUtils.test.ts(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/HoursAndMinutes.java(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/Recurrence.java(0 hunks)services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/TimeSpan.java(1 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/HoursAndMinutes.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/Recurrence.java(0 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java(1 hunks)services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardValidationService.java(0 hunks)services/cards-publication/src/main/modeling/swagger.yaml(0 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/kafka/integration/KafkaSerializeDeserializeShould.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardPermissionControlServiceShould.java(7 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java(4 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/ExternalAppServiceShould.java(1 hunks)services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java(2 hunks)src/docs/asciidoc/reference_doc/card_notification.adoc(0 hunks)src/docs/asciidoc/reference_doc/user_cards.adoc(0 hunks)src/docs/asciidoc/resources/migration_guide_to_4.6.adoc(1 hunks)src/test/api/karate/cards/cardsReminder.feature(1 hunks)src/test/api/karate/cards/cardsWithTimespans.feature(7 hunks)src/test/cypress/cypress/integration/UserCard.spec.js(0 hunks)ui/main/src/app/business/model/card.model.ts(1 hunks)ui/main/src/app/business/view/timeline/circles.spec.ts(2 hunks)ui/main/src/app/business/view/timeline/circles.ts(1 hunks)ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts(3 hunks)ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts(1 hunks)ui/main/src/app/modules/calendar/calendar.component.ts(1 hunks)ui/main/src/app/services/reminder/ReminderUtils.spec.ts(0 hunks)ui/main/src/app/services/reminder/ReminderUtils.ts(0 hunks)ui/main/src/app/services/reminder/RruleReminderUtils.ts(1 hunks)
💤 Files with no reviewable changes (13)
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/HoursAndMinutes.java
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/HoursAndMinutes.java
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/Recurrence.java
- src/test/cypress/cypress/integration/UserCard.spec.js
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/Recurrence.java
- src/docs/asciidoc/reference_doc/card_notification.adoc
- client/cards/src/main/avro/timeSpan.avdl
- ui/main/src/app/services/reminder/ReminderUtils.spec.ts
- services/cards-publication/src/main/modeling/swagger.yaml
- src/docs/asciidoc/reference_doc/user_cards.adoc
- node-services/cards-reminder/src/tests/reminderUtils.test.ts
- services/cards-publication/src/main/java/org/opfab/cards/publication/services/CardValidationService.java
- ui/main/src/app/services/reminder/ReminderUtils.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- services/cards-consultation/src/main/java/org/opfab/cards/consultation/model/TimeSpan.java
- services/cards-publication/src/main/java/org/opfab/cards/publication/model/TimeSpan.java
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/ExternalAppServiceShould.java
- node-services/cards-reminder/src/domain/model/card.model.ts
- services/cards-publication/src/test/java/org/opfab/cards/publication/kafka/integration/KafkaSerializeDeserializeShould.java
- ui/main/src/app/services/reminder/RruleReminderUtils.ts
- src/test/api/karate/cards/cardsReminder.feature
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/TestHelpers.java
- services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardPermissionControlServiceShould.java
- ui/main/src/app/business/view/timeline/circles.ts
- ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.ts
- ui/main/src/app/business/view/userCard/cardBuilder/cardBuilder.spec.ts
- node-services/cards-reminder/src/domain/application/reminderUtils.ts
- src/test/api/karate/cards/cardsWithTimespans.feature
🔇 Additional comments (9)
services/cards-publication/src/test/java/org/opfab/cards/publication/services/CardProcessServiceShould.java (3)
453-453: LGTM: TimeSpan constructor updated correctly.
The removal of the recurrence parameter from the TimeSpan constructor aligns with the PR's objective of removing recurrence functionality.
491-491: LGTM: TimeSpan constructor updated correctly.
The removal of the recurrence parameter from the TimeSpan constructor is consistent with the recurrence removal objective while preserving the test's main purpose of validating parent card references.
512-512: LGTM: TimeSpan constructors updated consistently.
Both TimeSpan constructor calls have been correctly updated to remove the recurrence parameter, maintaining consistency throughout the test case while preserving its main purpose of validating initial parent UID references.
Also applies to: 525-525
ui/main/src/app/business/view/timeline/circles.spec.ts (1)
14-14: LGTM! Import statement correctly updated.
The removal of the Recurrence import aligns with the PR objective of removing recurrence functionality.
ui/main/src/app/modules/calendar/calendar.component.ts (1)
101-112: LGTM! Clean implementation of calendar event creation.
The simplified event creation logic is well-structured and maintains proper integration with FullCalendar. The removal of recurrence handling aligns with the PR objectives while preserving essential functionality.
Let's verify that all recurrence-related code has been properly removed:
✅ Verification successful
Remaining recurrence references found in reminder and task-related code
The search results show that while recurrence handling has been removed from the calendar component, there are still recurrence-related code in other parts of the codebase, specifically in:
- Reminder functionality (
RruleReminderUtils.spec.ts) - Task templates and builders
Let's verify if these are independent features that should maintain their recurrence functionality:
Recurrence removal from calendar is correct, other modules maintain independent recurrence functionality
The verification shows that:
- The calendar component has no direct dependencies on
RruleReminderUtilsor task templates - The remaining recurrence-related code is in independent features:
- Task templates: Uses calendar-related translations for UI labels but maintains its own recurrence logic
- Reminder utils: Handles recurring reminders as a separate feature
The removal of recurrence handling from the calendar component is clean and doesn't impact these independent features that legitimately need their own recurrence functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining recurrence-related code in the UI module
# that might have been missed during the cleanup.
echo "Checking for remaining recurrence references in TypeScript files..."
rg -i "recurrence" "ui/main/src" --type ts
echo "Checking for remaining timespan recurrence properties in templates..."
rg -i "timespan.*recurrence" "ui/main/src" --type html
Length of output: 15442
Script:
#!/bin/bash
# Check if the recurrence in reminder and task is related to calendar functionality
echo "Checking calendar component imports..."
rg -l "RruleReminderUtils|taskCardTemplate" "ui/main/src/app/modules/calendar"
echo "Checking calendar-related interfaces..."
ast-grep --pattern 'interface $_ {
$$$
recurrence
$$$
}'
echo "Checking task template usage of calendar..."
rg "calendar" "ui/main/src/app/builtInTemplates/task" -A 5 -B 5
Length of output: 22633
ui/main/src/app/business/model/card.model.ts (2)
116-116: LGTM! TimeSpan constructor simplified.
The removal of the optional recurrence parameter from TimeSpan aligns with the PR objective of removing recurrence functionality.
Line range hint 1-250: Verify the transition from Recurrence to RRule.
The codebase is transitioning from the old Recurrence class to using RRule for handling recurring events. This is a good architectural decision as it:
- Simplifies the time span model
- Utilizes a standardized recurrence rule format
- Maintains backward compatibility
Let's verify that all usages of the old Recurrence class have been properly migrated:
✅ Verification successful
Let me check the actual usage of Recurrence in those files to verify if they are legacy references or part of the migration:
Let me check for any remaining Recurrence class definitions or imports:
Based on the analysis of the codebase, I can now provide the final response:
The transition from Recurrence to RRule is complete and verified
The codebase has been successfully migrated from the old Recurrence class to using RRule. The evidence shows:
- No
Recurrenceclass definitions exist in the codebase - No type annotations using
Recurrenceremain - All occurrences of "Recurrence" in the codebase are only in:
- Test descriptions documenting RRule behavior
- UI labels for "withoutRecurrence" options
- Helper method names that use RRule internally
The migration appears to be complete and properly implemented, maintaining backward compatibility while adopting the standardized RRule format for recurring events.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old Recurrence class
rg -l "Recurrence" --type ts
# Search for RRule usage to ensure proper migration
rg -l "RRule" --type ts
Length of output: 1601
Script:
#!/bin/bash
# Check actual usage of Recurrence in the files
rg "Recurrence" -A 3 -B 3 --type ts
# Also check for any imports of Recurrence
rg "import.*Recurrence" --type ts
Length of output: 100840
Script:
#!/bin/bash
# Search for Recurrence class definition
ast-grep --pattern 'class Recurrence'
# Search for any remaining Recurrence type annotations
ast-grep --pattern ': Recurrence'
Length of output: 75
node-services/cards-reminder/src/tests/cardsReminderControl.test.ts (2)
14-14: LGTM! Imports cleaned up.
Removed unused imports (HourAndMinutes, Recurrence, CardOperation) and kept only the necessary ones.
Also applies to: 16-16
Line range hint 1-400: Comprehensive test coverage for both RRule and timespan scenarios.
The test suite effectively covers:
- Basic reminder functionality
- Edge cases (null/negative values)
- Multiple cards
- Service reset
- Card lifecycle (updates/deletion)
- Time-based scenarios
This ensures the reliability of the new implementation.
Let's verify the test coverage:
✅ Verification successful
Based on the shell script results, I can now provide the final response:
Test coverage is comprehensive and well-structured
The test suite effectively covers all the core functionality of both RRule and TimeSpan implementations:
-
The
RRuleclass (indomain/model/card.model.ts) and its service implementation (RRuleReminderService) are thoroughly tested with:- Time-based scenarios (different hours, minutes)
- Day-based recurrences (specific days of week)
- Month-based recurrences (first/last days, specific dates)
- Multiple time zones
- Different locales (en, fr)
- Complex combinations (multiple days/months)
- Edge cases (month transitions, leap years)
-
The
TimeSpanclass implementation is adequately tested through the cards reminder tests covering:- Basic start/end time validations
- Multiple timespan scenarios
- Null/undefined end times
- Time-based boundaries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any untested RRule or TimeSpan scenarios
rg -A 5 "class.*RRule|class.*TimeSpan" --type ts | grep -v "test"
# Search for any remaining test files related to recurrence
fd -e test.ts | xargs rg "Recurrence"
Length of output: 12619
| The field `timespans.recurrence` has been removed. So now, to create a recurrent card, you must use the field rRule. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand the recurrence removal migration guide
Since removing recurrence is a major change (main PR objective), this section needs significant expansion:
- Detailed migration steps for existing recurrent cards
- Examples showing how to convert from
timespans.recurrencetorRule - Impact on existing cards and how to handle them
- Any backward compatibility considerations during migration
Consider adding examples like:
- "timespans": {
- "recurrence": {
- "hoursAndMinutes": {"hours": 9, "minutes": 0},
- "daysOfWeek": ["MONDAY", "WEDNESDAY", "FRIDAY"]
- }
- }
+ "rRule": "FREQ=WEEKLY;BYDAY=MO,WE,FR;BYHOUR=9;BYMINUTE=0"Committable suggestion skipped: line range outside the PR's diff.
|
