Better API to reflect optional yet related fields in an iCal item

src/ical/builder.rs

@@ -7,6 +7,7 @@ use ics::properties::{Completed, Created, LastModified, PercentComplete, Status,
 use ics::{ICalendar, ToDo};
 
 use crate::item::Item;
+use crate::task::CompletionStatus;
 use crate::settings::{ORG_NAME, PRODUCT_NAME};
 
 fn ical_product_id() -> String {
@@ -30,14 +31,17 @@ pub fn build_from(item: &Item) -> Result<String, Box<dyn Error>> {
 
     match item {
         Item::Task(t) => {
-            if t.completed() {
+            match t.completion_status() {
+                CompletionStatus::Uncompleted => {
+                    todo.push(Status::needs_action());
+                },
+                CompletionStatus::Completed(completion_date) => {
                     todo.push(PercentComplete::new("100"));
-                t.completion_date().map(|dt| todo.push(
+                    completion_date.as_ref().map(|dt| todo.push(
                         Completed::new(format_date_time(dt))
                     ));
                     todo.push(Status::completed());
-            } else {
-                todo.push(Status::needs_action());
+                }
             }
         },
         _ => {
@@ -108,12 +112,9 @@ mod tests {
         let now = Utc::now();
         let s_now = format_date_time(&now);
 
-        let mut task = Item::Task(Task::new(
+        let task = Item::Task(Task::new(
             String::from("This is a task with ÜTF-8 characters"), completed, &cal_id
         ));
-        if completed {
-            task.unwrap_task_mut().set_completed_on(Some(now));
-        }
 
         let ical = build_from(&task).unwrap();
         (s_now, task.uid().to_string(), ical)

src/ical/parser.rs

@@ -9,6 +9,7 @@ use crate::Item;
 use crate::item::SyncStatus;
 use crate::item::ItemId;
 use crate::Task;
+use crate::task::CompletionStatus;
 use crate::Event;
 
 
@@ -53,13 +54,15 @@ pub fn parse(content: &str, item_id: ItemId, sync_status: SyncStatus) -> Result<
                     uid = prop.value.clone();
                 }
                 if prop.name == "DTSTAMP" {
-                    // this property specifies the date and time that the information associated with
-                    // the calendar component was last revised in the calendar store.
+                    // The property can be specified once, but is not mandatory
+                    // "This property specifies the date and time that the information associated with
+                    //  the calendar component was last revised in the calendar store."
                     last_modified = parse_date_time_from_property(&prop.value)
                 }
                 if prop.name == "COMPLETED" {
-                    // this property specifies the date and time that the information associated with
-                    // the calendar component was last revised in the calendar store.
+                    // The property can be specified once, but is not mandatory
+                    // "This property defines the date and time that a to-do was
+                    //  actually completed."
                     completion_date = parse_date_time_from_property(&prop.value)
                 }
                 if prop.name == "CREATED" {
@@ -79,13 +82,17 @@ pub fn parse(content: &str, item_id: ItemId, sync_status: SyncStatus) -> Result<
                 Some(dt) => dt,
                 None => return Err(format!("Missing DTSTAMP for item {}, but this is required by RFC5545", item_id).into()),
             };
-            if completion_date.is_none() && completed ||
-               completion_date.is_some() && !completed
-            {
-                log::warn!("Inconsistant iCal data: completion date is {:?} but completion status is {:?}", completion_date, completed);
+            let completion_status = match completed {
+                false => {
+                    if completion_date.is_some() {
+                        log::warn!("Task {:?} has an inconsistent content: its STATUS is not completed, yet it has a COMPLETED timestamp at {:?}", uid, completion_date);
                     }
+                    CompletionStatus::Uncompleted
+                },
+                true => CompletionStatus::Completed(completion_date),
+            };
 
-            Item::Task(Task::new_with_parameters(name, uid, item_id, sync_status, creation_date, last_modified, completion_date))
+            Item::Task(Task::new_with_parameters(name, uid, item_id, completion_status, sync_status, creation_date, last_modified))
         },
     };
 
@@ -174,6 +181,20 @@ COMPLETED:20210402T081557
 STATUS:COMPLETED
 END:VTODO
 END:VCALENDAR
+"#;
+
+const EXAMPLE_ICAL_COMPLETED_WITHOUT_A_COMPLETION_DATE: &str = r#"BEGIN:VCALENDAR
+VERSION:2.0
+PRODID:-//Nextcloud Tasks v0.13.6
+BEGIN:VTODO
+UID:19960401T080045Z-4000F192713-0052@example.com
+CREATED:20210321T001600
+LAST-MODIFIED:20210402T081557
+DTSTAMP:20210402T081557
+SUMMARY:Clean up your room or Mom will be angry
+STATUS:COMPLETED
+END:VTODO
+END:VCALENDAR
 "#;
 
     const EXAMPLE_MULTIPLE_ICAL: &str = r#"BEGIN:VCALENDAR
@@ -214,12 +235,13 @@ END:VCALENDAR
         assert_eq!(task.id(), &item_id);
         assert_eq!(task.uid(), "0633de27-8c32-42be-bcb8-63bc879c6185@some-domain.com");
         assert_eq!(task.completed(), false);
+        assert_eq!(task.completion_status(), &CompletionStatus::Uncompleted);
         assert_eq!(task.sync_status(), &sync_status);
         assert_eq!(task.last_modified(), &Utc.ymd(2021, 03, 21).and_hms(0, 16, 0));
     }
 
     #[test]
-    fn test_ical_completion_parsing() {
+    fn test_completed_ical_parsing() {
         let version_tag = VersionTag::from(String::from("test-tag"));
         let sync_status = SyncStatus::Synced(version_tag);
         let item_id: ItemId = "http://some.id/for/testing".parse().unwrap();
@@ -228,6 +250,20 @@ END:VCALENDAR
         let task = item.unwrap_task();
 
         assert_eq!(task.completed(), true);
+        assert_eq!(task.completion_status(), &CompletionStatus::Completed(Some(Utc.ymd(2021, 04, 02).and_hms(8, 15, 57))));
+    }
+
+    #[test]
+    fn test_completed_without_date_ical_parsing() {
+        let version_tag = VersionTag::from(String::from("test-tag"));
+        let sync_status = SyncStatus::Synced(version_tag);
+        let item_id: ItemId = "http://some.id/for/testing".parse().unwrap();
+
+        let item = parse(EXAMPLE_ICAL_COMPLETED_WITHOUT_A_COMPLETION_DATE, item_id.clone(), sync_status.clone()).unwrap();
+        let task = item.unwrap_task();
+
+        assert_eq!(task.completed(), true);
+        assert_eq!(task.completion_status(), &CompletionStatus::Completed(None));
     }
 
     #[test]

src/lib.rs

@@ -20,7 +20,7 @@ pub use item::Item;
 pub use item::ItemId;
 pub use item::VersionTag;
 pub use item::SyncStatus;
-mod task;
+pub mod task;
 pub use task::Task;
 mod event;
 pub use event::Event;

src/task.rs

@@ -6,6 +6,26 @@ use crate::item::ItemId;
 use crate::item::SyncStatus;
 use crate::calendar::CalendarId;
 
+/// RFC5545 defines the completion as several optional fields, yet some combinations make no sense.
+/// This enum provides an API that forbids such impossible combinations.
+///
+/// * `COMPLETED` is an optional timestamp that tells whether this task is completed
+/// * `STATUS` is an optional field, that can be set to `NEEDS-ACTION`, `COMPLETED`, or others.
+/// Even though having a `COMPLETED` date but a `STATUS:NEEDS-ACTION` is theorically possible, it obviously makes no sense. This API ensures this cannot happen
+#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
+pub enum CompletionStatus {
+    Completed(Option<DateTime<Utc>>),
+    Uncompleted,
+}
+impl CompletionStatus {
+    pub fn is_completed(&self) -> bool {
+        match self {
+            CompletionStatus::Completed(_) => true,
+            _ => false,
+        }
+    }
+}
+
 /// A to-do task
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct Task {
@@ -23,13 +43,15 @@ pub struct Task {
     creation_date: Option<DateTime<Utc>>,
     /// The last time this item was modified
     last_modified: DateTime<Utc>,
+    /// The completion status of this task
+    completion_status: CompletionStatus,
 
     /// The display name of the task
     name: String,
-    /// The time it was completed. Set to None if this task has not been completed
-    completion_date: Option<DateTime<Utc>>,
+
 }
 
+
 impl Task {
     /// Create a brand new Task that is not on a server yet.
     /// This will pick a new (random) task ID.
@@ -39,20 +61,23 @@ impl Task {
         let new_uid = Uuid::new_v4().to_hyphenated().to_string();
         let new_creation_date = Some(Utc::now());
         let new_last_modified = Utc::now();
-        let new_completion_date = if completed { Some(Utc::now()) } else { None };
-        Self::new_with_parameters(name, new_uid, new_item_id, new_sync_status, new_creation_date, new_last_modified, new_completion_date)
+        let new_completion_status = if completed {
+                CompletionStatus::Completed(Some(Utc::now()))
+            } else { CompletionStatus::Uncompleted };
+        Self::new_with_parameters(name, new_uid, new_item_id, new_completion_status, new_sync_status, new_creation_date, new_last_modified)
     }
 
     /// Create a new Task instance, that may be synced already
     pub fn new_with_parameters(name: String, uid: String, id: ItemId,
-                               sync_status: SyncStatus, creation_date: Option<DateTime<Utc>>, last_modified: DateTime<Utc>, completion_date: Option<DateTime<Utc>>) -> Self
+                               completion_status: CompletionStatus,
+                               sync_status: SyncStatus, creation_date: Option<DateTime<Utc>>, last_modified: DateTime<Utc>) -> Self
     {
         Self {
             id,
             uid,
             name,
+            completion_status,
             sync_status,
-            completion_date,
             creation_date,
             last_modified,
         }
@@ -61,16 +86,16 @@ impl Task {
     pub fn id(&self) -> &ItemId     { &self.id          }
     pub fn uid(&self) -> &str       { &self.uid         }
     pub fn name(&self) -> &str      { &self.name        }
-    pub fn completed(&self) -> bool { self.completion_date.is_some() }
+    pub fn completed(&self) -> bool { self.completion_status.is_completed() }
     pub fn sync_status(&self) -> &SyncStatus      { &self.sync_status  }
     pub fn last_modified(&self) -> &DateTime<Utc> { &self.last_modified }
     pub fn creation_date(&self) -> Option<&DateTime<Utc>>   { self.creation_date.as_ref() }
-    pub fn completion_date(&self) -> Option<&DateTime<Utc>> { self.completion_date.as_ref() }
+    pub fn completion_status(&self) -> &CompletionStatus    { &self.completion_status }
 
     pub fn has_same_observable_content_as(&self, other: &Task) -> bool {
            self.id == other.id
         && self.name == other.name
-        && self.completion_date == other.completion_date
+        && self.completion_status == other.completion_status
         && self.last_modified == other.last_modified
         // sync status must be the same variant, but we ignore its embedded version tag
         && std::mem::discriminant(&self.sync_status) == std::mem::discriminant(&other.sync_status)
@@ -113,16 +138,16 @@ impl Task {
         self.name = new_name;
     }
 
-    /// Set the completion date (or pass None to un-complete the task)
-    pub fn set_completed_on(&mut self, new_completion_date: Option<DateTime<Utc>>) {
+    /// Set the completion status
+    pub fn set_completion_status(&mut self, new_completion_status: CompletionStatus) {
         self.update_sync_status();
         self.update_last_modified();
-        self.completion_date = new_completion_date;
+        self.completion_status = new_completion_status;
     }
     #[cfg(feature = "local_calendar_mocks_remote_calendars")]
     /// Set the completion status, but forces a "master" SyncStatus, just like CalDAV servers are always "masters"
-    pub fn mock_remote_calendar_set_completed_on(&mut self, nnew_completion_date: Option<DateTime<Utc>>) {
+    pub fn mock_remote_calendar_set_completion_status(&mut self, new_completion_status: CompletionStatus) {
         self.sync_status = SyncStatus::random_synced();
-        self.completion_date = new_completion_date;
+        self.completion_status = new_completion_status;
     }
 }

tests/scenarii.rs

@@ -12,6 +12,8 @@ use std::path::PathBuf;
 use std::sync::{Arc, Mutex};
 use std::error::Error;
 
+use chrono::Utc;
+
 use my_tasks::calendar::CalendarId;
 use my_tasks::calendar::SupportedComponents;
 use my_tasks::traits::CalDavSource;
@@ -23,6 +25,7 @@ use my_tasks::Item;
 use my_tasks::ItemId;
 use my_tasks::SyncStatus;
 use my_tasks::Task;
+use my_tasks::task::CompletionStatus;
 use my_tasks::calendar::cached_calendar::CachedCalendar;
 use my_tasks::Provider;
 use my_tasks::mock_behaviour::MockBehaviour;
@@ -373,7 +376,11 @@ pub fn scenarii_basic() -> Vec<ItemScenario> {
             initial_state: LocatedState::None,
             local_changes_to_apply: Vec::new(),
             remote_changes_to_apply: vec![ChangeToApply::Create(third_cal.clone(), Item::Task(
-                Task::new_with_parameters(String::from("Task Q, created on the server"), false, id_q, SyncStatus::random_synced() )
+                Task::new_with_parameters(
+                    String::from("Task Q, created on the server"),
+                    id_q.to_string(), id_q,
+                    CompletionStatus::Uncompleted,
+                    SyncStatus::random_synced(), Some(Utc::now()), Utc::now() )
             ))],
             after_sync: LocatedState::BothSynced( ItemState{
                 calendar: third_cal.clone(),
@@ -389,7 +396,11 @@ pub fn scenarii_basic() -> Vec<ItemScenario> {
             id: id_r.clone(),
             initial_state: LocatedState::None,
             local_changes_to_apply: vec![ChangeToApply::Create(third_cal.clone(), Item::Task(
-                Task::new_with_parameters(String::from("Task R, created locally"), false, id_r, SyncStatus::NotSynced )
+                Task::new_with_parameters(
+                    String::from("Task R, created locally"),
+                    id_r.to_string(), id_r,
+                    CompletionStatus::Uncompleted,
+                    SyncStatus::NotSynced, Some(Utc::now()), Utc::now() )
             ))],
             remote_changes_to_apply: Vec::new(),
             after_sync: LocatedState::BothSynced( ItemState{
@@ -563,7 +574,11 @@ pub fn scenarii_transient_task() -> Vec<ItemScenario> {
             initial_state: LocatedState::None,
             local_changes_to_apply: vec![
                 ChangeToApply::Create(cal, Item::Task(
-                    Task::new_with_parameters(String::from("A transient task that will be deleted before the sync"), false, id_transient, SyncStatus::NotSynced )
+                    Task::new_with_parameters(
+                        String::from("A transient task that will be deleted before the sync"),
+                        id_transient.to_string(), id_transient,
+                        CompletionStatus::Uncompleted,
+                        SyncStatus::NotSynced, Some(Utc::now()), Utc::now() )
                 )),
 
                 ChangeToApply::Rename(String::from("A new name")),
@@ -612,12 +627,21 @@ async fn populate_test_provider(scenarii: &[ItemScenario], mock_behaviour: Arc<M
             LocatedState::BothSynced(s) => (s, SyncStatus::random_synced()),
         };
 
+        let now = Utc::now();
+        let completion_status = match state.completed {
+            false => CompletionStatus::Uncompleted,
+            true => CompletionStatus::Completed(Some(now)),
+        };
+
         let new_item = Item::Task(
             Task::new_with_parameters(
                 state.name.clone(),
-                state.completed,
+                item.id.to_string(),
                 item.id.clone(),
+                completion_status,
                 sync_status,
+                Some(now),
+                now,
             ));
 
         match required_state {
@@ -713,10 +737,14 @@ where
             }
         },
         ChangeToApply::SetCompletion(new_status) => {
+            let completion_status = match new_status {
+                false => CompletionStatus::Uncompleted,
+                true => CompletionStatus::Completed(Some(Utc::now())),
+            };
             if is_remote {
-                task.mock_remote_calendar_set_completed(new_status.clone());
+                task.mock_remote_calendar_set_completion_status(completion_status);
             } else {
-                task.set_completed(new_status.clone());
+                task.set_completion_status(completion_status);
             }
         },
         ChangeToApply::Remove => {