Avoid NPE when missing DTSTART for recurring events

[ArnyminerZ]

Purpose

Fixes #1265 by correctly handling situations where dtstart is null.

Short description

Handled nullability of DTSTART of recurring instances by setting the local value to null, which would re-create the local instance.

Stop querying VTODO (or VEVENT) recurrence instances with DTSTART, now using their UID and RECURRENCE-ID only.

Checklist

• The PR has a proper title, description and label.
• I have self-reviewed the PR.
• I have added documentation to complex functions and functions that can be used by other modules.
• I have added reasonable tests or consciously decided to not add tests.

[ArnyminerZ]

I've also added a log message so that if we find some data missing we can know what is it all about. If you don't find it necessary we can remove it.

[rfc2822]

Handled nullability of DTSTART of recurring instances by setting the local value to null, which would re-create the local instance.

What would that mean in our case? Do I understand it correctly that

• the main VTODO (without RECURRENCE-ID) is normally created/updated,
• and what happens with the VTODO with RECURRENCE-ID;TZID=America/New_York:20250228T130000? It seems to be added instead of updated in the code – is it deleted before so that it doesn't add a new instance for every sync?

[sunkup]

How about adding a test to verify and document correct behaviour?

[ArnyminerZ]

How about adding a test to verify and document correct behaviour?

I've been looking at how to test this properly, and if I am not wrong, that would mean creating a new test class just like SyncManagerTest, but for JtxSyncManager, and checking whether it can handle those cases correctly right? @rfc2822 can correct me otherwise. I think it's actually a lot of code for just a "side-hustle".

Of course we can add it, but is it really worth it?

[sunkup]

Of course we can add it, but is it really worth it?

I'd say yes definetly - If the test is relevant we should add it and in this case I find it highly relevant to properly understand (#1336 (comment)), clamp and document the newly added behaviour.

[rfc2822]

At least we have a specific iCalendar we could test with… it's surely at least worth the try. If it's absolutely too cumbersome we can still cancel it then

What would that mean #1265 (comment)? Do I understand it correctly that

• the main VTODO (without RECURRENCE-ID) is normally created/updated,
• and what happens with the VTODO with RECURRENCE-ID;TZID=America/New_York:20250228T130000? It seems to be added instead of updated in the code – is it deleted before so that it doesn't add a new instance for every sync?

Did you already had a look at that?

[sunkup]

Not sure what you are trying to test here.

We should unit test processICalObject.

Testing for a recurrence with missing DTSTART should fail when the test is run with the old implementation (because NPE is thrown) and should succeed with the new implementation (the log statement shows).

Furthermore it would be good to have tests documenting/clamping the accompanying behaviour Ricki mentions.

EDIT:

Ah my bad. I think the PR is still work in progress and not ready for review yet @rfc2822

I will set it to draft again.

[sunkup]

Added two tests now. The second one checks that, for a recurring vtodo with exception, but missing dtstart:

• the main vtodo is added with its RRULE
• the exception vtodo from the non-calculated RRULE (because the main vtodo has no DTSTART) is being added with its recurrence ID and timezone

For the tests to run successfully on CI we will need to install jtxBoard before running tests. Would it be very hard to setup a gradle task which downloads and installs the latest jtxboard version before the tests run?

[sunkup]

Gradle task for installing jtxBoard automatically not possible yet. For now: Add assume statement before grant permission rule, so tests only run locally and are skipped in CI.

[rfc2822]

There's still a conflict

[sunkup]

Ah, sorry about that. Fixed

[rfc2822]

Nice that it works with the CaptureExceptionsRule :)

[rfc2822]

:)