MDL-65032 mod_forum: Updates based on Jun's feedback
authorPeter <peter@moodle.com>
Mon, 8 Apr 2019 01:54:12 +0000 (09:54 +0800)
committerPeter <peter@moodle.com>
Wed, 24 Apr 2019 04:49:41 +0000 (12:49 +0800)
20 files changed:
mod/forum/amd/build/lock_toggle.min.js
mod/forum/amd/src/lock_toggle.js
mod/forum/classes/local/data_mappers/legacy/discussion.php
mod/forum/classes/local/entities/discussion.php
mod/forum/classes/local/entities/forum.php
mod/forum/classes/local/exporters/discussion.php
mod/forum/classes/local/renderers/discussion.php
mod/forum/classes/local/vaults/db_table_vault.php
mod/forum/db/services.php
mod/forum/db/upgrade.php
mod/forum/externallib.php
mod/forum/lang/en/forum.php
mod/forum/lib.php
mod/forum/post.php
mod/forum/templates/discussion_lock_toggle.mustache
mod/forum/templates/forum_discussion.mustache
mod/forum/tests/behat/discussion_lock.feature
mod/forum/tests/entities_discussion_test.php
mod/forum/tests/externallib_test.php
mod/forum/version.php

index 930ec3d..fd17819 100644 (file)
Binary files a/mod/forum/amd/build/lock_toggle.min.js and b/mod/forum/amd/build/lock_toggle.min.js differ
index a002665..37ca209 100644 (file)
@@ -48,11 +48,8 @@ define([
             var state = toggleElement.data('state');
 
             Repository.setDiscussionLockState(forumId, discussionId, state)
-                .then(function(context) {
-                    return Templates.render('mod_forum/discussion_lock_toggle', context);
-                })
-                .then(function(html, js) {
-                    return Templates.replaceNode(toggleElement, html, js);
+                .then(function() {
+                    return location.reload();
                 })
                 .catch(Notification.exception);
 
index acfc43b..2973fa4 100644 (file)
@@ -58,7 +58,7 @@ class discussion {
                 'timestart' => $discussion->get_time_start(),
                 'timeend' => $discussion->get_time_end(),
                 'pinned' => $discussion->is_pinned(),
-                'locked' => $discussion->get_locked()
+                'timelocked' => $discussion->get_locked()
             ];
         }, $discussions);
     }
index 418b121..b433401 100644 (file)
@@ -234,7 +234,7 @@ class discussion {
     }
 
     /**
-     * Check if this discussion is pinned.
+     * Get the locked time of this discussion.
      *
      * @return bool
      */
@@ -254,11 +254,11 @@ class discussion {
     /**
      * Set the locked timestamp
      *
-     * @param int $timestamp
+     * @param int $timestamp The value we want to store into 'locked'
      */
     public function toggle_locked_state(int $timestamp) {
-        // If it is locked already then unlock else set it to the timestamp.
-        $this->timelocked = ($this->timelocked ? 0 : $timestamp);
+        // Check the current value against what we want the value to be i.e. '$timestamp'.
+        $this->timelocked = ($this->timelocked && $timestamp ? $this->timelocked : $timestamp);
     }
 
     /**
index ee4d572..66d6364 100644 (file)
@@ -539,16 +539,12 @@ class forum {
     }
 
     /**
-     * Is the discussion locked?
+     * Check whether the discussion is locked based on forum's time based locking criteria
      *
-     * @param discussion_entity $discussion The discussion to check
+     * @param discussion_entity $discussion
      * @return bool
      */
-    public function is_discussion_locked(discussion_entity $discussion) : bool {
-        if ($discussion->is_locked()) {
-            return true;
-        }
-
+    public function is_discussion_time_locked(discussion_entity $discussion) : bool {
         if (!$this->has_lock_discussions_after()) {
             return false;
         }
@@ -622,4 +618,18 @@ class forum {
 
         return false;
     }
+
+    /**
+     * Is the discussion locked? - Takes into account both discussion settings AND forum's criteria
+     *
+     * @param discussion_entity $discussion The discussion to check
+     * @return bool
+     */
+    public function is_discussion_locked(discussion_entity $discussion) : bool {
+        if ($discussion->is_locked()) {
+            return true;
+        }
+
+        return $this->is_discussion_time_locked($discussion);
+    }
 }
index 7d93a02..e7d2e4e 100644 (file)
@@ -64,6 +64,8 @@ class discussion extends exporter {
             'id' => ['type' => PARAM_INT],
             'forumid' => ['type' => PARAM_INT],
             'pinned' => ['type' => PARAM_BOOL],
+            'locked' => ['type' => PARAM_BOOL],
+            'istimelocked' => ['type' => PARAM_BOOL],
             'name' => ['type' => PARAM_TEXT],
             'group' => [
                 'optional' => true,
@@ -94,7 +96,6 @@ class discussion extends exporter {
             'userstate' => [
                 'type' => [
                     'subscribed' => ['type' => PARAM_BOOL],
-                    'locked' => ['type' => PARAM_BOOL],
                 ],
             ],
             'capabilities' => [
@@ -182,6 +183,8 @@ class discussion extends exporter {
             'id' => $discussion->get_id(),
             'forumid' => $forum->get_id(),
             'pinned' => $discussion->is_pinned(),
+            'locked' => $forum->is_discussion_locked($discussion),
+            'istimelocked' => $forum->is_discussion_time_locked($discussion),
             'name' => format_string($discussion->get_name(), true, [
                 'context' => $this->related['context']
             ]),
@@ -192,8 +195,7 @@ class discussion extends exporter {
                 'locked' => $discussion->get_locked()
             ],
             'userstate' => [
-                'subscribed' => \mod_forum\subscriptions::is_subscribed($user->id, $forumrecord, $discussion->get_id()),
-                'locked' => $discussion->is_locked()
+                'subscribed' => \mod_forum\subscriptions::is_subscribed($user->id, $forumrecord, $discussion->get_id())
             ],
             'capabilities' => [
                 'subscribe' => $capabilitymanager->can_subscribe_to_discussion($user, $discussion),
index 1b2ccf1..500bc75 100644 (file)
@@ -201,10 +201,6 @@ class discussion {
             $exporteddiscussion['html']['pindiscussion'] = $this->get_pin_discussion_html();
         }
 
-        if ($capabilities['manage']) {
-            $exporteddiscussion['html']['lockdiscussion'] = $this->get_lock_discussion_button_html();
-        }
-
         return $this->renderer->render_from_template('mod_forum/forum_discussion', $exporteddiscussion);
     }
 
@@ -278,24 +274,6 @@ class discussion {
         return $html;
     }
 
-    /**
-     * Get the HTML to render the subscription button.
-     *
-     * @return string
-     */
-    private function get_lock_discussion_button_html() : string {
-        global $PAGE;
-
-        $forumrecord = $this->forumrecord;
-        $discussionrecord = $this->discussionrecord;
-
-        $html = html_writer::div(
-            forum_get_lock_discussion_icon($forumrecord, $discussionrecord, null, true),
-            'discussionlock'
-        );
-        return $html;
-    }
-
     /**
      * Get the HTML to render the move discussion selector and button.
      *
index dd05c0a..f7fc389 100644 (file)
@@ -53,7 +53,7 @@ abstract class db_table_vault {
     public function __construct(
         moodle_database $db,
         entity_factory $entityfactory,
-        $legacyfactory
+        object $legacyfactory
     ) {
         $this->db = $db;
         $this->entityfactory = $entityfactory;
index d031e92..3506d12 100644 (file)
@@ -142,6 +142,7 @@ $functions = array(
         'description' => 'Set the lock state for the discussion',
         'type' => 'write',
         'ajax' => true,
+        'capabilities' => 'moodle/course:manageactivities',
         'services' => array(MOODLE_OFFICIAL_MOBILE_SERVICE),
     ),
 );
index 2ea3fa1..7addc73 100644 (file)
@@ -140,7 +140,7 @@ function xmldb_forum_upgrade($oldversion) {
         upgrade_mod_savepoint(true, 2019040400, 'forum');
     }
 
-    if ($oldversion < 2019040401) {
+    if ($oldversion < 2019040402) {
         // Define field deleted to be added to forum_posts.
         $table = new xmldb_table('forum_discussions');
         $field = new xmldb_field('timelocked', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, '0', 'pinned');
@@ -151,7 +151,7 @@ function xmldb_forum_upgrade($oldversion) {
         }
 
         // Forum savepoint reached.
-        upgrade_mod_savepoint(true, 2019040401, 'forum');
+        upgrade_mod_savepoint(true, 2019040402, 'forum');
     }
 
     return true;
index 5631932..4eb6e97 100644 (file)
@@ -1532,22 +1532,25 @@ class mod_forum_external extends external_api {
         $vaultfactory = mod_forum\local\container::get_vault_factory();
         $forumvault = $vaultfactory->get_forum_vault();
         $forum = $forumvault->get_from_id($params['forumid']);
-        // If the targetstate(currentstate) is not 0 then it should be set to the current time.
-        $targetstate = $targetstate ? 0 : time();
-        self::validate_context($forum->get_context());
 
         $managerfactory = mod_forum\local\container::get_manager_factory();
         $capabilitymanager = $managerfactory->get_capability_manager($forum);
+        if (!$capabilitymanager->can_manage_forum($USER)) {
+            throw new moodle_exception('errorcannotlock', 'forum');
+        }
+
+        // If the targetstate(currentstate) is not 0 then it should be set to the current time.
+        $lockedvalue = $targetstate ? 0 : time();
+        self::validate_context($forum->get_context());
+
         $discussionvault = $vaultfactory->get_discussion_vault();
         $discussion = $discussionvault->get_from_id($params['discussionid']);
 
         // If the current state doesn't equal the desired state then update the current.
         // state to the desired state.
-        if ($capabilitymanager->can_manage_forum($USER)) {
-            $discussion->toggle_locked_state($targetstate);
-            $response = $discussionvault->update_discussion($discussion);
-            $discussion = !$response ? $response : $discussion;
-        }
+        $discussion->toggle_locked_state($lockedvalue);
+        $response = $discussionvault->update_discussion($discussion);
+        $discussion = !$response ? $response : $discussion;
 
         $exporterfactory = mod_forum\local\container::get_exporter_factory();
         $exporter = $exporterfactory->get_discussion_exporter($USER, $forum, $discussion);
@@ -1575,6 +1578,12 @@ class mod_forum_external extends external_api {
      * @return external_description
      */
     public static function set_lock_state_returns() {
-        return \mod_forum\local\exporters\discussion::get_read_structure();
+        return new external_single_structure([
+                'id' => new external_value(PARAM_INT, 'The discussion we are locking.'),
+                'locked' => new external_value(PARAM_BOOL, 'The locked state of the discussion.'),
+                'times' => new external_single_structure([
+                    'locked' => new external_value(PARAM_INT, 'The locked time of the discussion.'),
+                ])
+        ]);
     }
 }
index 425bdde..15d9a11 100644 (file)
@@ -223,6 +223,7 @@ $string['erroremptymessage'] = 'Post message cannot be empty';
 $string['erroremptysubject'] = 'Post subject cannot be empty.';
 $string['errorenrolmentrequired'] = 'You must be enrolled in this course to access this content';
 $string['errorwhiledelete'] = 'An error occurred while deleting record.';
+$string['errorcannotlock'] = 'You do not have the permission to lock discussions.';
 $string['eventassessableuploaded'] = 'Some content has been posted.';
 $string['everyonecanchoose'] = 'Everyone can choose to be subscribed';
 $string['everyonecannowchoose'] = 'Everyone can now choose to be subscribed';
index 5015625..7a42f63 100644 (file)
@@ -1824,7 +1824,7 @@ function forum_get_discussions($cm, $forumsort="", $fullpost=true, $unused=-1, $
     }
 
     $discussionfields = "d.id as discussionid, d.course, d.forum, d.name, d.firstpost, d.userid, d.groupid, d.assessed," .
-    " d.timemodified, d.usermodified, d.timestart, d.timeend, d.pinned, d.locked";
+    " d.timemodified, d.usermodified, d.timestart, d.timeend, d.pinned, d.timelocked";
 
     $allnames = get_all_user_name_fields(true, 'u');
     $sql = "SELECT $postdata, $discussionfields,
@@ -2583,66 +2583,6 @@ function forum_print_discussion_header(&$post, $forum, $group = -1, $datestring
 
 }
 
-/**
- * Return the markup for the discussion lock toggling icon.
- * @param stdClass $forum forum record
- * @param stdClass $discussion discussion record
- * @param null $returnurl the return url to use
- * @param bool $includetext Whether or not to include the text with the icon
- * @return string
- * @throws coding_exception
- * @throws moodle_exception
- */
-function forum_get_lock_discussion_icon($forum, $discussion, $returnurl = null, $includetext = false) {
-    global $USER, $OUTPUT, $PAGE;
-
-    if ($returnurl === null && $PAGE->url) {
-        $returnurl = $PAGE->url->out();
-    }
-
-    $discussionid = $discussion->id;
-    $lockstatus = forum_discussion_is_locked($forum, $discussion);
-    $subscriptionlink = new moodle_url('/mod/forum/lockdiscussion.php', array(
-        'sesskey' => sesskey(),
-        'id' => $forum->id,
-        'd' => $discussion->id,
-        'returnurl' => $returnurl,
-    ));
-
-    if ($lockstatus) {
-        $output = $OUTPUT->pix_icon('t/unlock', get_string('clicktounlockdiscussion', 'forum'), 'core');
-        if ($includetext) {
-            $output .= get_string('locked', 'mod_forum');
-        }
-
-        return html_writer::link($subscriptionlink, $output, array(
-            'title' => get_string('clicktounlockdiscussion', 'forum'),
-            'class' => 'iconsmall',
-            'data-forumid' => $forum->id,
-            'data-discussionid' => $discussionid,
-            'data-action' => 'toggle',
-            'data-type' => 'lock-toggle',
-            'data-state' => $discussion->locked
-        ));
-
-    } else {
-        $output = $OUTPUT->pix_icon('t/lock', get_string('clicktolockdiscussion', 'forum'), 'core');
-        if ($includetext) {
-            $output .= get_string('notlocked', 'mod_forum');
-        }
-
-        return html_writer::link("#", $output, array(
-            'title' => get_string('clicktolockdiscussion', 'forum'),
-            'class' => 'iconsmall',
-            'data-forumid' => $forum->id,
-            'data-discussionid' => $discussionid,
-            'data-action' => 'toggle',
-            'data-type' => 'lock-toggle',
-            'data-state' => $discussion->locked
-        ));
-    }
-}
-
 /**
  * Return the markup for the discussion subscription toggling icon.
  *
index 079f7ec..01d70f3 100644 (file)
@@ -217,6 +217,11 @@ if (!empty($forum)) {
                     'returnurl' => '/mod/forum/view.php?f=' . $forum->id)),
                     get_string('youneedtoenrol'));
             }
+
+            // The forum has been locked. Just redirect back to the discussion page.
+            if (forum_discussion_is_locked($forum, $discussion)) {
+                redirect(new moodle_url('/mod/forum/discuss.php', array('d' => $discussion->id)));
+            }
         }
         print_error('nopostforum', 'forum');
     }
index 679ee53..396fafd 100644 (file)
 
     Example context (json):
     {
+        "id": 0,
+        "locked": 1
     }
 }}
-{{#capabilities.manage}}
-    <a
-        class="iconsmall"
-        data-type="lock-toggle"
-        data-action="toggle"
-        data-discussionid="{{id}}"
-        data-forumid="{{forumid}}"
-        data-state="{{times.locked}}"
-        tabindex="-1"
-        {{#userstate.locked}}
-            title="{{#str}}locked, forum{{/str}}"
-        {{/userstate.locked}}
-        {{^userstate.locked}}
-            title="{{#str}}notlocked, forum{{/str}}"
-        {{/userstate.locked}}
-    >
-        {{#userstate.locked}}
-            {{#pix}}t/unlock, core, {{#str}}clicktounlockdiscussion, forum{{/str}}{{/pix}}{{#str}}locked, forum{{/str}}
-        {{/userstate.locked}}
-        {{^userstate.locked}}
-            {{#pix}}t/lock, core, {{#str}}clicktolockdiscussion, forum{{/str}}{{/pix}}{{#str}}notlocked, forum{{/str}}
-        {{/userstate.locked}}
-    </a>
-{{/capabilities.manage}}
-
-
+<a
+    class="iconsmall"
+    data-type="lock-toggle"
+    data-action="toggle"
+    data-discussionid="{{id}}"
+    data-forumid="{{forumid}}"
+    data-state="{{locked}}"
+    href="#"
+    {{#locked}}
+        title="{{#str}}clicktounlockdiscussion, forum{{/str}}"
+    {{/locked}}
+    {{^locked}}
+        title="{{#str}}clicktolockdiscussion, forum{{/str}}"
+    {{/locked}}
+>
+    {{#locked}}
+        {{#pix}}t/unlock, core, {{#str}}clicktounlockdiscussion, forum{{/str}}{{/pix}}{{#str}}locked, forum{{/str}}
+    {{/locked}}
+    {{^locked}}
+        {{#pix}}t/lock, core, {{#str}}clicktolockdiscussion, forum{{/str}}{{/pix}}{{#str}}notlocked, forum{{/str}}
+    {{/locked}}
+</a>
\ No newline at end of file
index 5c84339..0555e76 100644 (file)
 <div id="discussion-container-{{uniqid}}" data-content="forum-discussion">
 {{#html}}
     <div class="d-flex flex-wrap flex-row-reverse m-b-1" data-container="discussion-tools" style="text-align: right;">
-        <div class="pl-1">{{{lockdiscussion}}}</div>
+        {{#capabilities.manage}}
+            {{^timelocked}}
+            <div class="pl-1 discussionlock">
+                {{> forum/discussion_lock_toggle }}
+            </div>
+            {{/timelocked}}
+        {{/capabilities.manage}}
         <div class="pl-1">{{{subscribe}}}</div>
     </div>
     {{{neighbourlinks}}}
index 3c33f8c..0fcce46 100644 (file)
@@ -38,7 +38,6 @@ Feature: As a teacher, you can manually lock individual discussions when viewing
     And I follow "Lock"
     Then "a[@title='Lock']" "css_element" should not be visible
     Then "Locked" "link" should be visible
-    And I reload the page
     Then I should see "This discussion has been locked so you can no longer reply to it."
     And I follow "Discussion 2"
     Then I should not see "This discussion has been locked so you can no longer reply to it."
index 9c01ac0..c3bb6f8 100644 (file)
@@ -148,7 +148,8 @@ class mod_forum_entities_discussion_testcase extends advanced_testcase {
             $basetime,
             $starttime,
             $endtime,
-            false
+            false,
+            0
         );
         $CFG->forum_enabletimedposts = true;
 
index 7db8013..90340a2 100644 (file)
@@ -1451,22 +1451,24 @@ class mod_forum_external_testcase extends externallib_advanced_testcase {
         $this->getDataGenerator()->enrol_user($user->id, $course->id, $studentrole->id, 'manual');
 
         // Only a teacher should be able to lock a discussion.
-        $result = mod_forum_external::set_lock_state($forum->id, $discussion->id, 0);
-        $result = external_api::clean_returnvalue(mod_forum_external::set_lock_state_returns(), $result);
-        $this->assertFalse($result['userstate']['locked']);
-        $this->assertEquals('0', $result['times']['locked']);
+        try {
+            $result = mod_forum_external::set_lock_state($forum->id, $discussion->id, 0);
+            $this->fail('Exception expected due to missing capability.');
+        } catch (moodle_exception $e) {
+            $this->assertEquals('errorcannotlock', $e->errorcode);
+        }
 
         // Set the lock.
         self::setAdminUser();
         $result = mod_forum_external::set_lock_state($forum->id, $discussion->id, 0);
         $result = external_api::clean_returnvalue(mod_forum_external::set_lock_state_returns(), $result);
-        $this->assertTrue($result['userstate']['locked']);
+        $this->assertTrue($result['locked']);
         $this->assertNotEquals(0, $result['times']['locked']);
 
         // Unset the lock.
         $result = mod_forum_external::set_lock_state($forum->id, $discussion->id, time());
         $result = external_api::clean_returnvalue(mod_forum_external::set_lock_state_returns(), $result);
-        $this->assertFalse($result['userstate']['locked']);
+        $this->assertFalse($result['locked']);
         $this->assertEquals('0', $result['times']['locked']);
     }
 
index 587bf1e..4672f96 100644 (file)
@@ -24,6 +24,6 @@
 
 defined('MOODLE_INTERNAL') || die();
 
-$plugin->version   = 2019040401;       // The current module version (Date: YYYYMMDDXX)
+$plugin->version   = 2019040402;       // The current module version (Date: YYYYMMDDXX)
 $plugin->requires  = 2018112800;       // Requires this Moodle version
 $plugin->component = 'mod_forum';      // Full name of the plugin (used for diagnostics)