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()); + } +}