From d1e52c05d640899c9a15bacd8691fd17f4a61685 Mon Sep 17 00:00:00 2001
From: Paul Beckingham <paul@beckingham.net>
Date: Mon, 12 Sep 2011 22:57:32 -0400
Subject: [PATCH] Unit Tests

- While fixing bug.360.t, discovered a bigger problem, which is not yet
  fixed.  When a due date is removed from a recurring child task, the
  imask, parent and recur attributes should also be removed.  Similarly
  when a recur frequency is removed from a recurring child task, the
  imask and parent should also be removed.
- Net result: two new failing tests.
---
 src/A3.cpp                 |  6 -----
 src/commands/CmdModify.cpp | 47 +++++++++++++++++++-------------------
 test/bug.360.t             |  6 +++--
 3 files changed, 28 insertions(+), 31 deletions(-)

diff --git a/src/A3.cpp b/src/A3.cpp
index 94777f3e5..b0801479f 100644
--- a/src/A3.cpp
+++ b/src/A3.cpp
@@ -1795,7 +1795,6 @@ bool A3::extract_attr (
 {
   Nibbler n (input);
 
-  // Ensure a clean parse.
   name  = "";
   value = "";
 
@@ -1803,8 +1802,6 @@ bool A3::extract_attr (
   {
     if (n.skip (':'))
     {
-      // Both quoted and unquoted Att's are accepted.
-      // Consider removing this for a stricter parse.
       if (n.getQuoted   ('"', value)  ||
           n.getQuoted   ('\'', value) ||
           n.getUntilEOS (value))
@@ -1828,7 +1825,6 @@ bool A3::extract_attmod (
 {
   Nibbler n (input);
 
-  // Ensure a clean parse.
   name     = "";
   value    = "";
   modifier = "";
@@ -1847,8 +1843,6 @@ bool A3::extract_attmod (
     if (n.skip (':') ||
         n.skip ('='))
     {
-      // Both quoted and unquoted Att's are accepted.
-      // Consider removing this for a stricter parse.
       if (n.getQuoted   ('"', value)  ||
           n.getQuoted   ('\'', value) ||
           n.getUntilEOS (value))
diff --git a/src/commands/CmdModify.cpp b/src/commands/CmdModify.cpp
index b6d281337..206e97b29 100644
--- a/src/commands/CmdModify.cpp
+++ b/src/commands/CmdModify.cpp
@@ -78,37 +78,38 @@ int CmdModify::execute (std::string& output)
     Task before (*task);
     modify_task_description_replace (*task, modifications);
 
+    // Perform some logical consistency checks.
+    if (task->has ("recur")  &&
+        !task->has ("due")   &&
+        !before.has ("due"))
+      throw std::string ("You cannot specify a recurring task without a due date.");
+
+    if (task->has ("until")  &&
+        !task->has ("recur") &&
+        !before.has ("recur"))
+      throw std::string ("You cannot specify an until date for a non-recurring task.");
+
+    if (before.has ("recur") &&
+        before.has ("due")   &&
+        (!task->has ("due")  ||
+         task->get ("due") == ""))
+      throw std::string ("You cannot remove the due date from a recurring task.");
+
+    if (before.has ("recur")  &&
+        task->has ("recur")   &&
+        (!task->has ("recur") ||
+         task->get ("recur") == ""))
+      throw std::string ("You cannot remove the recurrence from a recurring task.");
+
     if (taskDiff (before, *task) &&
         permission.confirmed (*task, taskDifferences (before, *task) + "Proceed with change?"))
     {
+      // Checks passed, modify the task.
       ++count;
       context.tdb2.modify (*task);
       if (before.get ("project") != task->get ("project"))
         context.footnote (onProjectChange (before, *task));
 
-      // Perform some logical consistency checks.
-      // TODO Shouldn't these tests be in Task::validate?
-      if (task->has ("recur") &&
-          !task->has ("due")  &&
-          !before.has ("due"))
-        throw std::string ("You cannot specify a recurring task without a due date.");
-
-      if (task->has ("until")  &&
-          !task->has ("recur") &&
-          !before.has ("recur"))
-        throw std::string ("You cannot specify an until date for a non-recurring task.");
-
-      if (before.has ("recur")     &&
-          before.has ("due")        &&
-          task->has ("due") &&
-          task->get ("due") == "")
-        throw std::string ("You cannot remove the due date from a recurring task.");
-
-      if (before.has ("recur")        &&
-          task->has ("recur") &&
-          task->get ("recur") == "")
-        throw std::string ("You cannot remove the recurrence from a recurring task.");
-
       // Make all changes.
       bool warned = false;
       std::vector <Task> siblings = context.tdb2.siblings (*task);
diff --git a/test/bug.360.t b/test/bug.360.t
index 45a2b890b..5b73853ec 100755
--- a/test/bug.360.t
+++ b/test/bug.360.t
@@ -50,11 +50,13 @@ unlike ($output, qr/You cannot remove the recurrence from a recurring task./ms,
 
 # Now try to generate the error above via regular means - ie, is it actually
 # doing what it should?
-$output = qx{../src/task rc:bug.rc 1 modify recur:};
+# TODO Removing recur: from a recurring task should also remove imask and parent.
+$output = qx{../src/task rc:bug.rc 2 modify recur:};
 like ($output, qr/You cannot remove the recurrence from a recurring task./ms, 'Recurrence removal error');
 
 # Prevent removal of the due date from a recurring task.
-$output = qx{../src/task rc:bug.rc 1 modify due:};
+# TODO Removing due: from a recurring task should also remove recur, imask and parent
+$output = qx{../src/task rc:bug.rc 2 modify due:};
 like ($output, qr/You cannot remove the due date from a recurring task./ms, 'Cannot remove due date from a recurring task');
 
 # Allow removal of the due date from a non-recurring task.