Do not allow adding open intervals with a future time

[sruffell]

If an interval is started at a future time, as was possible with the
continue command when given a future time, subsequent commands could
fail if the end data was set to the current time.

Which would result in the following message:
Range.cpp:359: time_t Range::total() const: Assertion `is_open () || end >= start' failed.

This change makes it invalid to start a time in the future, however it
might still be possible to get in this case if the time on the system
jumps backward after starting an interval.

Related to #350
Closes #364

Signed-off-by: Shaun Ruffell sruffell@sruffell.net

[sruffell]

Sorry about the multiple pushes on this seemingly simple change. I originally had a different error message, but in the last commit I moved the check for a future interval in CmdStart down into the validate function as well, so all commands would see the same message "Time tracking cannot be set in the future"

[lauft]

The problem with #364 is in my opinion not that a future interval was entered, but some commands, e.g. CmdSummary, somehow do not expect future dates° and get thrown off course.

Of course, Timewarriors main focus is tracking of time spent and thus past intervals, but as pointed out in #350 (comment), there are situations where entering a future interval is reasonable. Therefore, Timewarrior should not encourage it* but also should not obstruct entering future intervals.

In the medium-/long-term Timewarrior should be enabled to deal with future intervals (other than throwing exceptions) such that general prohibition of future intervals can be dropped.

For now, we block entering of future intervals, but it should be done at the command level and not at the core for the reasons stated above.

---

° Obviously, open intervals with future time are the problematic ones. CmdTrack can enter (closed) future intervals and CmdSummary deals with them just fine...

* The "not encouraging" part would be that when the user has recorded a future interval, every simple start would abort because of interval overlap.

[lauft]

Instead of modifying CmdStart we should fix this by aligning CmdContinue and adding the same check for future times:

git diff src/commands/CmdContinue.cpp
diff --git a/src/commands/CmdContinue.cpp b/src/commands/CmdContinue.cpp
index 6ad679e2..62242c4d 100644
--- a/src/commands/CmdContinue.cpp
+++ b/src/commands/CmdContinue.cpp
@@ -38,10 +38,16 @@ int CmdContinue (
   Journal& journal)
 {
   const bool verbose = rules.getBoolean ("verbose");
+  const Datetime now {};
+
+  auto filter = cli.getFilter ({ now, 0 });
+
+  if (filter.start > now)
+  {
+    throw std::string ("Time tracking cannot be set in the future.");
+  }
 
-  // Gather IDs and TAGs.
   std::set <int> ids = cli.getIds ();
-  auto filter = cli.getFilter ();
 
   if (!ids.empty () && !filter.tags().empty ())
   {

[sruffell]

Of course, Timewarriors main focus is tracking of time spent and thus past intervals, but as pointed out in #350 (comment), there are situations where entering a future interval is reasonable. Therefore, Timewarrior should not encourage it* but also should not obstruct entering future intervals.

In the medium-/long-term Timewarrior should enabled to deal with future intervals (other than throwing exceptions) such that general prohibition of future intervals can be dropped.

For now, we block entering of future intervals, but it should be done at the command level and not at the core for the reasons stated above.

° Obviously, open intervals with future time are the problematic ones. CmdTrack can enter (closed) future intervals and CmdSummary deals with them just fine...

I guess I misunderstood the discussion on #350. My understanding was that, while future closed intervals were a valid use case (and currently possible) future open intervals were intended to be invalid.

If there aren't any valid cases for open intervals that start in the future, could you explain a bit more why making that validation step per-command is preferable to moving it to the validation function? It would seem to me that having a common function is safer in case a new command comes along and forgets to add any necessary checks (i.e. cmd import)

[lauft]

... My understanding was that, while future closed intervals were a valid use case (and currently possible) future open intervals were intended to be invalid. If there aren't any valid cases for open intervals that start in the future, ...

Use case for an open interval with future time:
I am preparing a meeting which starts in 15 minutes. While setting everything up I also start the time tracking for the meeting in advance, because I do not want to do this when everyone has gathered and I am most likely to forget it afterwards.

... could you explain a bit more why making that validation step per-command is preferable to moving it to the validation function?

Because it is a patch for something that should be dropped. And therefore I do not want it integrated into core functionality. 😉

To me it somehow boils down to:
What is the point of prohibiting future (starting) times in intervals? What is gained by this rule?

As seen, there are usecases where one does require them and "you could also do it this way!" is not a good argument against them. Timewarrior should rather enable workflow, not necessarily define it.

The gain of prohibiting future times in intervals seems little to me. Why is now more special than now±1? Why is now special at all? Yes, it is the point in time where you most likely issued the command, but you could insert any other value and it would divide your intervals into before and after all the same.

Timewarrior's commands (old and new) should be enabled to cope with all intervals, as long as they are ordered and do not overlap.

To rephrase what I said in #366 (review):

Timewarriors main focus is tracking of time spent and thus past intervals, but there are situations where entering a future interval is reasonable/desired/...
Therefore, Timewarrior should not not obstruct entering future intervals – but maybe also not encourage it.

[sl4mmy]

+1 to everything @lauft said.

I'm only about 80% successful remembering to start tracking time at the beginning of a meeting (even when I prep the command line invocation in advance so I just have to press enter 🤦 ). Being able to start an open interval in the future is something I would use every day.

[sruffell]

Thanks for the clarifications.

Given that we don't want to make a temporary change in the validate function, and that the move and modify functions also can potentially result in open intervals that are in the future (and not to mention a change of the system clock could result in future open intervals), how about we just make the changes to handle future times instead of carrying temporary changes?

I've opened #371 which allows timewarrior to handle open intervals in the future. @sl4mmy, care to try that out to see if it works for you and provide feedback? If there aren't any insurmountable issues there, we could close this PR and merge that one instead of temporarily changing 3 other commands in the code base.

[sl4mmy]

Oh, hey, just saw this comment after I commented on the other PR. Anyways, sure, I'll give that a try and report back my experience. Thanks!

[lauft]

I implemented the temporary fix for #364 and added @sruffell additional test for the continue command. Until #371 is settled we proceed defensively...