From a967d2aa8c27ecfbba029307ea58f55243d12028 Mon Sep 17 00:00:00 2001 From: Frederic Massart Date: Tue, 12 Jan 2016 14:20:51 +0800 Subject: [PATCH] MDL-52485 tool_lp: Template due date can be changed at will The only rule is that it must be set in the future (though we permit due dates time minutes old), or it must be unset. --- admin/tool/lp/classes/plan.php | 5 +- admin/tool/lp/classes/template.php | 31 +++--- admin/tool/lp/classes/template_cohort.php | 7 +- admin/tool/lp/tests/api_test.php | 2 +- admin/tool/lp/tests/task_test.php | 2 +- admin/tool/lp/tests/template_test.php | 115 ++++++++++++++++++++++ 6 files changed, 134 insertions(+), 28 deletions(-) create mode 100644 admin/tool/lp/tests/template_test.php diff --git a/admin/tool/lp/classes/plan.php b/admin/tool/lp/classes/plan.php index b686fdc1c3d..c153c425c42 100644 --- a/admin/tool/lp/classes/plan.php +++ b/admin/tool/lp/classes/plan.php @@ -428,8 +428,9 @@ class plan extends persistent { */ protected function validate_duedate($value) { - // We do not check duedate when plan is draft, complete or unset. - if ($this->get_status() == self::STATUS_DRAFT + // We do not check duedate when plan is draft, complete, unset, or based on a template. + if ($this->is_based_on_template() + || $this->get_status() == self::STATUS_DRAFT || $this->get_status() == self::STATUS_COMPLETE || empty($value)) { return true; diff --git a/admin/tool/lp/classes/template.php b/admin/tool/lp/classes/template.php index 8c18ab89f84..869d4d67576 100644 --- a/admin/tool/lp/classes/template.php +++ b/admin/tool/lp/classes/template.php @@ -38,9 +38,6 @@ class template extends persistent { const TABLE = 'tool_lp_template'; - /** One day threshold **/ - const DUEDATE_THRESHOLD = 86400; - /** @var template object before update. */ protected $beforeupdate = null; @@ -166,7 +163,10 @@ class template extends persistent { /** * Validate the due date. - * We set a threshold to avoid creating templates with duedate too soon, so the plans can be created safely. + * + * The due date can always be changed, but when it is it must be: + * - unset + * - set in the future. * * @param int $value The due date. * @return bool|lang_string @@ -176,25 +176,18 @@ class template extends persistent { // During update. if ($this->get_id()) { $before = $this->beforeupdate->get_duedate(); - $haschanged = $before != $value; - if (!empty($before) && $before <= time() && $haschanged) { - // We cannot set a due date after it was reached. - return new lang_string('errorcannotchangeapastduedate', 'tool_lp'); + // The value has not changed, then it's always OK. + if ($before == $value) { + return true; } } - // During create and update when due date in the past or too soon. - if (!empty($value)) { - if ($value <= time()) { - // We cannot set the date in the past, but we can leave it empty. - return new lang_string('errorcannotsetduedateinthepast', 'tool_lp'); - } - - if ($value <= time() + self::DUEDATE_THRESHOLD) { - // We cannot set the date too soon, but we can leave it empty. - return new lang_string('errorcannotsetduedatetoosoon', 'tool_lp'); - } + // During create and update, the date must be set in the future, or not set. + if (!empty($value) && $value <= time() - 600) { + // We cannot set the date in the past. But we allow for 10 minutes of margin so that + // a user can set the due date to "now" without risking to hit a validation error. + return new lang_string('errorcannotsetduedateinthepast', 'tool_lp'); } return true; diff --git a/admin/tool/lp/classes/template_cohort.php b/admin/tool/lp/classes/template_cohort.php index 5dcdd230db1..16ac255fac6 100644 --- a/admin/tool/lp/classes/template_cohort.php +++ b/admin/tool/lp/classes/template_cohort.php @@ -163,21 +163,18 @@ class template_cohort extends persistent { global $DB; // Safe enough with 2 hours. - $time = time() + self::DUEDATE_THRESHOLD; $skipsql = !$unlinkedaremissing ? '(t.id = p.templateid OR t.id = p.origtemplateid)' : 't.id = p.templateid'; // TODO MDL-52526 only unexpired template are considered and fix the time()+1 duedate issue. $sql = "SELECT cm.userid, t.* FROM {cohort_members} cm JOIN {" . self::TABLE . "} tc ON cm.cohortid = tc.cohortid - JOIN {" . template::TABLE . "} t ON (tc.templateid = t.id - AND t.visible = 1 - AND (t.duedate > :time OR t.duedate = 0)) + JOIN {" . template::TABLE . "} t ON (tc.templateid = t.id AND t.visible = 1) LEFT JOIN {" . plan::TABLE . "} p ON (cm.userid = p.userid AND $skipsql) WHERE p.id IS NULL ORDER BY t.id"; - $results = $DB->get_records_sql($sql, array('time' => $time)); + $results = $DB->get_records_sql($sql); $missingplans = array(); foreach ($results as $usertemplate) { diff --git a/admin/tool/lp/tests/api_test.php b/admin/tool/lp/tests/api_test.php index f7d201cb763..e8f16ec88a3 100644 --- a/admin/tool/lp/tests/api_test.php +++ b/admin/tool/lp/tests/api_test.php @@ -681,7 +681,7 @@ class tool_lp_api_testcase extends advanced_testcase { $data->shortname = 'Awesome!'; $data->description = 'This is too awesome!'; $data->descriptionformat = FORMAT_HTML; - $data->duedate = $time + tool_lp\template::DUEDATE_THRESHOLD + 200; + $data->duedate = $time + 200; api::update_template($data); $tpl1->read(); diff --git a/admin/tool/lp/tests/task_test.php b/admin/tool/lp/tests/task_test.php index 84707b678e7..8f8a6982539 100644 --- a/admin/tool/lp/tests/task_test.php +++ b/admin/tool/lp/tests/task_test.php @@ -128,7 +128,7 @@ class tool_lp_task_testcase extends advanced_testcase { $user5 = $dg->create_user(); $cohort = $dg->create_cohort(); - $tpl = $lpg->create_template(array('duedate' => time() + tool_lp\template::DUEDATE_THRESHOLD + 400)); + $tpl = $lpg->create_template(array('duedate' => time() + 400)); // Add 2 users to the cohort. cohort_add_member($cohort->id, $user1->id); diff --git a/admin/tool/lp/tests/template_test.php b/admin/tool/lp/tests/template_test.php new file mode 100644 index 00000000000..e28a463b7cc --- /dev/null +++ b/admin/tool/lp/tests/template_test.php @@ -0,0 +1,115 @@ +. + +/** + * Template persistent class tests. + * + * @package tool_lp + * @copyright 2016 Frédéric Massart - FMCorz.net + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); +global $CFG; + +use tool_lp\template; + +/** + * Template persistent testcase. + * + * @package tool_lp + * @copyright 2016 Frédéric Massart - FMCorz.net + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class tool_lp_template_testcase extends advanced_testcase { + + public function test_validate_duedate() { + global $DB; + + $this->resetAfterTest(); + $tpl = $this->getDataGenerator()->get_plugin_generator('tool_lp')->create_template(); + + // No due date -> pass. + $tpl->set_duedate(0); + $this->assertTrue($tpl->is_valid()); + + // Setting new due date in the past -> fail. + $tpl->set_duedate(1); + $errors = $tpl->get_errors(); + $this->assertCount(1, $errors); + $this->assertArrayHasKey('duedate', $errors); + + // Setting new due date in very close past -> pass. + $tpl->set_duedate(time() - 10); + $this->assertTrue($tpl->is_valid()); + + // Setting new due date in future -> pass. + $tpl->set_duedate(time() + 600); + $this->assertTrue($tpl->is_valid()); + + // Save due date in the future. + $tpl->update(); + + // Going from future date to past -> fail. + $tpl->set_duedate(1); + $errors = $tpl->get_errors(); + $this->assertCount(1, $errors); + $this->assertArrayHasKey('duedate', $errors); + + // Going from future date to none -> pass. + $tpl->set_duedate(0); + $this->assertTrue($tpl->is_valid()); + + // Going from future date to other future -> pass. + $tpl->set_duedate(time() + 6000); + $this->assertTrue($tpl->is_valid()); + + // Going from future date to close past -> pass. + $tpl->set_duedate(time() - 10); + $this->assertTrue($tpl->is_valid()); + + // Mocking past due date. + $record = $tpl->to_record(); + $record->duedate = 1; + $DB->update_record(template::TABLE, $record); + $tpl->read(); + $this->assertEquals(1, $tpl->get_duedate()); + + // Not changing the past due date -> pass. + // Note: changing visibility to force validation. + $tpl->set_visible(0); + $tpl->set_visible(1); + $this->assertTrue($tpl->is_valid()); + + // Changing past due date to other past -> fail. + $tpl->set_duedate(10); + $errors = $tpl->get_errors(); + $this->assertCount(1, $errors); + $this->assertArrayHasKey('duedate', $errors); + + // Changing past due date close past -> pass. + $tpl->set_duedate(time() + 10); + $this->assertTrue($tpl->is_valid()); + + // Changing past due date to future -> pass. + $tpl->set_duedate(time() + 1000); + $this->assertTrue($tpl->is_valid()); + + // Changing past due date to none -> pass. + $tpl->set_duedate(0); + $this->assertTrue($tpl->is_valid()); + } +}