From 5d4cafb7a662ec43d0a9e4bde8bbb39cf603ec96 Mon Sep 17 00:00:00 2001 From: Paul Beckingham Date: Tue, 5 Oct 2010 00:39:22 -0400 Subject: [PATCH] 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 | 32 +++++++++++++++++++++---------- src/command.cpp | 50 ++++++++++++++++++++++++++++-------------------- src/tests/wait.t | 2 +- 3 files changed, 52 insertions(+), 32 deletions(-) diff --git a/src/Task.cpp b/src/Task.cpp index 232cdbbce..ca61e5285 100644 --- a/src/Task.cpp +++ b/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 or characters."); // TODO i18n + throw std::string ("Cannot add a task that is blank, or contains or characters."); if (has ("due")) { Date due (::atoi (get ("due").c_str ())); - // Verify until > due - if (has ("until")) + // Verify wait < due + if (has ("wait")) { - Date until (::atoi (get ("until").c_str ())); - if (due > until) - throw std::string ("An 'until' date must be after a 'due' date."); // TODO i18n + Date wait (::atoi (get ("wait").c_str ())); + if (wait > due) + 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."); } //////////////////////////////////////////////////////////////////////////////// diff --git a/src/command.cpp b/src/command.cpp index 17a36dfc3..bf0247fee 100644 --- a/src/command.cpp +++ b/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); diff --git a/src/tests/wait.t b/src/tests/wait.t index a4be650f8..34dc00f63 100755 --- a/src/tests/wait.t +++ b/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';