MDL-65032 mod_forum: Updates based on Jun's feedback

mod/forum/amd/src/lock_toggle.js

@@ -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);
 

mod/forum/classes/local/data_mappers/legacy/discussion.php

@@ -58,7 +58,7 @@ public function to_legacy_objects(array $discussions) : array {
                 'timestart' => $discussion->get_time_start(),
                 'timeend' => $discussion->get_time_end(),
                 'pinned' => $discussion->is_pinned(),
-                'locked' => $discussion->get_locked()
+                'timelocked' => $discussion->get_locked()
             ];
         }, $discussions);
     }

mod/forum/classes/local/entities/discussion.php

@@ -234,7 +234,7 @@ public function is_pinned() : bool {
     }
 
     /**
-     * Check if this discussion is pinned.
+     * Get the locked time of this discussion.
      *
      * @return bool
      */
@@ -254,11 +254,11 @@ public function is_locked() : bool {
     /**
      * 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);
     }
 
     /**

mod/forum/classes/local/entities/forum.php

@@ -539,16 +539,12 @@ public function has_lock_discussions_after() : bool {
     }
 
     /**
-     * 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 @@ public function is_due_date_reached() : bool {
 
         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);
+    }
 }

mod/forum/classes/local/exporters/discussion.php

@@ -64,6 +64,8 @@ protected static function define_other_properties() {
             '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 @@ protected static function define_other_properties() {
             'userstate' => [
                 'type' => [
                     'subscribed' => ['type' => PARAM_BOOL],
-                    'locked' => ['type' => PARAM_BOOL],
                 ],
             ],
             'capabilities' => [
@@ -182,6 +183,8 @@ protected function get_other_values(renderer_base $output) {
             '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 @@ protected function get_other_values(renderer_base $output) {
                 '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),

mod/forum/classes/local/renderers/discussion.php

@@ -201,10 +201,6 @@ public function render(
             $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 @@ private function get_subscription_button_html() : string {
         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.
      *

mod/forum/classes/local/vaults/db_table_vault.php

@@ -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;

mod/forum/db/services.php

@@ -142,6 +142,7 @@
         'description' => 'Set the lock state for the discussion',
         'type' => 'write',
         'ajax' => true,
+        'capabilities' => 'moodle/course:manageactivities',
         'services' => array(MOODLE_OFFICIAL_MOBILE_SERVICE),
     ),
 );

mod/forum/db/upgrade.php

@@ -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;

mod/forum/externallib.php

@@ -1532,22 +1532,25 @@ public static function set_lock_state($forumid, $discussionid, $targetstate) {
         $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 @@ public static function set_lock_state_parameters() {
      * @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.'),
+                ])
+        ]);
     }
 }

mod/forum/lang/en/forum.php

@@ -223,6 +223,7 @@
 $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';

mod/forum/lib.php

@@ -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.
  *

mod/forum/post.php

@@ -217,6 +217,11 @@
                     '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');
     }

mod/forum/templates/discussion_lock_toggle.mustache

@@ -30,31 +30,29 @@
 
     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>