Moodle
  1. Moodle
  2. MDL-39182

When limiting users' total posts the warning when the user has reached the warning limit is not displayed in an obvious place.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3.6, 2.4
    • Fix Version/s: 2.5
    • Component/s: Forum
    • Labels:
    • Testing Instructions:
      Hide
      1. Login as a teacher and create a forum with a time period for blocking set.
      2. Set a post threshold for blocking and for warning.
      3. Ensure the student role does not have capability "mod/forum:postwithoutthrottling" within the forum.
      4. Login as a student and post as many messages as the number you set in the warning field.
      5. Click to post again and ensure a warning is shown saying you are about to reach the limit with it in an obvious place.
      6. Post until you reach the threshold for blocking.
      7. Post once you have reached this limit and ensure an error is thrown.
      Show
      Login as a teacher and create a forum with a time period for blocking set. Set a post threshold for blocking and for warning. Ensure the student role does not have capability "mod/forum:postwithoutthrottling" within the forum. Login as a student and post as many messages as the number you set in the warning field. Click to post again and ensure a warning is shown saying you are about to reach the limit with it in an obvious place. Post until you reach the threshold for blocking. Post once you have reached this limit and ensure an error is thrown.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-39182_master
    • Rank:
      49785

      Description

      See MDLQA-5277.

      When limiting users' total posts the warning is displayed at the top of the page which gets hidden as the page automatically scrolls to the HTML editor, which does not make it very useful unless the user happens to scroll to the top of the page before posting.

        Issue Links

          Activity

          Hide
          Rajesh Taneja added a comment -

          Patch Looks good mark, few things you might want to consider before pushing it for integration:

          1. As you are changing php docs, it will be nice to replace object with stdClass
            #L0L7088 and #L0R7089
          2. #L0L7088 object should be replaced with int|stdClass, as $forum can be object or int.
          3. #L0R7092 mixed is not valid coding style, it should be bool|string
          4. #L0L7098 not sure if removing this check is required
          5. I can see you have used double quotes in sql. It is recommended to use single quotes #L0R7121
          6. I can see "Add a new post" button is always visible even though user has reached threshold. It will be nice not to show that button, as by clicking this button, user see error msg.
          7. For fixing above, your patch will not be helpful as it doesn't return proper status and always returns false.
          8. Also, it will be nice to show status on forum page that user has reached his/her threshold, so removing button makes sense to user.

          In all changes looks good, and looking at usage of it seems safe to alter api.
          But personally, I will not alter the behaviour of api like this during freeze. It will be nice to pass an extra optional param by reference and get string from there. Leaving this upto you to decide.

          Not related:

          1. It will be nice to have $a initialized in forum/view.php - Line 159 as it's throwing string standards warning. (seems you have fixed this another issue.)
          Show
          Rajesh Taneja added a comment - Patch Looks good mark, few things you might want to consider before pushing it for integration: As you are changing php docs, it will be nice to replace object with stdClass #L0L7088 and #L0R7089 #L0L7088 object should be replaced with int|stdClass, as $forum can be object or int. #L0R7092 mixed is not valid coding style, it should be bool|string #L0L7098 not sure if removing this check is required I can see you have used double quotes in sql. It is recommended to use single quotes #L0R7121 I can see "Add a new post" button is always visible even though user has reached threshold. It will be nice not to show that button, as by clicking this button, user see error msg. For fixing above, your patch will not be helpful as it doesn't return proper status and always returns false. Also, it will be nice to show status on forum page that user has reached his/her threshold, so removing button makes sense to user. In all changes looks good, and looking at usage of it seems safe to alter api. But personally, I will not alter the behaviour of api like this during freeze. It will be nice to pass an extra optional param by reference and get string from there. Leaving this upto you to decide. Not related: It will be nice to have $a initialized in forum/view.php - Line 159 as it's throwing string standards warning. (seems you have fixed this another issue.)
          Hide
          Mark Nelson added a comment -

          Hi Raj, thanks. I have addressed your issues. I now return an object if a warning or error should be thrown, from this object the developer can determine what needs to be done.

          Show
          Mark Nelson added a comment - Hi Raj, thanks. I have addressed your issues. I now return an object if a warning or error should be thrown, from this object the developer can determine what needs to be done.
          Hide
          Rajesh Taneja added a comment -

          Thanks Mark, Patch looks good.

          1. #L2R83 as per design, form definition should not check/throw error. It will be nice to handle it in post.php
          2. #L0R7088 it will be nice to write few words about @param $forum in php_docs.
          Show
          Rajesh Taneja added a comment - Thanks Mark, Patch looks good. #L2R83 as per design, form definition should not check/throw error. It will be nice to handle it in post.php #L0R7088 it will be nice to write few words about @param $forum in php_docs.
          Hide
          Mark Nelson added a comment -

          Hi Raj, thanks. All done now. Submitting to integration.

          Show
          Mark Nelson added a comment - Hi Raj, thanks. All done now. Submitting to integration.
          Hide
          Damyon Wiese added a comment -

          Thanks Mark, this has been integrated to master now.

          Show
          Damyon Wiese added a comment - Thanks Mark, this has been integrated to master now.
          Hide
          Frédéric Massart added a comment -

          Failed for consideration.

          The testing instructions do not fail, but if I stay on the page that displays "Your post was successfully added.", then hit "F5" and confirm the post re-submission, I can post again. If we do not want to prevent this, then the test is passing.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Failed for consideration. The testing instructions do not fail, but if I stay on the page that displays "Your post was successfully added.", then hit "F5" and confirm the post re-submission, I can post again. If we do not want to prevent this, then the test is passing. Cheers, Fred
          Hide
          Mark Nelson added a comment -

          Hi Fred, thanks for spotting that. However, it occurs in previous versions of the forum, so is going to need a patch for 2.3, 2.4 and master - I created a separate issue for it here MDL-39303. I say we pass this and focus on what you discovered on the separate issue.

          Show
          Mark Nelson added a comment - Hi Fred, thanks for spotting that. However, it occurs in previous versions of the forum, so is going to need a patch for 2.3, 2.4 and master - I created a separate issue for it here MDL-39303 . I say we pass this and focus on what you discovered on the separate issue.
          Hide
          Damyon Wiese added a comment -

          Passing this re: comments (new bug opened).

          Show
          Damyon Wiese added a comment - Passing this re: comments (new bug opened).
          Hide
          Dan Poltawski added a comment -

          Can we reset the linked QA test?

          Show
          Dan Poltawski added a comment - Can we reset the linked QA test?
          Hide
          Mark Nelson added a comment -

          Nope, there are other issues that need to be integrated before we can do that.

          Show
          Mark Nelson added a comment - Nope, there are other issues that need to be integrated before we can do that.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I feel myself really alone tonight! So was time to push your fixes upstream!

          "Lest we forget. We will remember them."

          Thanks and ciao!

          Show
          Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: