Nag based on task state before modification

With this patch, taskwarrior uses the urgency of tasks before any
modifications are applied when deciding whether to show nag messages.

Previously, taskwarrior would apply modifications before deciding
whether to show nag messages, which could lead to spurious nag messages
when completing an active task.

src/commands/CmdDone.cpp

@@ -68,11 +68,11 @@ int CmdDone::execute (std::string&)
   // Accumulated project change notifications.
   std::map <std::string, std::string> projectChanges;
 
-  Task& lowest = filtered.front();
-
   if(filtered.size() > 1) {
     feedback_affected("This command will alter {1} tasks.", filtered.size());
   }
+
+  std::vector <Task> modified;
   for (auto& task : filtered)
   {
     Task before (task);
@@ -109,11 +109,12 @@ int CmdDone::execute (std::string&)
         ++count;
         feedback_affected ("Completed task {1} '{2}'.", task);
         feedback_unblocked (task);
-        if (task.urgency () < lowest.urgency ())
-          lowest = task;
         dependencyChainOnComplete (task);
         if (Context::getContext ().verbose ("project"))
           projectChanges[task.get ("project")] = onProjectChange (task);
+
+        // Save unmodified task for potential nagging later
+        modified.push_back(before);
       }
       else
       {
@@ -133,7 +134,7 @@ int CmdDone::execute (std::string&)
     }
   }
   
-  nag(lowest);
+  nag (modified);
   
   // Now list the project changes.
   for (const auto& change : projectChanges)

src/commands/CmdStart.cpp

@@ -68,10 +68,11 @@ int CmdStart::execute (std::string&)
   // Accumulated project change notifications.
   std::map <std::string, std::string> projectChanges;
 
-  bool nagged = false;
   if(filtered.size() > 1) {
     feedback_affected("This command will alter {1} tasks.", filtered.size());
   }
+
+  std::vector <Task> modified;
   for (auto& task : filtered)
   {
     if (! task.has ("start"))
@@ -101,11 +102,12 @@ int CmdStart::execute (std::string&)
         Context::getContext ().tdb2.modify (task);
         ++count;
         feedback_affected ("Starting task {1} '{2}'.", task);
-        if (!nagged)
-          nagged = nag (task);
         dependencyChainOnStart (task);
         if (Context::getContext ().verbose ("project"))
           projectChanges[task.get ("project")] = onProjectChange (task, false);
+
+        // Save unmodified task for potential nagging later
+        modified.push_back(before);
       }
       else
       {
@@ -125,6 +127,8 @@ int CmdStart::execute (std::string&)
     }
   }
 
+  nag (modified);
+
   // Now list the project changes.
   for (auto& change : projectChanges)
     if (change.first != "")

src/main.h

@@ -48,7 +48,7 @@ void updateRecurrenceMask (Task&);
 void handleRecurrence2 ();
 
 // nag.cpp
-bool nag (Task&);
+void nag (std::vector <Task>&);
 
 // rules.cpp
 void initializeColorRules ();

src/nag.cpp

@@ -24,41 +24,37 @@
 //
 ////////////////////////////////////////////////////////////////////////////////
 
+#include <algorithm>
 #include <cmake.h>
 #include <Context.h>
+#include <iterator>
+#include <unordered_set>
+#include <utility>
+#include <vector>
 
 ////////////////////////////////////////////////////////////////////////////////
-// Returns a Boolean indicator as to whether a nag message was generated, so
-// that commands can control the number of nag messages displayed (ie one is
-// enough).
-//
-// Otherwise generates a nag message, if one is defined, if there are tasks of
-// higher urgency.
-bool nag (Task& task)
+// Generates a nag message when there are READY tasks of a higher urgency.
+void nag (std::vector <Task>& tasks)
 {
-  // Special tag overrides nagging.
-  if (task.hasTag ("nonag"))
-    return false;
-
   auto msg = Context::getContext ().config.get ("nag");
-  if (msg != "")
-  {
-    // Scan all pending, non-recurring tasks.
-    auto pending = Context::getContext ().tdb2.pending.get_tasks ();
-    for (auto& t : pending)
-    {
-      if ((t.getStatus () == Task::pending  ||
-           t.getStatus () == Task::waiting) &&
-          t.hasTag ("READY")                &&
-          t.urgency () > task.urgency ())
+  if (msg == "")
+      return;
+
+  auto pending = Context::getContext ().tdb2.pending.get_tasks ();
+  for (auto& t1 : tasks) {
+    if (t1.hasTag ("nonag"))
+      continue;
+
+    for (auto& t2 : pending) {
+      if (t1.get ("uuid") != t2.get ("uuid") &&
+        t2.hasTag ("READY")                  &&
+        t1.urgency () < t2.urgency ())
       {
         Context::getContext ().footnote (msg);
-        return true;
+        return;
       }
     }
   }
-
-  return false;
 }
 
 ////////////////////////////////////////////////////////////////////////////////

test/nag.t

@@ -91,6 +91,70 @@ class TestNagging(TestCase):
         code, out, err = self.t("1 done")
         self.assertNotIn("NAG", err)
 
+    def test_nagging_bulk(self):
+        """Verify that only one nag message occurs when completing multiple tasks"""
+        self.t("add one")
+        self.t.faketime("+1d")
+        self.t("add two")
+        self.t("add two")
+
+        code, out, err = self.t("2 done")
+
+        self.assertEqual(err.count("NAG"), 1)
+
+    def test_nagging_active(self):
+        """Bug 2163: Verify that nagging does not occur when completing the most urgent active task"""
+        self.t("add one")
+        self.t.faketime("+1d")
+        self.t("add two")
+        self.t("2 start")
+
+        code, out, err = self.t("2 done")
+
+        # Taskwarrior should not nag about more urgent tasks
+        self.assertNotIn("NAG", err)
+
+    def test_nagging_start_only_task(self):
+        """Verify that nagging does not occur when there are no other tasks while starting a task"""
+        self.t("add one")
+
+        code, out, err = self.t("1 start")
+
+        self.assertNotIn("NAG", err)
+
+    def test_nagging_start(self):
+        """Verify that nagging occurs when there are READY tasks of higher urgency while starting a task"""
+        self.t("add one")
+        self.t.faketime("+10d")
+        self.t("add two")
+
+        code, out, err = self.t("2 start")
+
+        self.assertIn("NAG", err)
+
+    def test_nagging_nonag(self):
+        """Verify that nagging does not occur when a task has the nonag tag"""
+        self.t("add one +other")
+        self.t.faketime("+10d")
+        self.t("add two +nonag")
+
+        code, out, err = self.t("2 done")
+
+        self.assertNotIn("NAG", err)
+
+    def test_nagging_nonag_bulk(self):
+        """Verify that nagging occurs even if some tasks in a bulk operation have a nonag tag"""
+        self.t("add one +other")
+        self.t.faketime("+10d")
+        self.t("add two +other")
+        self.t.faketime("+10d")
+        self.t("add three +nonag")
+
+        code, out, err = self.t("2-3 done")
+
+        self.assertIn("NAG", err)
+
+
 if __name__ == "__main__":
     from simpletap import TAPTestRunner
     unittest.main(testRunner=TAPTestRunner())