Don't set created and last modified fields when not present in ICS data.
Needs ReviewPublic

Authored by dcaliste on Jan 14 2020, 1:02 PM.

Details

Reviewers
vkrause
dvratil
winterz
Group Reviewers
KDE PIM
Summary

CREATED and LAST-MODIFIED fields are not mandatory in RFC5545.
Unfortunately, Incidence::recreate() is unconditionally
setting a created and a last modified time stamps. This causes
all read events or todos from ICS data to always have a
created date and a last modified one.

Test Plan

Added a test in testicalformat to check that unspecified created
or last modified dates results in invalid created and lastModified
properties.

Diff Detail

Repository
R172 KCalendar Core
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21115
Build 21133: arc lint + arc unit
dcaliste created this revision.Jan 14 2020, 1:02 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptJan 14 2020, 1:02 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
dcaliste requested review of this revision.Jan 14 2020, 1:02 PM

The patch is not necessary the ideal one, but I would like to start a discussion on this matter. Currently, when calling ICalFormat::fromString(), all created incidences feature a created() and a lastModified() properties. They either come from the values read from the ICS data string, or they are set to the current date time by Incidence::recreate() from Incidence constructor.

RFC 5545 is specifying that CREATED and LAST-MODIFIED fields are not mandatory. With the incidence creator setting these two unconditionally, we cannot distinguish between ICS data with or without created and last modified fields set.

I wonder if we should on the contrary to the current patch, change Incidence::Incidence() itself, not to call Incidence::recreate() but just set the revision to 0 and set the scheduling id, but not set created and last modified. This would be more matching the RFC, but it is a huge change that could have unexpected consequences, since code may rely on the fact that the constructor of Incidence do set these two fields.

Finally, the patch itself may indeed suffer from the same unexpected consequences.

What do you think ? Should we ignore this, try to get incidence closer to the ICS data, or even don't set created and last modified field by default ?

dcaliste updated this revision to Diff 73521.Jan 14 2020, 1:20 PM

Add in the test another ICS data to ensure that existing CREATED and LAST-MODIFIED flags are kept.

Ping ! Do you have any remarks on this matter of created date set ?