diff --git a/src/commands/CmdDone.cpp b/src/commands/CmdDone.cpp index 181314950..4f9b9f5ef 100644 --- a/src/commands/CmdDone.cpp +++ b/src/commands/CmdDone.cpp @@ -68,11 +68,11 @@ int CmdDone::execute (std::string&) // Accumulated project change notifications. std::map projectChanges; - Task& lowest = filtered.front(); - if(filtered.size() > 1) { feedback_affected("This command will alter {1} tasks.", filtered.size()); } + + std::vector 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) diff --git a/src/commands/CmdStart.cpp b/src/commands/CmdStart.cpp index 59567ee46..20085bf54 100644 --- a/src/commands/CmdStart.cpp +++ b/src/commands/CmdStart.cpp @@ -68,10 +68,11 @@ int CmdStart::execute (std::string&) // Accumulated project change notifications. std::map projectChanges; - bool nagged = false; if(filtered.size() > 1) { feedback_affected("This command will alter {1} tasks.", filtered.size()); } + + std::vector 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 != "") diff --git a/src/main.h b/src/main.h index 38a217137..324b4c2ed 100644 --- a/src/main.h +++ b/src/main.h @@ -48,7 +48,7 @@ void updateRecurrenceMask (Task&); void handleRecurrence2 (); // nag.cpp -bool nag (Task&); +void nag (std::vector &); // rules.cpp void initializeColorRules (); diff --git a/src/nag.cpp b/src/nag.cpp index ca87a6448..96bce271d 100644 --- a/src/nag.cpp +++ b/src/nag.cpp @@ -24,41 +24,37 @@ // //////////////////////////////////////////////////////////////////////////////// +#include #include #include +#include +#include +#include +#include //////////////////////////////////////////////////////////////////////////////// -// 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 & 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; } //////////////////////////////////////////////////////////////////////////////// diff --git a/test/nag.t b/test/nag.t index cc623732c..ae91c20bb 100755 --- a/test/nag.t +++ b/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())