Conversation
When parsing RRULE and EXRULE, skip rules whose UNTIL ≤ event DTSTART and log a warning. Add unit tests for RRULE/EXRULE with UNTIL before DTSTART and rename the test class.
…ose UNTIL is before DTSTART
There was a problem hiding this comment.
Pull Request Overview
This PR improves handling of invalid recurrence rules by filtering out RRULE and EXRULE instances where the UNTIL date is on or before the event's DTSTART date. The implementation moves validation from EventValidator to RecurrenceFieldsProcessor to prevent invalid rules from being created in the first place.
Key changes:
- Added filtering logic in RecurrenceFieldsProcessor to skip RRULE/EXRULE with invalid UNTIL dates
- Removed the validation and removal logic from EventValidator that previously handled these cases
- Added comprehensive tests to verify the new behavior works correctly
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| RecurrenceFieldsProcessor.kt | Added validation to filter out RRULE/EXRULE with UNTIL <= DTSTART during processing |
| EventValidator.kt | Removed validation methods and repair logic for invalid RRULE UNTIL dates |
| RecurrenceFieldProcessorTest.kt | Added tests for RRULE and EXRULE filtering, fixed class name typo |
| EventValidatorTest.kt | Removed all tests related to UNTIL validation that are no longer needed |
| AndroidCalendarProviderBehaviorTest.kt | Added test to verify invalid RRULE can still be inserted into calendar provider |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
.../test/kotlin/at/bitfire/synctools/mapping/calendar/processor/RecurrenceFieldProcessorTest.kt
Outdated
Show resolved
Hide resolved
.../test/kotlin/at/bitfire/synctools/mapping/calendar/processor/RecurrenceFieldProcessorTest.kt
Show resolved
Hide resolved
sunkup
left a comment
sunkup
left a comment
There was a problem hiding this comment.
Looks good, but see the comment.
...src/main/kotlin/at/bitfire/synctools/mapping/calendar/processor/RecurrenceFieldsProcessor.kt
Outdated
Show resolved
Hide resolved
…ld be dropped too + test
… recurrence properties, so that the generated event is non-recurring
|
Added better handling for the case that there are only invalid RRULEs/RDATEs, which actually happens from time to time (there are calendar apps that generate such rules when editing events in a special way). In such cases, the generated event is non-recurring and thus
@sunkup Re-requesting review, please have a look at the two last commits. |
sunkup
left a comment
sunkup
left a comment
There was a problem hiding this comment.
Looks alright to me, but I think this PR is important enough to require two reviewers - so let's see what @ArnyminerZ says 👍
* [WIP] Split TimeFieldsBuilder into StartTimeBuilder and EndTimeBuilder; make operation more clear * [WIP] Continue with EndTimeBuilder; drop TimeFieldsBuilder * [WIP] Naming changes * Finish EndTimeBuilder * Add DurationBuilder and update EndTimeBuilder for recurring events - Add DurationBuilder to handle duration for recurring events - Update EndTimeBuilder to skip DTEND for recurring events - Add unit tests for both builders - Fix Duration.toRfc5545Duration() for zero seconds * Refactor DurationBuilder to remove debug print and use Duration.ZERO - Remove the `System.err.println` statement that prints `startDate` and `endDate`. - Replace `java.time.Duration.ofSeconds(0)` with `java.time.Duration.ZERO` * Add / refactor tests
ArnyminerZ
left a comment
ArnyminerZ
left a comment
There was a problem hiding this comment.
Actually I had already reviewed this PR. I don't know why it wasn't approved.
Looks good to me otherwise. Quite a lot of code, but seems logical to me.
rfc2822
deleted the
94-drop-eventvalidatorremoverruleswithuntilbeforedtstart
branch
3 participants
Uh oh!
There was an error while loading. Please reload this page.