Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-30260

Re-implementation of emailstop is dangerously broken

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.6, 2.1.3, 2.2
    • Fix Version/s: 2.0.7, 2.1.4
    • Component/s: Messages
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      Testing this is mostly a case of checking that nothing was broken. You'll need 2 users.

      Some things to be aware of:
      1) when we say popup in the context of message notifications we mean a small overlay that appears in the bottom right of the screen (assuming the them hasnt moved it).
      2) For performance reasons the query to check whether there are pending popup notifications will not be run if it was last run less than 2 minutes ago. Note that this isnt at least 2 minutes between popups. This is 2 minutes between checks for whether a popup should be displayed.
      3) MDL-30544 - you may need to reset your messaging preferences are turning off "temporarily disable notifications"

      Check that user B does not have notifications disabled. The checkbox is on the messaging preferences page. Check that they have popup enabled for both online and offline.

      As user A send user B a message. Check that the notification appears next time user B loads a course page.

      Now go to user B's messaging preferences and temporarily disable notifications using the temporary disable checkbox (dont alter their actual notification preferences). Send them another message and check that you are NOT notified.

      Show
      Testing this is mostly a case of checking that nothing was broken. You'll need 2 users. Some things to be aware of: 1) when we say popup in the context of message notifications we mean a small overlay that appears in the bottom right of the screen (assuming the them hasnt moved it). 2) For performance reasons the query to check whether there are pending popup notifications will not be run if it was last run less than 2 minutes ago. Note that this isnt at least 2 minutes between popups. This is 2 minutes between checks for whether a popup should be displayed. 3) MDL-30544 - you may need to reset your messaging preferences are turning off "temporarily disable notifications" Check that user B does not have notifications disabled. The checkbox is on the messaging preferences page. Check that they have popup enabled for both online and offline. As user A send user B a message. Check that the notification appears next time user B loads a course page. Now go to user B's messaging preferences and temporarily disable notifications using the temporary disable checkbox (dont alter their actual notification preferences). Send them another message and check that you are NOT notified.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      MDL-30260_disable_messages

      Description

      This is a regression from MDL-22232.

      I am looking at line 146 of lib/messagelib.php https://github.com/moodle/moodle/commit/a6f388c6640b14691601b852fe0b584425680c4f#L2R146

      1. This code assumes that whatever is calling message_send has set $eventdata->userto->emailstop, even though this has not been required by the API for all the time from Moodle 2.0 to 2.1.2. Therefore lots of places call this without that field being set - in particular in one of my third-partly plugins. I don't recall this API change being publicised anywhere.

      a. Thus, it should be mentioned in the release notes - I added http://docs.moodle.org/dev/Moodle_2.1.2_release_notes#Random_API_changes. Since 2.1.2 and people have already read the release notes an upgraded, we probably need to flag this where people will see too.

      b. If that field is not set, then we need a developer debug warning, and

      c. we should probably have an extra DB query to load the correct value from the DB, rather than behaving as if it were null.

      2. It fails to check emailstop in the 'forced' branch of the if statement, so the emailstop functionality does not eve work reliably, other than with default configuration.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

                People

                • Votes:
                  2 Vote for this issue
                  Watchers:
                  7 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    9/Jan/12