Moodle
  1. Moodle
  2. MDL-30260

Re-implementation of emailstop is dangerously broken

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical 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
    • Rank:
      27417

      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.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Adding watchers.

          Show
          Tim Hunt added a comment - Adding watchers.
          Hide
          Tim Hunt added a comment - - edited

          Oh, fantastic, add another:

          3. emailstop was not added to the list of default user profile fields in places like get_role_users() - thereby creatly increasing the chance that MDL-22232 broke other code.

          4. There is at least one other broken occurrence in core code, in quiz_send_notification_messages mod/quiz/locallib.php. I'll let you do your own grepping to find the rest.

          Show
          Tim Hunt added a comment - - edited Oh, fantastic, add another: 3. emailstop was not added to the list of default user profile fields in places like get_role_users() - thereby creatly increasing the chance that MDL-22232 broke other code. 4. There is at least one other broken occurrence in core code, in quiz_send_notification_messages mod/quiz/locallib.php. I'll let you do your own grepping to find the rest.
          Hide
          Andrew Davis added a comment -

          re #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."

          Currently a message setting being set to forced by the admin overrules emailstop being set by the user. This is because the user themselves can set emailstop and we can't allow them to overrule the admin. As the help item for emailstop says "Temporarily disable all notifications except those marked as "forced" by the site administrator". I can easily reverse this logic but I think its correct as it is. Or at least, I think allowing students to turn off notifications set to forced by the admin allows bad situations to occur.

          Show
          Andrew Davis added a comment - re #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." Currently a message setting being set to forced by the admin overrules emailstop being set by the user. This is because the user themselves can set emailstop and we can't allow them to overrule the admin. As the help item for emailstop says "Temporarily disable all notifications except those marked as "forced" by the site administrator". I can easily reverse this logic but I think its correct as it is. Or at least, I think allowing students to turn off notifications set to forced by the admin allows bad situations to occur.
          Hide
          Andrew Davis added a comment -

          Ive added git info and has started rectifying the problems Tim has pointed out.

          Show
          Andrew Davis added a comment - Ive added git info and has started rectifying the problems Tim has pointed out.
          Hide
          Andrew Davis added a comment -

          Ive now been through every call to message_send() and have checked that everywhere is supplying user.emailstop

          Show
          Andrew Davis added a comment - Ive now been through every call to message_send() and have checked that everywhere is supplying user.emailstop
          Hide
          Tim Hunt added a comment - - edited

          Thank you very much for jumping on this so quickly Andrew. You've made a good start, but there are still issues:

          A. Still no changes in any upgrade.txt file. How are developers supposed to find out about the API change?

          B. I would like someone more konwledgable (Eloy/Petr?) to answer the question https://github.com/andyjdavis/moodle/compare/master...MDL-30260_disable_messages#L0R22

          C. I would move

          if (!isset($eventdata->userto->emailstop)) {
              debugging('userto->emailstop is not set. Retreiving it from the user table');
              $eventdata->userto->emailstop = $DB->get_field('user', 'emailstop', array('id'=>$eventdata->userto->id));
          }
          

          outside the if statement. I think we should give the developers the warning about emailstop, even if they are developing on a site with all processors set to forced for some crazy reason. That would also reduce the size of the diff because indent would not change.

          D. Are you sure you reviewed all the ways all calls to message_send load the data about the users it is going to send to? In particular, you have not acted on my point 3. above. You need to find all the places we load lists of users with a default set of fields, and make sure emailstop is added.

          Show
          Tim Hunt added a comment - - edited Thank you very much for jumping on this so quickly Andrew. You've made a good start, but there are still issues: A. Still no changes in any upgrade.txt file. How are developers supposed to find out about the API change? B. I would like someone more konwledgable (Eloy/Petr?) to answer the question https://github.com/andyjdavis/moodle/compare/master...MDL-30260_disable_messages#L0R22 C. I would move if (!isset($eventdata->userto->emailstop)) { debugging('userto->emailstop is not set. Retreiving it from the user table'); $eventdata->userto->emailstop = $DB->get_field('user', 'emailstop', array('id'=>$eventdata->userto->id)); } outside the if statement. I think we should give the developers the warning about emailstop, even if they are developing on a site with all processors set to forced for some crazy reason. That would also reduce the size of the diff because indent would not change. D. Are you sure you reviewed all the ways all calls to message_send load the data about the users it is going to send to? In particular, you have not acted on my point 3. above. You need to find all the places we load lists of users with a default set of fields, and make sure emailstop is added.
          Hide
          Andrew Davis added a comment -

          For the moment at least Ive decided to observe the value of the admin's emailstop flag. I'll raise this with Eloy/Petr when I can get hold of them.

          I've added an upgrade.txt file in /message. None of the existing upgrade.txt file locations seemed suitable for this.

          Show
          Andrew Davis added a comment - For the moment at least Ive decided to observe the value of the admin's emailstop flag. I'll raise this with Eloy/Petr when I can get hold of them. I've added an upgrade.txt file in /message. None of the existing upgrade.txt file locations seemed suitable for this.
          Hide
          Andrew Davis added a comment - - edited

          I didn't actually find any places where users were loaded by get_role_users() then supplied to message_send(). All the same I have now added user.emailstop to the list of default fields in get_role_users() for the sake of anyone who does it in the future (or if I'm wrong).

          There were quite a few places where users were loaded by get_users_by_capability() or by get_admin()/get_admin() which use u.* in their queries so fewer changes were necessary than I was expecting.

          Show
          Andrew Davis added a comment - - edited I didn't actually find any places where users were loaded by get_role_users() then supplied to message_send(). All the same I have now added user.emailstop to the list of default fields in get_role_users() for the sake of anyone who does it in the future (or if I'm wrong). There were quite a few places where users were loaded by get_users_by_capability() or by get_admin()/get_admin() which use u.* in their queries so fewer changes were necessary than I was expecting.
          Hide
          Tim Hunt added a comment -

          I think lib/upgrade.txt is the right place. That should be where we tell people about changes in the core API. message/output/upgrade.txt is where you write information for people creating new message output plugins.

          My contrib plugin sends email to people obtained by get_role_users(), which is where this all started. I wonder if we should actually change get_role_users to use u.*, like the other similar places? That is slightly less efficient, but much more robust against future changes, and more consistent.

          Show
          Tim Hunt added a comment - I think lib/upgrade.txt is the right place. That should be where we tell people about changes in the core API. message/output/upgrade.txt is where you write information for people creating new message output plugins. My contrib plugin sends email to people obtained by get_role_users(), which is where this all started. I wonder if we should actually change get_role_users to use u.*, like the other similar places? That is slightly less efficient, but much more robust against future changes, and more consistent.
          Hide
          Tim Hunt added a comment -

          Ping?

          Show
          Tim Hunt added a comment - Ping?
          Hide
          Andrew Davis added a comment -

          Oops. Sorry Tim. Submitting it for integration now.

          I've left the u.whatever list of fields as is to keep the changes as small as possible and to avoid introducing any unnecessary inefficiencies.

          Show
          Andrew Davis added a comment - Oops. Sorry Tim. Submitting it for integration now. I've left the u.whatever list of fields as is to keep the changes as small as possible and to avoid introducing any unnecessary inefficiencies.
          Hide
          Tim Hunt added a comment -

          Two minor code review comments, if you care enough to amend:

          1. Eloy prefers comments to be written with a space after the //. So

          // emailstop could be hard coded "false" to ensure error reports are sent
          // but then admin's would have to alter their messaging preferences to temporarily stop them

          2. It is spelt Retrieving, not Retreiving.

          Other than that, looks good.

          Show
          Tim Hunt added a comment - Two minor code review comments, if you care enough to amend: 1. Eloy prefers comments to be written with a space after the //. So // emailstop could be hard coded "false" to ensure error reports are sent // but then admin's would have to alter their messaging preferences to temporarily stop them 2. It is spelt Retrieving, not Retreiving. Other than that, looks good.
          Hide
          Andrew Davis added a comment -

          I have fixed those 2 things Did it yesterday sometime.

          Show
          Andrew Davis added a comment - I have fixed those 2 things Did it yesterday sometime.
          Hide
          Aparup Banerjee added a comment -

          Thanks Andrew and Tim , patches have been integrated into 2.x stables and master.

          Show
          Aparup Banerjee added a comment - Thanks Andrew and Tim , patches have been integrated into 2.x stables and master.
          Hide
          Ankit Agarwal added a comment -

          Hi,
          I get the following error messages while testing this :-

          ( ! ) Notice: Undefined property: stdClass::$format in H:\wamp\www\repo2\master\moodle\lib\navigationlib.php on line 1555
          Call Stack
          #	Time	Memory	Function	Location
          1	0.0011	438912	{main}( )	..\edit.php:0
          2	0.2993	17618816	global_navigation->find( )	..\edit.php:78
          3	0.2993	17618816	global_navigation->initialise( )	..\navigationlib.php:2422
          4	0.3703	22540192	global_navigation->load_course_sections( )	..\navigationlib.php:1239
          
          ( ! ) Notice: Undefined property: stdClass::$format in H:\wamp\www\repo2\master\moodle\lib\navigationlib.php on line 1556
          Call Stack
          #	Time	Memory	Function	Location
          1	0.0011	438912	{main}( )	..\edit.php:0
          2	0.2993	17618816	global_navigation->find( )	..\edit.php:78
          3	0.2993	17618816	global_navigation->initialise( )	..\navigationlib.php:2422
          4	0.3703	22540192	global_navigation->load_course_sections( )	..\navigationlib.php:1239
          Coding problem - missing course modinfo property in get_fast_modinfo() call
          line 1098 of \lib\modinfolib.php: call to debugging()
          line 1584 of \lib\navigationlib.php: call to get_fast_modinfo()
          line 1645 of \lib\navigationlib.php: call to global_navigation->generate_sections_and_activities()
          line 1567 of \lib\navigationlib.php: call to global_navigation->load_generic_course_sections()
          line 1239 of \lib\navigationlib.php: call to global_navigation->load_course_sections()
          line 2422 of \lib\navigationlib.php: call to global_navigation->initialise()
          line 78 of \message\edit.php: call to global_navigation->find()
          
          ( ! ) Notice: Undefined property: stdClass::$numsections in H:\wamp\www\repo2\master\moodle\lib\navigationlib.php on line 1585
          Call Stack
          #	Time	Memory	Function	Location
          1	0.0011	438912	{main}( )	..\edit.php:0
          2	0.2993	17618816	global_navigation->find( )	..\edit.php:78
          3	0.2993	17618816	global_navigation->initialise( )	..\navigationlib.php:2422
          4	0.3703	22540192	global_navigation->load_course_sections( )	..\navigationlib.php:1239
          5	0.3709	22540824	global_navigation->load_generic_course_sections( )	..\navigationlib.php:1567
          6	0.3709	22540872	global_navigation->generate_sections_and_activities( )	..\navigationlib.php:1645
          

          Steps to reproduce:-

          1. Login as a user and Goto a course
          2. From the navigation menu select >my profile settings>messaging, you should be able to see these error msgs
          3. Note my profile settings>messaging works fine when accessed from front page.

          I also tried to exchange msg between users multiple times (with the settings checked) but never got an popup, although the messages were delivered fine.

          failing this sorry
          Thanks

          Show
          Ankit Agarwal added a comment - Hi, I get the following error messages while testing this :- ( ! ) Notice: Undefined property: stdClass::$format in H:\wamp\www\repo2\master\moodle\lib\navigationlib.php on line 1555 Call Stack # Time Memory Function Location 1 0.0011 438912 {main}( ) ..\edit.php:0 2 0.2993 17618816 global_navigation->find( ) ..\edit.php:78 3 0.2993 17618816 global_navigation->initialise( ) ..\navigationlib.php:2422 4 0.3703 22540192 global_navigation->load_course_sections( ) ..\navigationlib.php:1239 ( ! ) Notice: Undefined property: stdClass::$format in H:\wamp\www\repo2\master\moodle\lib\navigationlib.php on line 1556 Call Stack # Time Memory Function Location 1 0.0011 438912 {main}( ) ..\edit.php:0 2 0.2993 17618816 global_navigation->find( ) ..\edit.php:78 3 0.2993 17618816 global_navigation->initialise( ) ..\navigationlib.php:2422 4 0.3703 22540192 global_navigation->load_course_sections( ) ..\navigationlib.php:1239 Coding problem - missing course modinfo property in get_fast_modinfo() call line 1098 of \lib\modinfolib.php: call to debugging() line 1584 of \lib\navigationlib.php: call to get_fast_modinfo() line 1645 of \lib\navigationlib.php: call to global_navigation->generate_sections_and_activities() line 1567 of \lib\navigationlib.php: call to global_navigation->load_generic_course_sections() line 1239 of \lib\navigationlib.php: call to global_navigation->load_course_sections() line 2422 of \lib\navigationlib.php: call to global_navigation->initialise() line 78 of \message\edit.php: call to global_navigation->find() ( ! ) Notice: Undefined property: stdClass::$numsections in H:\wamp\www\repo2\master\moodle\lib\navigationlib.php on line 1585 Call Stack # Time Memory Function Location 1 0.0011 438912 {main}( ) ..\edit.php:0 2 0.2993 17618816 global_navigation->find( ) ..\edit.php:78 3 0.2993 17618816 global_navigation->initialise( ) ..\navigationlib.php:2422 4 0.3703 22540192 global_navigation->load_course_sections( ) ..\navigationlib.php:1239 5 0.3709 22540824 global_navigation->load_generic_course_sections( ) ..\navigationlib.php:1567 6 0.3709 22540872 global_navigation->generate_sections_and_activities( ) ..\navigationlib.php:1645 Steps to reproduce:- Login as a user and Goto a course From the navigation menu select >my profile settings>messaging, you should be able to see these error msgs Note my profile settings>messaging works fine when accessed from front page. I also tried to exchange msg between users multiple times (with the settings checked) but never got an popup, although the messages were delivered fine. failing this sorry Thanks
          Hide
          Andrew Davis added a comment -

          The navigation lib problem is unrelated to this issue. Ive raised MDL-30543 to have it looked at.

          Investigating the lack of popups now.

          Show
          Andrew Davis added a comment - The navigation lib problem is unrelated to this issue. Ive raised MDL-30543 to have it looked at. Investigating the lack of popups now.
          Hide
          Aparup Banerjee added a comment -

          noting that i'm getting the pop ups (after refreshing the page)

          Show
          Aparup Banerjee added a comment - noting that i'm getting the pop ups (after refreshing the page)
          Hide
          Aparup Banerjee added a comment -

          reseting for testing again. test instructions were improved.

          Show
          Aparup Banerjee added a comment - reseting for testing again. test instructions were improved.
          Hide
          Andrew Davis added a comment - - edited

          Ive amended the testing instructions as there is an unrelated bug getting in the way of testing quiz notifications.

          update: have raised MDL-30545 to check on whether quiz notifications are appearing for who they should be.

          Show
          Andrew Davis added a comment - - edited Ive amended the testing instructions as there is an unrelated bug getting in the way of testing quiz notifications. update: have raised MDL-30545 to check on whether quiz notifications are appearing for who they should be.
          Hide
          Ankit Agarwal added a comment -

          Everything working fine
          passing
          Thanks

          Show
          Ankit Agarwal added a comment - Everything working fine passing Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sent upstream! Just in time for Moodle 2.2rc1 (if related), yay!

          Closing and big thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Sent upstream! Just in time for Moodle 2.2rc1 (if related), yay! Closing and big thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: