fix(time): actually test if begin time lies in the future, throwing a…

…n error that begin time cannot be after end time

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

crates/core/src/commands/adjust.rs

@@ -7,8 +7,9 @@ use getset::Getters;
 use typed_builder::TypedBuilder;
 
 use crate::{
-    error::ActivityLogErrorKind, get_storage_from_config, ActivityQuerying, ActivityStore,
-    ActivityWriteOps, PaceConfig, PaceDateTime, PaceResult, SyncStorage, UpdateOptions,
+    error::{ActivityLogErrorKind, PaceTimeErrorKind},
+    get_storage_from_config, ActivityQuerying, ActivityStore, ActivityWriteOps, PaceConfig,
+    PaceDateTime, PaceResult, SyncStorage, UpdateOptions,
 };
 
 /// `adjust` subcommand options
@@ -88,8 +89,15 @@ impl AdjustCommandOptions {
         }
 
         if let Some(start) = &self.start {
+            // Test if PaceDateTime actually lies in the future
             let start_time =
                 PaceDateTime::new(NaiveDateTime::new(*activity.begin().date(), *start));
+
+            // Test if PaceDateTime actually lies in the future
+            if start_time > PaceDateTime::now() {
+                return Err(PaceTimeErrorKind::StartTimeInFuture(start_time).into());
+            };
+
             _ = activity.set_begin(start_time);
         }
 

crates/core/src/commands/begin.rs

@@ -4,8 +4,8 @@ use std::collections::HashSet;
 use clap::Parser;
 
 use crate::{
-    extract_time_or_now, get_storage_from_config, Activity, ActivityKind, ActivityStateManagement,
-    ActivityStore, PaceConfig, PaceResult, SyncStorage,
+    error::PaceTimeErrorKind, extract_time_or_now, get_storage_from_config, Activity, ActivityKind,
+    ActivityStateManagement, ActivityStore, PaceConfig, PaceDateTime, PaceResult, SyncStorage,
 };
 
 /// `begin` subcommand options
@@ -46,7 +46,7 @@ impl BeginCommandOptions {
     pub fn handle_begin(&self, config: &PaceConfig) -> PaceResult<()> {
         let Self {
             category,
-            start: time,
+            start,
             description,
             tags,
             .. // TODO: exclude projects for now
@@ -58,7 +58,12 @@ impl BeginCommandOptions {
             .map(|tags| tags.iter().cloned().collect::<HashSet<String>>());
 
         // parse time from string or get now
-        let date_time = extract_time_or_now(time)?;
+        let date_time = extract_time_or_now(start)?;
+
+        // Test if PaceDateTime actually lies in the future
+        if date_time > PaceDateTime::now() {
+            return Err(PaceTimeErrorKind::StartTimeInFuture(date_time).into());
+        }
 
         // TODO: Parse categories and subcategories from string
         // let (category, subcategory) = if let Some(ref category) = category {

crates/core/src/domain/time.rs

@@ -6,7 +6,7 @@ use std::{
 
 use crate::{
     commands::review::{DateFlags, TimeFlags},
-    error::{ActivityLogErrorKind, PaceErrorKind, PaceOptResult, PaceResult},
+    error::{PaceOptResult, PaceResult, PaceTimeErrorKind},
 };
 use chrono::{
     DateTime, Local, LocalResult, NaiveDate, NaiveDateTime, NaiveTime, SubsecRound, TimeZone,
@@ -105,15 +105,15 @@ pub fn duration_to_str(initial_time: DateTime<Local>) -> String {
 /// # Returns
 ///
 /// A tuple containing the time and date
-pub fn extract_time_or_now(time: &Option<String>) -> PaceResult<NaiveDateTime> {
+pub fn extract_time_or_now(time: &Option<String>) -> PaceResult<PaceDateTime> {
     Ok(if let Some(ref time) = time {
-        NaiveDateTime::new(
+        PaceDateTime::new(NaiveDateTime::new(
             Local::now().date_naive(),
             NaiveTime::parse_from_str(time, "%H:%M")?,
-        )
+        ))
     } else {
         // if no time is given, use the current time
-        Local::now().naive_local().round_subsecs(0)
+        PaceDateTime::now()
     })
 }
 
@@ -134,7 +134,7 @@ pub fn parse_time_from_user_input(time: &Option<String>) -> PaceOptResult<NaiveD
     time.as_ref()
         .map(|time| -> PaceResult<NaiveDateTime> {
             let Ok(time) = NaiveTime::parse_from_str(time, "%H:%M") else {
-                return Err(PaceErrorKind::ParsingTimeFromUserInputFailed(time.clone()).into());
+                return Err(PaceTimeErrorKind::ParsingTimeFromUserInputFailed(time.clone()).into());
             };
 
             Ok(NaiveDateTime::new(Local::now().date_naive(), time))
@@ -155,11 +155,11 @@ pub enum PaceDurationRange {
 pub struct PaceDuration(u64);
 
 impl FromStr for PaceDuration {
-    type Err = ActivityLogErrorKind;
+    type Err = PaceTimeErrorKind;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         s.parse::<u64>().map_or_else(
-            |_| Err(ActivityLogErrorKind::ParsingDurationFailed(s.to_string())),
+            |_| Err(PaceTimeErrorKind::ParsingDurationFailed(s.to_string())),
             |duration| Ok(Self(duration)),
         )
     }
@@ -231,7 +231,7 @@ impl std::ops::Deref for PaceTime {
 }
 
 /// Wrapper for the start and end time of an activity to implement default
-#[derive(Debug, Serialize, Deserialize, Hash, Clone, Copy, Eq, PartialEq)]
+#[derive(Debug, Serialize, Deserialize, Hash, Clone, Copy, Eq, PartialEq, PartialOrd, Ord)]
 pub struct PaceDateTime(NaiveDateTime);
 
 impl PaceDateTime {
@@ -379,10 +379,10 @@ mod tests {
         let result = extract_time_or_now(&time).expect("Time extraction failed");
         assert_eq!(
             result,
-            NaiveDateTime::new(
+            PaceDateTime(NaiveDateTime::new(
                 Local::now().date_naive(),
                 NaiveTime::from_hms_opt(12, 0, 0).expect("Invalid date"),
-            )
+            ))
         );
     }
 

crates/core/src/error.rs

@@ -5,7 +5,7 @@ use miette::Diagnostic;
 use std::{error::Error, path::PathBuf};
 use thiserror::Error;
 
-use crate::{domain::activity::ActivityGuid, Activity};
+use crate::{domain::activity::ActivityGuid, Activity, PaceDateTime};
 
 /// Result type that is being returned from test functions and methods that can fail and thus have errors.
 pub type TestResult<T> = Result<T, Box<dyn Error + 'static>>;
@@ -70,25 +70,36 @@ pub enum PaceErrorKind {
     /// [`std::io::Error`]
     #[error(transparent)]
     StdIo(#[from] std::io::Error),
+
     /// Serialization to TOML failed: {0}
     #[error(transparent)]
     SerializationToTomlFailed(#[from] toml::ser::Error),
+
     /// Deserialization from TOML failed: {0}
     #[error(transparent)]
     DeserializationFromTomlFailed(#[from] toml::de::Error),
+
     /// Activity log error: {0}
     #[error(transparent)]
     ActivityLog(#[from] ActivityLogErrorKind),
+
+    /// Time related error: {0}
+    #[error(transparent)]
+    PaceTime(#[from] PaceTimeErrorKind),
     #[cfg(feature = "sqlite")]
+
     /// SQLite error: {0}
     #[error(transparent)]
     SQLite(#[from] rusqlite::Error),
+
     /// Chrono parse error: {0}
     #[error(transparent)]
     ChronoParse(#[from] chrono::ParseError),
+
     /// Chrono duration is negative: {0}
     #[error(transparent)]
     ChronoDurationIsNegative(#[from] chrono::OutOfRangeError),
+
     /// Config file {file_name} not found in directory hierarchy starting from {current_dir}
     ConfigFileNotFound {
         /// The current directory
@@ -97,12 +108,13 @@ pub enum PaceErrorKind {
         /// The file name
         file_name: String,
     },
+
     /// Configuration file not found, please run `pace setup config` to initialize `pace`
     ParentDirNotFound(PathBuf),
+
     /// Database storage not implemented, yet!
     DatabaseStorageNotImplemented,
-    /// Failed to parse time '{0}' from user input, please use the format HH:MM
-    ParsingTimeFromUserInputFailed(String),
+
     /// There is no path available to store the activity log
     NoPathAvailable,
 }
@@ -113,56 +125,91 @@ pub enum PaceErrorKind {
 pub enum ActivityLogErrorKind {
     /// No activities found in the activity log
     NoActivitiesFound,
+
     /// Activity with ID {0} not found
     FailedToReadActivity(ActivityGuid),
+
     /// Negative duration for activity
     NegativeDuration,
+
     /// There are no activities to hold
     NoActivityToHold,
+
     /// Failed to unwrap Arc
     ArcUnwrapFailed,
+
     /// There are no unfinished activities to end
     NoUnfinishedActivities,
+
     /// There is no cache to sync
     NoCacheToSync,
+
     /// Cache not available
     CacheNotAvailable,
+
     /// Activity with id '{0}' not found
     ActivityNotFound(ActivityGuid),
+
     /// Activity with id '{0}' can't be removed from the activity log
     ActivityCantBeRemoved(usize),
+
     /// This activity has no id
     ActivityIdNotSet,
+
     /// Activity with id '{0}' already in use, can't create a new activity with the same id
     ActivityIdAlreadyInUse(ActivityGuid),
-    /// Failed to parse duration '{0}' from activity log, please use only numbers >= 0
-    ParsingDurationFailed(String),
+
     /// Activity in the ActivityLog has a different id than the one provided: {0} != {1}
     ActivityIdMismatch(ActivityGuid, ActivityGuid),
+
     /// Activity already has an intermission: {0}
     ActivityAlreadyHasIntermission(Box<Activity>),
+
     /// There have been some activities that have not been ended
     ActivityNotEnded,
+
     /// No active activity found with id '{0}'
     NoActiveActivityFound(ActivityGuid),
+
     /// Activity with id '{0}' already ended
     ActivityAlreadyEnded(ActivityGuid),
+
     /// Activity with id '{0}' already has been archived
     ActivityAlreadyArchived(ActivityGuid),
+
     /// Active activity with id '{0}' found, although we wanted a held activity
     ActiveActivityFound(ActivityGuid),
+
     /// Activity with id '{0}' is not held, but we wanted to resume it
     NoHeldActivityFound(ActivityGuid),
+
     /// No activity kind options found for activity with id '{0}'
     ActivityKindOptionsNotFound(ActivityGuid),
+
     /// ParentId not set for activity with id '{0}'
     ParentIdNotSet(ActivityGuid),
+
     /// Category not set for activity with id '{0}'
     CategoryNotSet(ActivityGuid),
+
     /// No active activity to adjust
     NoActiveActivityToAdjust,
 }
 
+/// [`PaceTimeErrorKind`] describes the errors that can happen while dealing with time.
+#[non_exhaustive]
+#[derive(Error, Debug, Display)]
+pub enum PaceTimeErrorKind {
+    /// Failed to parse time '{0}' from user input, please use the format HH:MM
+    ParsingTimeFromUserInputFailed(String),
+
+    /// The start time cannot be in the future: {0}
+    StartTimeInFuture(PaceDateTime),
+
+    /// Failed to parse duration '{0}' from activity log, please use only numbers >= 0
+    ParsingDurationFailed(String),
+}
+
 trait PaceErrorMarker: Error {}
 
 impl PaceErrorMarker for std::io::Error {}
@@ -173,6 +220,7 @@ impl PaceErrorMarker for rusqlite::Error {}
 impl PaceErrorMarker for chrono::ParseError {}
 impl PaceErrorMarker for chrono::OutOfRangeError {}
 impl PaceErrorMarker for ActivityLogErrorKind {}
+impl PaceErrorMarker for PaceTimeErrorKind {}
 
 impl<E> From<E> for PaceError
 where