fix(time-tracking): reject inverted time-entry intervals

pkg/models/error.go

@@ -2568,3 +2568,31 @@ func (err ErrTimeEntryAlreadyEnded) HTTPError() web.HTTPError {
 		Message:  "A time entry that has already ended cannot be reopened into a running timer. Start a new timer instead.",
 	}
 }
+
+// ErrTimeEntryEndBeforeStart represents an error where a time entry's end time
+// precedes its start time, which would persist a negative interval.
+type ErrTimeEntryEndBeforeStart struct {
+	TimeEntryID int64
+}
+
+// IsErrTimeEntryEndBeforeStart checks if an error is ErrTimeEntryEndBeforeStart.
+func IsErrTimeEntryEndBeforeStart(err error) bool {
+	_, ok := err.(ErrTimeEntryEndBeforeStart)
+	return ok
+}
+
+func (err ErrTimeEntryEndBeforeStart) Error() string {
+	return fmt.Sprintf("Time entry end time is before its start time [TimeEntryID: %v]", err.TimeEntryID)
+}
+
+// ErrCodeTimeEntryEndBeforeStart holds the unique world-error code of this error
+const ErrCodeTimeEntryEndBeforeStart = 18007
+
+// HTTPError holds the http error description
+func (err ErrTimeEntryEndBeforeStart) HTTPError() web.HTTPError {
+	return web.HTTPError{
+		HTTPCode: http.StatusBadRequest,
+		Code:     ErrCodeTimeEntryEndBeforeStart,
+		Message:  "A time entry's end time cannot be before its start time.",
+	}
+}

pkg/models/time_tracking.go

@@ -80,6 +80,10 @@ func (te *TimeEntry) Create(s *xorm.Session, a web.Auth) (err error) {
 		te.StartTime = time.Now()
 	}
 
+	if err = te.validateTimes(); err != nil {
+		return err
+	}
+
 	if _, err = s.Insert(te); err != nil {
 		return err
 	}
@@ -198,6 +202,10 @@ func (te *TimeEntry) Update(s *xorm.Session, a web.Auth) (err error) {
 		return ErrTimeEntryAlreadyEnded{TimeEntryID: te.ID}
 	}
 
+	if err = te.validateTimes(); err != nil {
+		return err
+	}
+
 	// task_id / project_id are listed so a reassignment (and the zero value of
 	// the side being cleared) is written; the XOR was validated in CanUpdate.
 	_, err = s.
@@ -313,6 +321,16 @@ func (te *TimeEntry) validateContainer() error {
 	return nil
 }
 
+// validateTimes rejects a completed entry whose end precedes its start (a
+// negative interval). A null end is a running timer and is always valid; an end
+// equal to the start is allowed (a zero-length entry).
+func (te *TimeEntry) validateTimes() error {
+	if te.EndTime != nil && te.EndTime.Before(te.StartTime) {
+		return ErrTimeEntryEndBeforeStart{TimeEntryID: te.ID}
+	}
+	return nil
+}
+
 func (te *TimeEntry) CanCreate(s *xorm.Session, a web.Auth) (bool, error) {
 	if _, isShareAuth := a.(*LinkSharing); isShareAuth {
 		return false, nil

pkg/models/time_tracking_test.go

@@ -484,6 +484,57 @@ func TestTimeEntry_UpdateReopenGuard(t *testing.T) {
 	})
 }
 
+func TestTimeEntry_RejectsInvertedInterval(t *testing.T) {
+	a := &user.User{ID: 1}
+	start := time.Date(2020, 1, 1, 10, 0, 0, 0, time.UTC)
+	before := time.Date(2020, 1, 1, 9, 0, 0, 0, time.UTC)
+
+	t.Run("create rejects an end before the start", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		te := &TimeEntry{TaskID: 1, StartTime: start, EndTime: timePtr(before)}
+		err := te.Create(s, a)
+		require.Error(t, err)
+		assert.True(t, IsErrTimeEntryEndBeforeStart(err), "unexpected error type: %v", err)
+	})
+
+	t.Run("create allows an end equal to the start", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		te := &TimeEntry{TaskID: 1, StartTime: start, EndTime: timePtr(start)}
+		require.NoError(t, te.Create(s, a))
+	})
+
+	t.Run("create allows a running timer with no end", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		te := &TimeEntry{TaskID: 1, StartTime: start} // EndTime nil
+		require.NoError(t, te.Create(s, a))
+	})
+
+	t.Run("update rejects an end before the start", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		// Entry 1 is user1's completed entry.
+		te := &TimeEntry{ID: 1, TaskID: 1, StartTime: start, EndTime: timePtr(before)}
+		can, err := te.CanUpdate(s, a)
+		require.NoError(t, err)
+		require.True(t, can)
+
+		err = te.Update(s, a)
+		require.Error(t, err)
+		assert.True(t, IsErrTimeEntryEndBeforeStart(err), "unexpected error type: %v", err)
+	})
+}
+
 func TestTimeEntry_StopRunningTimer(t *testing.T) {
 	t.Run("stops the caller's running timer and returns it", func(t *testing.T) {
 		db.LoadAndAssertFixtures(t)