Bug #493

- Not a fix for the bug, but improved task consistency checking.  The
  real fix can only be made when the Task::waiting status is abolished.

src/Task.cpp

@@ -634,21 +634,21 @@ void Task::validate () const
   if (!has ("uuid")  ||
       !has ("entry") ||
       !has ("description"))
-    throw std::string ("A task must have a description in order to be valid."); // TODO i18n
+    throw std::string ("A task must have a description in order to be valid.");
 
   if (get ("description") == "") // No i18n
-    throw std::string ("Cannot add a task that is blank, or contains <CR> or <LF> characters."); // TODO i18n
+    throw std::string ("Cannot add a task that is blank, or contains <CR> or <LF> characters.");
 
   if (has ("due"))
   {
     Date due (::atoi (get ("due").c_str ()));
 
-    // Verify until > due
+    // Verify wait < due
-    if (has ("until"))
+    if (has ("wait"))
     {
-      Date until (::atoi (get ("until").c_str ()));
+      Date wait (::atoi (get ("wait").c_str ()));
-      if (due > until)
+      if (wait > due)
-        throw std::string ("An 'until' date must be after a 'due' date."); // TODO i18n
+        throw std::string ("A 'wait' date must be after a 'due' date.");
     }
 
     Date entry (::atoi (get ("entry").c_str ()));
@@ -657,24 +657,36 @@ void Task::validate () const
     {
       Date start (::atoi (get ("start").c_str ()));
       if (entry > start)
-        throw std::string ("A 'start' date must be after an 'entry' date."); // TODO i18n
+        throw std::string ("A 'start' date must be after an 'entry' date.");
     }
 
     if (has ("end"))
     {
       Date end (::atoi (get ("end").c_str ()));
       if (entry > end)
-        throw std::string ("An 'end' date must be after an 'entry' date."); // TODO i18n
+        throw std::string ("An 'end' date must be after an 'entry' date.");
     }
   }
+  else
+  {
+    if (has ("recur"))
+      throw std::string ("You cannot specify a recurring task without a due date.");
+  }
+
+  if (has ("until") && !has ("recur"))
+    throw std::string ("You cannot specify an until date for a non-recurring task.");
 
   // Recur durations must be valid.
   if (has ("recur"))
   {
     Duration d;
     if (! d.valid (get ("recur")))
-      throw std::string ("A recurrence value must be valid."); // TODO i18n
+      throw std::string ("A recurrence value must be valid.");
   }
+
+  if (has ("wait") &&
+      getStatus () == Task::recurring)
+    throw std::string ("You cannot create a task that is both waiting and recurring.");
 }
 
 ////////////////////////////////////////////////////////////////////////////////

src/command.cpp

@@ -95,20 +95,6 @@ int handleAdd (std::string &outs)
     foreach (tag, context.tagAdditions)
       context.task.addTag (*tag);
 
-    // Perform some logical consistency checks.
-    if (context.task.has ("wait") &&
-        context.task.has ("due") &&
-        Date (context.task.get ("due")) < Date (context.task.get ("wait")))
-      context.footnote ("Warning: the wait date falls after the due date.");
-
-    if (context.task.has ("recur") &&
-        !context.task.has ("due"))
-      throw std::string ("You cannot specify a recurring task without a due date.");
-
-    if (context.task.has ("until")  &&
-        !context.task.has ("recur"))
-      throw std::string ("You cannot specify an until date for a non-recurring task.");
-
     // Must load pending to resolve dependencies, and to provide a new ID.
     context.tdb.lock (context.config.getBoolean ("locking"));
 
@@ -608,7 +594,7 @@ void handleMerge (std::string& outs)
     {
       // replace argument with uri from config
       std::string tmp = context.config.get ("merge." + file + ".uri");
-      
+
       if (tmp != "")
         file = tmp;
     }
@@ -648,6 +634,7 @@ void handleMerge (std::string& outs)
 
         std::string autopush = context.config.get ("merge.autopush");
 
+        // TODO Task accepts many different values for "yes".  Try Config::getBoolean.
         if ( ((autopush == "ask") && (confirm ("Do you want to push the changes to the database you merged from?")) )
            || (autopush == "yes") )
         {
@@ -668,7 +655,7 @@ void handlePush (std::string& outs)
   if (context.hooks.trigger ("pre-push-command"))
   {
     std::string file = trim (context.task.get ("description"));
-		
+
 		if (file.length () == 0)
 		{
 			// get default target from config
@@ -678,7 +665,7 @@ void handlePush (std::string& outs)
 		{
 			// replace argument with uri from config
 			std::string tmp = context.config.get ("push." + file + ".uri");
-			
+
 			if (tmp != "")
 				file = tmp;
 		}
@@ -711,7 +698,7 @@ void handlePull (std::string& outs)
   if (context.hooks.trigger ("pre-pull-command"))
   {
     std::string file = trim (context.task.get ("description"));
-		
+
 		if (file.length () == 0)
 		{
 			// get default target from config
@@ -721,7 +708,7 @@ void handlePull (std::string& outs)
 		{
 			// replace argument with uri from config
 			std::string tmp = context.config.get ("pull." + file + ".uri");
-			
+
 			if (tmp != "")
 				file = tmp;
 		}
@@ -729,7 +716,7 @@ void handlePull (std::string& outs)
     if (file.length () > 0)
     {
 			Directory location (context.config.get ("data.location"));
-			
+
 			// add *.data to path if necessary
 			if (file.find ("*.data") == std::string::npos)
 			{
@@ -893,7 +880,7 @@ int handleShow (std::string &outs)
       "complete.all.projects complete.all.tags search.case.sensitive hooks "
       "active.indicator tag.indicator recurrence.indicator recurrence.limit "
       "list.all.projects list.all.tags undo.style verbose rule.precedence.color "
-      "merge.autopush "
+      "merge.autopush merge.default.uri pull.default.uri "
 #ifdef FEATURE_SHELL
       "shell.prompt "
 #endif
@@ -1542,6 +1529,9 @@ int handleDone (std::string &outs)
         // Change status.
         task->setStatus (Task::completed);
 
+        // Only allow valid tasks.
+        task->validate ();
+
         if (taskDiff (before, *task))
         {
           if (context.hooks.trigger ("pre-completed", *task))
@@ -1696,6 +1686,9 @@ int handleModify (std::string &outs)
 
         if (taskDiff (before, *other))
         {
+          // Only allow valid tasks.
+          other->validate ();
+
           if (changes && permission.confirmed (before, taskDifferences (before, *other) + "Proceed with change?"))
           {
             // TODO Are dependencies being explicitly removed?
@@ -1773,6 +1766,9 @@ int handleAppend (std::string &outs)
 
           if (taskDiff (before, *other))
           {
+            // Only allow valid tasks.
+            other->validate ();
+
             if (changes && permission.confirmed (before, taskDifferences (before, *other) + "Proceed with change?"))
             {
               context.tdb.update (*other);
@@ -1852,6 +1848,9 @@ int handlePrepend (std::string &outs)
 
           if (taskDiff (before, *other))
           {
+            // Only allow valid tasks.
+            other->validate ();
+
             if (changes && permission.confirmed (before, taskDifferences (before, *other) + "Are you sure?"))
             {
               context.tdb.update (*other);
@@ -1937,6 +1936,9 @@ int handleDuplicate (std::string &outs)
       sprintf (entryTime, "%u", (unsigned int) time (NULL));
       dup.set ("entry", entryTime);
 
+      // Only allow valid tasks.
+      dup.validate ();
+
       context.tdb.add (dup);
 
       if (context.config.getBoolean ("echo.command"))
@@ -2298,6 +2300,9 @@ int handleAnnotate (std::string &outs)
 
       if (taskDiff (before, *task))
       {
+        // Only allow valid tasks.
+        task->validate ();
+
         if (permission.confirmed (before, taskDifferences (before, *task) + "Proceed with change?"))
         {
           context.tdb.update (*task);
@@ -2393,6 +2398,9 @@ int handleDenotate (std::string &outs)
 
       if (taskDiff (before, *task))
       {
+        // Only allow valid tasks.
+        task->validate ();
+
         if (permission.confirmed (before, taskDifferences (before, *task) + "Proceed with change?"))
         {
           context.tdb.update (*task);

src/tests/wait.t

@@ -76,7 +76,7 @@ like ($output, qr/tomorrow/ms, 'waiting task visible when specifically queried')
 
 # Message is 'Warning: the wait date falls after the due date.'
 $output = qx{../task rc:wait.rc add Complain due:today wait:tomorrow};
-like ($output, qr/wait\sdate\sfalls/ms, 'warning on wait after due');
+like ($output, qr/A 'wait' date must be after a 'due' date\./, 'error on wait after due');
 
 # Cleanup.
 unlink 'pending.data';