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

      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.

        Gliffy Diagrams

          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: