From b33a99a7481a39b25a8e16f0be5042b094850e24 Mon Sep 17 00:00:00 2001 From: Korrat Date: Thu, 22 Jul 2021 17:08:00 +0200 Subject: [PATCH] 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 | 11 ++++--- src/commands/CmdStart.cpp | 10 ++++-- src/main.h | 2 +- src/nag.cpp | 44 ++++++++++++--------------- test/nag.t | 64 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 98 insertions(+), 33 deletions(-) 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())