Moodle
  1. Moodle
  2. MDL-27171

Improved configuration options for the messaging system

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.2
    • Fix Version/s: 2.1
    • Component/s: Messages
    • Labels:
    • Testing Instructions:
      Hide

      Test with the OU script:
      Admin Set up a test server using the new code.
      Admin Create a test users Student1 and Student2.
      Admin Go to the Manage message outputs screen, and check that the three standard messaging plugins are shown. Jabber should be disabled, because it is not configured.
      Admin Configure the Jabber plugin, and check that it can then be enabled.
      Admin Go to the Default message outputs page. Check that every combination is set to Permitted; that email is the default for everything, logged in and logged out, except for instant messages which default to email and popup when online.
      Student1 Log in as Student1, go to the message settings in their profile. Check that they see these defaults. Click save changes.
      Admin Back as admin, on the Default message outputs
      for Instant messages, Forbid email, and force popup.
      for forum notifications, change the default when online from email to popup.
      Student1 Back as student 1, on their message settings page. Check
      that the forced settings for instant messaging have taken effect.
      that the changes to the default settings have not affected the defaults that Student1 saved at step 6.
      Student2 Log in as student 2, and check that they do see the updated defaults on their message settings page.
      Student2 Send student1 an instant message.
      Student2 Subscribe to a forum.
      Studnet1 Log in. Check that the instant message pop-up appears.
      Student1 Go to the forum and make a post.
      Student2 Check that the forum email arrives.

      In addition to this script, go to the various messaging screens, the user message settings, see the new message screens in admin -> plugins -> messaging

      Show
      Test with the OU script: Admin Set up a test server using the new code. Admin Create a test users Student1 and Student2. Admin Go to the Manage message outputs screen, and check that the three standard messaging plugins are shown. Jabber should be disabled, because it is not configured. Admin Configure the Jabber plugin, and check that it can then be enabled. Admin Go to the Default message outputs page. Check that every combination is set to Permitted; that email is the default for everything, logged in and logged out, except for instant messages which default to email and popup when online. Student1 Log in as Student1, go to the message settings in their profile. Check that they see these defaults. Click save changes. Admin Back as admin, on the Default message outputs for Instant messages, Forbid email, and force popup. for forum notifications, change the default when online from email to popup. Student1 Back as student 1, on their message settings page. Check that the forced settings for instant messaging have taken effect. that the changes to the default settings have not affected the defaults that Student1 saved at step 6. Student2 Log in as student 2, and check that they do see the updated defaults on their message settings page. Student2 Send student1 an instant message. Student2 Subscribe to a forum. Studnet1 Log in. Check that the instant message pop-up appears. Student1 Go to the forum and make a post. Student2 Check that the forum email arrives. In addition to this script, go to the various messaging screens, the user message settings, see the new message screens in admin -> plugins -> messaging
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      ou-messaging-release
    • Rank:
      16943

      Description

      See http://moodle.org/mod/forum/discuss.php?d=172825

      These are enhancements that the OU hope to develop and get added to Moodle core.

      1. Messaging defaults.bmml
        7 kB
        Tim Hunt
      2. Messaging defaults.bmml
        7 kB
        Tim Hunt
      1. Messaging defaults.png
        38 kB

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Added UI Mockup: <Messaging defaults>

          Show
          Tim Hunt added a comment - Added UI Mockup: <Messaging defaults>
          Hide
          Charles Fulton added a comment -

          I'm glad to see this as a tracker item; the group (CLAMP) I work with was developing something similar that's tied in the user upload, but your mockup looks more feature-complete than what I hacked together (which was really just a variation on the user messaging page).

          Show
          Charles Fulton added a comment - I'm glad to see this as a tracker item; the group (CLAMP) I work with was developing something similar that's tied in the user upload, but your mockup looks more feature-complete than what I hacked together (which was really just a variation on the user messaging page).
          Hide
          Tim Hunt added a comment -

          Thanks for your support Charles. I have now posted a detailed specification in the forum thread http://moodle.org/mod/forum/discuss.php?d=172825. If you have the time to review it, I would be most grateful.

          Show
          Tim Hunt added a comment - Thanks for your support Charles. I have now posted a detailed specification in the forum thread http://moodle.org/mod/forum/discuss.php?d=172825 . If you have the time to review it, I would be most grateful.
          Hide
          Dan Poltawski added a comment -

          Assignign to Ruslan who is working on this

          Show
          Dan Poltawski added a comment - Assignign to Ruslan who is working on this
          Hide
          Dan Poltawski added a comment - - edited

          Where is gerrit when we need it. Some review comments from me:

          1) Currently using functions in upgrade, this cannot be permitted.
          2) This feels like it shouldn't be necessary:

          commit 330b6cb59aa7f85002f3413bc7cbb7062e806a34
          Author: Ruslan Kabalin <ruslan.kabalin@luns.net.uk>
          Date: Tue May 24 10:01:56 2011 +0100
          
          core: change the order of plugins returned by get_plugin_types function
          
          We need to ensure that message/output is installed first so that default
          messaging settings are set correctly afterwards.
          

          3) Linebreaks are missing from phpdoc between description and param definition
          e.g.:

          /**
           * Get all message processors, validate corresponding plugin existance and
           * system configuration
           * @param bool $ready only return ready-to-use processors
           * @return mixed $processors array of objects containing information on message processors
           */
          

          Should be:

          /**
           * Get all message processors, validate corresponding plugin existance and
           * system configuration
           *
           * @param bool $ready only return ready-to-use processors
           * @return mixed $processors array of objects containing information on message processors
           */
          

          4)the ternary operator is used, I don't think this is permitted in Moodle coding guidelines, although I can't find a mention

          Show
          Dan Poltawski added a comment - - edited Where is gerrit when we need it. Some review comments from me: 1) Currently using functions in upgrade, this cannot be permitted. 2) This feels like it shouldn't be necessary: commit 330b6cb59aa7f85002f3413bc7cbb7062e806a34 Author: Ruslan Kabalin <ruslan.kabalin@luns.net.uk> Date: Tue May 24 10:01:56 2011 +0100 core: change the order of plugins returned by get_plugin_types function We need to ensure that message/output is installed first so that default messaging settings are set correctly afterwards. 3) Linebreaks are missing from phpdoc between description and param definition e.g.: /** * Get all message processors, validate corresponding plugin existance and * system configuration * @param bool $ready only return ready-to-use processors * @ return mixed $processors array of objects containing information on message processors */ Should be: /** * Get all message processors, validate corresponding plugin existance and * system configuration * * @param bool $ready only return ready-to-use processors * @ return mixed $processors array of objects containing information on message processors */ 4)the ternary operator is used, I don't think this is permitted in Moodle coding guidelines, although I can't find a mention
          Hide
          Dan Poltawski added a comment -

          5) I think this should be using moodle_url:

          "$CFG->wwwroot/$CFG->admin/message.php"
          
          Show
          Dan Poltawski added a comment - 5) I think this should be using moodle_url: "$CFG->wwwroot/$CFG->admin/message.php"
          Hide
          Tim Hunt added a comment -

          I expect you already know, but new moodle_url('/admin/index.php'); automagically does the $CFG->admin thing.

          Show
          Tim Hunt added a comment - I expect you already know, but new moodle_url('/admin/index.php'); automagically does the $CFG->admin thing.
          Hide
          Dan Poltawski added a comment -

          6) There are some AMOS operations which need to happen we need to put itno commit message

          Show
          Dan Poltawski added a comment - 6) There are some AMOS operations which need to happen we need to put itno commit message
          Hide
          Dan Poltawski added a comment -

          Tested based on OU specificiaton.

          Show
          Dan Poltawski added a comment - Tested based on OU specificiaton.
          Hide
          Ruslan Kabalin added a comment -
          Show
          Ruslan Kabalin added a comment - The documentation is here: http://docs.moodle.org/en/Development:Messaging_2.0_improvements
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          This change is looking AWESOME! it really improves the control that admin/users have over messaging.

          I've been looking at this all afternoon; I am only vaguely familiar with messaging so have been learning that as I go.
          Presently this review is NOT complete I need to finish reviewing the code and then look at the outcome - particually in regards to backwards compatability and upgrades.
          The following are the notes I've made so far as I have been studying this work, by all means if you think I have something wrong let me know, otherwise I'll be keen to hear what you think.

          Specific notes:

          lib/messagelib.php

          • line 124: $defaultpreferences is an object not an array - use isset instead. This happens frequently with this function better review all uses
          • line 129: coding_exceptions should not use language strings - if a meaningful string is required this certainly shouldn't be a coding exception rather a moodle_exception.
          • function message_update_providers is there any reason this function no longer returns true? it probably still should to ensure backwards compatability unless there is a reason it shouldn't.
          • message_update_providers We need to check wether the defaults have changed for the provider and update if required.
          • message_update_providers should clean up its related records when deleting message_providers.

          message/lib.php

          • get_message_processors the static handling of this function is not nice at all, lets assume it is called twice by a script, first $ready is true so processors static is populated with all processors that are enabled and configured, the next call leaves ready = false however $processors has already been filtered to enabled + configured and that is what is returned rather than all processors. I think that the create_function should also be changed to an iteration solution at the same time (see general note 4).
          • get_message_output_default_preferences $preferences = (object) array(); should be $preferences = new stdClass;
          • In regards to the translate_message_default_setting function:
            1. I dislike the hardcoding of email defaults here. Am I right that in noting they are used when the processor default ($plugindefault) is invalid for one reason or another? by the looks of it this should never happen as those defaults are forced into the DB right? anyway not sure if there is a better solution - just noting it for the time being.
            2. Do we care that the $plugindefault can be greater than 4 bit - because bitwise is being used this makes no difference.
            3. translate_message_default_setting notify is a deprecated function, if we end up here we should either throw an exception or use the default.
            4. $loggedin = $loggedoff = 0x00; These should default to null not 0
            5. 2353 + 2354: ~MESSAGE_PERMITTED_MASK is not needed $plugindefault & MESSAGE_DEFAULT_LOGGEDIN is enough and is clearer to read.

          lib/adminlib.php

          • Include on line 4943 isn't needed (that file includes lib/messagelib.php which includes message/lib.php).

          General notes:

          1/ Variable names should not contain _'s as per coding guidelines (first noted in messagelib.php $is_user_configured) http://docs.moodle.org/en/Development:Coding_style

          2/ I don't like the way in which preferences need to be established within the database, the outcome is such that there are checks where coding_exceptions are thrown.

          3/ Why is it that message/output now needs to be the first plugin type installed? Is this by chance this tied to the above idea whereby message defaults need to be established. I am REALLY quite unsure about that - we should avoid any chicken-egg problems if possible and this looks like one is being introduced.

          4/ In regards to the lambda functions that are being used we would prefer that they were expanded out, in the case of array_filter its easy with an iteration, with array_sort style functions we'd prefer a new function was created. The reason for this is simply that it clearer what is going on and the code is more self documenting. I've talked to Martin about this - we don't have a hard policy on it is only a preference.

          5/ After config.php has been included additional require_once calls should use $CFG->dirroot (or $CFG->admindir/$CFG->libdir)

          Other notes - to be turned into bugs later:

          • Missing string for in mod_feedback, messageprovider:message.
          • Messaging could REALLY use some unit tests.

          After talking to Martin about this issue could you please also confirm that at least the following test situations have been run:

          • Fresh install of Moodle 2.0+
          • Upgrading a Moodle 1.9 site
          • Upgrading a Moodle 2.0 site without custom message providers
          • Upgrading a Moodle 2.0 site with custom message providers
          • Altering a message providers defaults after upgrade
          • All core message providers have been tested and work
          • All core message providers have been disabled and things still work as expected.

          I've also asked Eloy to have a look at this if he finds time however his priority is the impending quiz changes.
          Either way tomorrow I will continue on with this review and then we'll make a decision about whether it goes in this week and we fix any remaining issues or whether it has to go back for another round.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, This change is looking AWESOME! it really improves the control that admin/users have over messaging. I've been looking at this all afternoon; I am only vaguely familiar with messaging so have been learning that as I go. Presently this review is NOT complete I need to finish reviewing the code and then look at the outcome - particually in regards to backwards compatability and upgrades. The following are the notes I've made so far as I have been studying this work, by all means if you think I have something wrong let me know, otherwise I'll be keen to hear what you think. Specific notes: lib/messagelib.php line 124: $defaultpreferences is an object not an array - use isset instead. This happens frequently with this function better review all uses line 129: coding_exceptions should not use language strings - if a meaningful string is required this certainly shouldn't be a coding exception rather a moodle_exception. function message_update_providers is there any reason this function no longer returns true? it probably still should to ensure backwards compatability unless there is a reason it shouldn't. message_update_providers We need to check wether the defaults have changed for the provider and update if required. message_update_providers should clean up its related records when deleting message_providers. message/lib.php get_message_processors the static handling of this function is not nice at all, lets assume it is called twice by a script, first $ready is true so processors static is populated with all processors that are enabled and configured, the next call leaves ready = false however $processors has already been filtered to enabled + configured and that is what is returned rather than all processors. I think that the create_function should also be changed to an iteration solution at the same time (see general note 4). get_message_output_default_preferences $preferences = (object) array(); should be $preferences = new stdClass; In regards to the translate_message_default_setting function: I dislike the hardcoding of email defaults here. Am I right that in noting they are used when the processor default ($plugindefault) is invalid for one reason or another? by the looks of it this should never happen as those defaults are forced into the DB right? anyway not sure if there is a better solution - just noting it for the time being. Do we care that the $plugindefault can be greater than 4 bit - because bitwise is being used this makes no difference. translate_message_default_setting notify is a deprecated function, if we end up here we should either throw an exception or use the default. $loggedin = $loggedoff = 0x00; These should default to null not 0 2353 + 2354: ~MESSAGE_PERMITTED_MASK is not needed $plugindefault & MESSAGE_DEFAULT_LOGGEDIN is enough and is clearer to read. lib/adminlib.php Include on line 4943 isn't needed (that file includes lib/messagelib.php which includes message/lib.php). General notes: 1/ Variable names should not contain _'s as per coding guidelines (first noted in messagelib.php $is_user_configured) http://docs.moodle.org/en/Development:Coding_style 2/ I don't like the way in which preferences need to be established within the database, the outcome is such that there are checks where coding_exceptions are thrown. 3/ Why is it that message/output now needs to be the first plugin type installed? Is this by chance this tied to the above idea whereby message defaults need to be established. I am REALLY quite unsure about that - we should avoid any chicken-egg problems if possible and this looks like one is being introduced. 4/ In regards to the lambda functions that are being used we would prefer that they were expanded out, in the case of array_filter its easy with an iteration, with array_sort style functions we'd prefer a new function was created. The reason for this is simply that it clearer what is going on and the code is more self documenting. I've talked to Martin about this - we don't have a hard policy on it is only a preference. 5/ After config.php has been included additional require_once calls should use $CFG->dirroot (or $CFG->admindir/$CFG->libdir) Other notes - to be turned into bugs later: Missing string for in mod_feedback, messageprovider:message. Messaging could REALLY use some unit tests. After talking to Martin about this issue could you please also confirm that at least the following test situations have been run: Fresh install of Moodle 2.0+ Upgrading a Moodle 1.9 site Upgrading a Moodle 2.0 site without custom message providers Upgrading a Moodle 2.0 site with custom message providers Altering a message providers defaults after upgrade All core message providers have been tested and work All core message providers have been disabled and things still work as expected. I've also asked Eloy to have a look at this if he finds time however his priority is the impending quiz changes. Either way tomorrow I will continue on with this review and then we'll make a decision about whether it goes in this week and we fix any remaining issues or whether it has to go back for another round. Cheers Sam
          Hide
          Dan Poltawski added a comment -

          Hi Sam,

          Thanks for taking the time to do this detailed review. I agree with your comments they seem sensible. Its a public holiday here today so I thought i'd respond to parts I know about as Ruslan probably wont get a chance:

          line 129: coding_exceptions should not use language strings - if a meaningful string is required this certainly shouldn't be a coding exception rather a moodle_exception.

          This is existing behaviour from master, should we remove the string?

          function message_update_providers is there any reason this function no longer returns true? it probably still should to ensure backwards compatability unless there is a reason it shouldn't.

          This is a mistake. I requested we removed some needless 'return true' statements from new functions which were mistakenly added and this one got swept up with them (Ruslan had thought it was Moodle policy to return true even if it was meaningless due to other functions in the file).

          1/ Variable names should not contain _'s as per coding guidelines (first noted in messagelib.php $is_user_configured) http://docs.moodle.org/en/Development:Coding_style

          Wow, that is a testiment to the thoroughness of your review, as I think that is the only violation of that policy

          After talking to Martin about this issue could you please also confirm that at least the following test situations have been run:

          Working on this.

          cheers,
          dan

          Show
          Dan Poltawski added a comment - Hi Sam, Thanks for taking the time to do this detailed review. I agree with your comments they seem sensible. Its a public holiday here today so I thought i'd respond to parts I know about as Ruslan probably wont get a chance: line 129: coding_exceptions should not use language strings - if a meaningful string is required this certainly shouldn't be a coding exception rather a moodle_exception. This is existing behaviour from master, should we remove the string? function message_update_providers is there any reason this function no longer returns true? it probably still should to ensure backwards compatability unless there is a reason it shouldn't. This is a mistake. I requested we removed some needless 'return true' statements from new functions which were mistakenly added and this one got swept up with them (Ruslan had thought it was Moodle policy to return true even if it was meaningless due to other functions in the file). 1/ Variable names should not contain _'s as per coding guidelines (first noted in messagelib.php $is_user_configured) http://docs.moodle.org/en/Development:Coding_style Wow, that is a testiment to the thoroughness of your review, as I think that is the only violation of that policy After talking to Martin about this issue could you please also confirm that at least the following test situations have been run: Working on this. cheers, dan
          Hide
          Ruslan Kabalin added a comment - - edited

          Hello Sam,

          Thanks a lot for thorough review. I will go through all of them in details tomorrow. Great job, you have spotted many important things!

          et_message_processors the static handling of this function is not nice at all, ... the next call leaves ready = false

          That is exactly what I tried to avoid, only full processors list should be stored in static variable, thanks for pointing that out.

          Why is it that message/output now needs to be the first plugin type installed? Is this by chance this tied to the above idea whereby message defaults need to be established. I am REALLY quite unsure about that - we should avoid any chicken-egg problems if possible and this looks like one is being introduced.

          Yes, we need to have messaging processors installed first to process message defaults of any other plugin that have them. I thought it would be more effective rather than adding another iteration through the list of plugins and checking what each of them has in messages.php again after installation.

          Regarding testing, what we have done so far:

          Fresh install of Moodle 2.0+

          yes

          Upgrading a Moodle 1.9 site

          no

          Upgrading a Moodle 2.0 site without custom message providers

          no (do you mean the site that has no providers registered at all?)

          Upgrading a Moodle 2.0 site with custom message providers

          yes

          Altering a message providers defaults after upgrade

          Partly yes, good point actually, we have to test the case when some providers are removed and ensure corresponding settings are removed from DB (I have tested the full plugin deinstallation so far which does the job correctly). If defaults are changed (in db/messages.php) for existing providers we do not have to update anything, otherwise Admin's preferences will be flushed to db/messages.php defaults on every upgrade.

          All core message providers have been tested and work

          yes

          All core message providers have been disabled and things still work as expected.

          no

          Best,
          Ruslan

          Show
          Ruslan Kabalin added a comment - - edited Hello Sam, Thanks a lot for thorough review. I will go through all of them in details tomorrow. Great job, you have spotted many important things! et_message_processors the static handling of this function is not nice at all, ... the next call leaves ready = false That is exactly what I tried to avoid, only full processors list should be stored in static variable, thanks for pointing that out. Why is it that message/output now needs to be the first plugin type installed? Is this by chance this tied to the above idea whereby message defaults need to be established. I am REALLY quite unsure about that - we should avoid any chicken-egg problems if possible and this looks like one is being introduced. Yes, we need to have messaging processors installed first to process message defaults of any other plugin that have them. I thought it would be more effective rather than adding another iteration through the list of plugins and checking what each of them has in messages.php again after installation. Regarding testing, what we have done so far: Fresh install of Moodle 2.0+ yes Upgrading a Moodle 1.9 site no Upgrading a Moodle 2.0 site without custom message providers no (do you mean the site that has no providers registered at all?) Upgrading a Moodle 2.0 site with custom message providers yes Altering a message providers defaults after upgrade Partly yes, good point actually, we have to test the case when some providers are removed and ensure corresponding settings are removed from DB (I have tested the full plugin deinstallation so far which does the job correctly). If defaults are changed (in db/messages.php) for existing providers we do not have to update anything, otherwise Admin's preferences will be flushed to db/messages.php defaults on every upgrade. All core message providers have been tested and work yes All core message providers have been disabled and things still work as expected. no Best, Ruslan
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1 to wait a few days to get all this stuff fixed in order to land upstream in a better way.

          Note it's 2.1dev and we can integrate it out of the (strict) weekly if necessary and available sooner.

          Also, I think I'd squash all those commits (unless keeping that history is critical for somebody).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - +1 to wait a few days to get all this stuff fixed in order to land upstream in a better way. Note it's 2.1dev and we can integrate it out of the (strict) weekly if necessary and available sooner. Also, I think I'd squash all those commits (unless keeping that history is critical for somebody). Ciao
          Hide
          Sam Hemelryk added a comment -

          Hi guys, thanks for looking over those.
          Going back through them:

          • coding_exception - Given it was there originally it should probably stay and I'll create an issue to look at it later on and decide whether it should be removed or turned into a proper error.
          • underscores in variable names - haha yes I think it was the first things I spotted, so I just noted and carried on without looking for more. At least one is easy to fix.
          • message/output being first - Ok, I think in this case I think we should probably go for the less efficient option and process defaults after wards. The reason this came was in fact because a patch that Tim is working on involves putting the qtypes in first position as well which got us thinking about how we could best manage the ordering of things. As such there will be a new issue being created after the release of 2.1 to see if we can come up with a happy solution for this. For the time being I think processing defaults after the upgrade and then adding a comment to the effect that if message/output were processed first this could be avoided.
          • Thanks for the effort testing.

          I'm going to continue reviewing this now and let you know if I spot anything more.

          Cheers
          Sam.

          Show
          Sam Hemelryk added a comment - Hi guys, thanks for looking over those. Going back through them: coding_exception - Given it was there originally it should probably stay and I'll create an issue to look at it later on and decide whether it should be removed or turned into a proper error. underscores in variable names - haha yes I think it was the first things I spotted, so I just noted and carried on without looking for more. At least one is easy to fix. message/output being first - Ok, I think in this case I think we should probably go for the less efficient option and process defaults after wards. The reason this came was in fact because a patch that Tim is working on involves putting the qtypes in first position as well which got us thinking about how we could best manage the ordering of things. As such there will be a new issue being created after the release of 2.1 to see if we can come up with a happy solution for this. For the time being I think processing defaults after the upgrade and then adding a comment to the effect that if message/output were processed first this could be avoided. Thanks for the effort testing. I'm going to continue reviewing this now and let you know if I spot anything more. Cheers Sam.
          Hide
          Sam Hemelryk added a comment -

          Cool I've been having more of a play with this and didn't find anything more

          Show
          Sam Hemelryk added a comment - Cool I've been having more of a play with this and didn't find anything more
          Hide
          Ruslan Kabalin added a comment -

          Progress update:

          lib/messagelib.php

          line 124: $defaultpreferences is an object not an array - use isset instead. This happens frequently with this function better review all uses

          fixed in e8fc7940a8.

          line 129: coding_exceptions should not use language strings - if a meaningful string is required this certainly shouldn't be a coding exception rather a moodle_exception.

          as stated in another Sam's comment it will be addressed in the separate bug.

          function message_update_providers is there any reason this function no longer returns true? it probably still should to ensure backwards compatability unless there is a reason it shouldn't.

          fixed in 31afb0a437.

          message_update_providers We need to check whether the defaults have changed for the provider and update if required.

          This is not required, we insert defaults only on new message provider creation, for existing ones we have to preserve existing settings assuming that this is the admin's preference.

          message_update_providers should clean up its related records when deleting message_providers.

          Fixed in 298925d4. Notice that when the whole plugin is being removed, the clean up is done by message_uninstall function instead.

          message/lib.php

          get_message_processors the static handling of this function is not nice at all, lets assume it is called twice by a script, first $ready is true so processors static is populated with all processors that are enabled and configured, the next call leaves ready = false however $processors has already been filtered to enabled + configured and that is what is returned rather than all processors. I think that the create_function should also be changed to an iteration solution at the same time (see general note 4).

          fixed in 72e6af03, also refactored lambda function to make it easier to read.

          get_message_output_default_preferences $preferences = (object) array(); should be $preferences = new stdClass;

          fixed in e8fc7940a8.

          In regards to the translate_message_default_setting function:

          I dislike the hardcoding of email defaults here. Am I right that in noting they are used when the processor default ($plugindefault) is invalid for one reason or another? by the looks of it this should never happen as those defaults are forced into the DB right? anyway not sure if there is a better solution - just noting it for the time being.

          These email defaults are used primarily when defaults are not defined by provider (in db/messages.php) of if defaults are invalid.

          Do we care that the $plugindefault can be greater than 4 bit - because bitwise is being used this makes no difference.

          I do not have a strong opinion about it, it is just to minimise the case when suggested default is a senseless combination of constants.

          translate_message_default_setting notify is a deprecated function, if we end up here we should either throw an exception or use the default.

          fixed in bf0fb77

          $loggedin = $loggedoff = 0x00; These should default to null not 0

          2353 + 2354: ~MESSAGE_PERMITTED_MASK is not needed $plugindefault & MESSAGE_DEFAULT_LOGGEDIN is enough and is clearer to read.

          fixed in 838075147b

          lib/adminlib.php

          Include on line 4943 isn't needed (that file includes lib/messagelib.php which includes message/lib.php).

          fixed in a4de1d8f0

          Show
          Ruslan Kabalin added a comment - Progress update: lib/messagelib.php line 124: $defaultpreferences is an object not an array - use isset instead. This happens frequently with this function better review all uses fixed in e8fc7940a8. line 129: coding_exceptions should not use language strings - if a meaningful string is required this certainly shouldn't be a coding exception rather a moodle_exception. as stated in another Sam's comment it will be addressed in the separate bug. function message_update_providers is there any reason this function no longer returns true? it probably still should to ensure backwards compatability unless there is a reason it shouldn't. fixed in 31afb0a437. message_update_providers We need to check whether the defaults have changed for the provider and update if required. This is not required, we insert defaults only on new message provider creation, for existing ones we have to preserve existing settings assuming that this is the admin's preference. message_update_providers should clean up its related records when deleting message_providers. Fixed in 298925d4. Notice that when the whole plugin is being removed, the clean up is done by message_uninstall function instead. message/lib.php get_message_processors the static handling of this function is not nice at all, lets assume it is called twice by a script, first $ready is true so processors static is populated with all processors that are enabled and configured, the next call leaves ready = false however $processors has already been filtered to enabled + configured and that is what is returned rather than all processors. I think that the create_function should also be changed to an iteration solution at the same time (see general note 4). fixed in 72e6af03, also refactored lambda function to make it easier to read. get_message_output_default_preferences $preferences = (object) array(); should be $preferences = new stdClass; fixed in e8fc7940a8. In regards to the translate_message_default_setting function: I dislike the hardcoding of email defaults here. Am I right that in noting they are used when the processor default ($plugindefault) is invalid for one reason or another? by the looks of it this should never happen as those defaults are forced into the DB right? anyway not sure if there is a better solution - just noting it for the time being. These email defaults are used primarily when defaults are not defined by provider (in db/messages.php) of if defaults are invalid. Do we care that the $plugindefault can be greater than 4 bit - because bitwise is being used this makes no difference. I do not have a strong opinion about it, it is just to minimise the case when suggested default is a senseless combination of constants. translate_message_default_setting notify is a deprecated function, if we end up here we should either throw an exception or use the default. fixed in bf0fb77 $loggedin = $loggedoff = 0x00; These should default to null not 0 2353 + 2354: ~MESSAGE_PERMITTED_MASK is not needed $plugindefault & MESSAGE_DEFAULT_LOGGEDIN is enough and is clearer to read. fixed in 838075147b lib/adminlib.php Include on line 4943 isn't needed (that file includes lib/messagelib.php which includes message/lib.php). fixed in a4de1d8f0
          Hide
          Ruslan Kabalin added a comment -

          General notes:

          1/ Variable names should not contain _'s as per coding guidelines (first noted in messagelib.php $is_user_configured) http://docs.moodle.org/en/Development:Coding_style

          fixed in b46ca3d7

          2/ I don't like the way in which preferences need to be established within the database, the outcome is such that there are checks where coding_exceptions are thrown.

          Sam, can you please explain what you mean? At the point where coding_exception() is used in send_message() I assume that defaults are populated, if not then probably there was a problem during provider installation which I report in exception.

          3/ Why is it that message/output now needs to be the first plugin type installed? Is this by chance this tied to the above idea whereby message defaults need to be established. I am REALLY quite unsure about that - we should avoid any chicken-egg problems if possible and this looks like one is being introduced.

          message/output being first - Ok, I think in this case I think we should probably go for the less efficient option and process defaults after wards. The reason this came was in fact because a patch that Tim is working on involves putting the qtypes in first position as well which got us thinking about how we could best manage the ordering of things. As such there will be a new issue being created after the release of 2.1 to see if we can come up with a happy solution for this. For the time being I think processing defaults after the upgrade and then adding a comment to the effect that if message/output were processed first this could be avoided.

          fixed in 67147c255

          4/ In regards to the lambda functions that are being used we would prefer that they were expanded out, in the case of array_filter its easy with an iteration, with array_sort style functions we'd prefer a new function was created. The reason for this is simply that it clearer what is going on and the code is more self documenting. I've talked to Martin about this - we don't have a hard policy on it is only a preference.

          Addressed in 72e6af03

          5/ After config.php has been included additional require_once calls should use $CFG->dirroot (or $CFG->admindir/$CFG->libdir)

          fixed in 60bbe7684

          Show
          Ruslan Kabalin added a comment - General notes: 1/ Variable names should not contain _'s as per coding guidelines (first noted in messagelib.php $is_user_configured) http://docs.moodle.org/en/Development:Coding_style fixed in b46ca3d7 2/ I don't like the way in which preferences need to be established within the database, the outcome is such that there are checks where coding_exceptions are thrown. Sam, can you please explain what you mean? At the point where coding_exception() is used in send_message() I assume that defaults are populated, if not then probably there was a problem during provider installation which I report in exception. 3/ Why is it that message/output now needs to be the first plugin type installed? Is this by chance this tied to the above idea whereby message defaults need to be established. I am REALLY quite unsure about that - we should avoid any chicken-egg problems if possible and this looks like one is being introduced. message/output being first - Ok, I think in this case I think we should probably go for the less efficient option and process defaults after wards. The reason this came was in fact because a patch that Tim is working on involves putting the qtypes in first position as well which got us thinking about how we could best manage the ordering of things. As such there will be a new issue being created after the release of 2.1 to see if we can come up with a happy solution for this. For the time being I think processing defaults after the upgrade and then adding a comment to the effect that if message/output were processed first this could be avoided. fixed in 67147c255 4/ In regards to the lambda functions that are being used we would prefer that they were expanded out, in the case of array_filter its easy with an iteration, with array_sort style functions we'd prefer a new function was created. The reason for this is simply that it clearer what is going on and the code is more self documenting. I've talked to Martin about this - we don't have a hard policy on it is only a preference. Addressed in 72e6af03 5/ After config.php has been included additional require_once calls should use $CFG->dirroot (or $CFG->admindir/$CFG->libdir) fixed in 60bbe7684
          Hide
          Dan Poltawski added a comment -

          We have just had a discussion in the office about how to fix this dependently on plugin ordering problem and identified a better way to solve it:

          1) When a message processor is installed, it checks through all the message providers for the defaults and set them
          2) When a message provider is installed, it sets the defaults for all the message processors which are installed.

          This way we cover both bases, and we also allow message processor defaults to be set for third party modules which may not be installed.

          An example could be a theoretical 'SMS' plugin which comes as a combination of a block and a message processor plugin, users would be free to install one first and then the other and in both cases the message defaults would be set correctly.

          Hopefully that is the last remaining issue and we will complete that and testing tomorrow.

          Show
          Dan Poltawski added a comment - We have just had a discussion in the office about how to fix this dependently on plugin ordering problem and identified a better way to solve it: 1) When a message processor is installed, it checks through all the message providers for the defaults and set them 2) When a message provider is installed, it sets the defaults for all the message processors which are installed. This way we cover both bases, and we also allow message processor defaults to be set for third party modules which may not be installed. An example could be a theoretical 'SMS' plugin which comes as a combination of a block and a message processor plugin, users would be free to install one first and then the other and in both cases the message defaults would be set correctly. Hopefully that is the last remaining issue and we will complete that and testing tomorrow.
          Hide
          Ruslan Kabalin added a comment - - edited

          1) When a message processor is installed, it checks through all the message providers for the defaults and set them

          2) When a message provider is installed, it sets the defaults for all the message processors which are installed.

          implemented in afed8d0f17, as a result of this change we no longer need to populate defaults after installation (introduced in 67147c255).

          Testing

          Fresh install of Moodle 2.0+

          Pass

          Upgrading a Moodle 1.9 site

          Pass

          Upgrading a Moodle 2.0 site without custom message providers

          Pass

          Upgrading a Moodle 2.0 site with custom message providers

          Pass (replicated it by adding a messages.php with new providers to one of existing core plugins).

          Altering a message providers defaults after upgrade

          Pass

          All core message providers have been tested and work

          Tested with some message providers, there is no point to test all as the are using the same code to dispatch messages. Perhaps you meant processors (email, jabber, popup)? If so, I have tested with all core processors.

          All core message providers have been disabled and things still work as expected.

          Pass (I guess you meant processors)

          Show
          Ruslan Kabalin added a comment - - edited 1) When a message processor is installed, it checks through all the message providers for the defaults and set them 2) When a message provider is installed, it sets the defaults for all the message processors which are installed. implemented in afed8d0f17, as a result of this change we no longer need to populate defaults after installation (introduced in 67147c255). Testing Fresh install of Moodle 2.0+ Pass Upgrading a Moodle 1.9 site Pass Upgrading a Moodle 2.0 site without custom message providers Pass Upgrading a Moodle 2.0 site with custom message providers Pass (replicated it by adding a messages.php with new providers to one of existing core plugins). Altering a message providers defaults after upgrade Pass All core message providers have been tested and work Tested with some message providers, there is no point to test all as the are using the same code to dispatch messages. Perhaps you meant processors (email, jabber, popup)? If so, I have tested with all core processors. All core message providers have been disabled and things still work as expected. Pass (I guess you meant processors)
          Hide
          Dan Poltawski added a comment -

          Sam/Eloy, I guess we are now at the point where we think this can be integrated?

          Show
          Dan Poltawski added a comment - Sam/Eloy, I guess we are now at the point where we think this can be integrated?
          Hide
          Ruslan Kabalin added a comment -

          The documentation is now completed: http://docs.moodle.org/en/Development:Messaging_2.0_improvements, any feedback is welcome.

          Show
          Ruslan Kabalin added a comment - The documentation is now completed: http://docs.moodle.org/en/Development:Messaging_2.0_improvements , any feedback is welcome.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          By the sounds of it this is ready.. Hoorah!!
          I'll have one final last look through today and when I see Eloy tonight I'll check with him as to when he wants this to be integrated.
          Presently we planning to integrate this and Tim's Quiz work out of our normal cycle.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, By the sounds of it this is ready.. Hoorah!! I'll have one final last look through today and when I see Eloy tonight I'll check with him as to when he wants this to be integrated. Presently we planning to integrate this and Tim's Quiz work out of our normal cycle. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note the previous comment is an automated one, lol.

          As commented by Sam, this will surely land next week, after we integrate the quiz monster now that the weekly has been generated and we have some sort of "peace" with versions.

          Thanks for the hard work, ciao

          PS: Just thinking if it would be interesting to think about some potential QA tests related with this and share them with Helen, so everything can be tested as part of the QA iterations going to happen along the next weeks.

          Show
          Eloy Lafuente (stronk7) added a comment - Note the previous comment is an automated one, lol. As commented by Sam, this will surely land next week, after we integrate the quiz monster now that the weekly has been generated and we have some sort of "peace" with versions. Thanks for the hard work, ciao PS: Just thinking if it would be interesting to think about some potential QA tests related with this and share them with Helen, so everything can be tested as part of the QA iterations going to happen along the next weeks.
          Hide
          Ruslan Kabalin added a comment -

          You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts.

          Done

          Show
          Ruslan Kabalin added a comment - You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. Done
          Hide
          Sam Hemelryk added a comment -

          This has been integrated now.
          Once again thank you for the work guys the improvements made to messaging configuration are awesome.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - This has been integrated now. Once again thank you for the work guys the improvements made to messaging configuration are awesome. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I've been playing with this and everything seems to work as expected, so I'm passing it.

          Anyway, I've created MDL-27776 about to include complete QA tests for the whole messaging thing, stay tuned.

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I've been playing with this and everything seems to work as expected, so I'm passing it. Anyway, I've created MDL-27776 about to include complete QA tests for the whole messaging thing, stay tuned. Thanks and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is now upstream, yay! Many thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - This is now upstream, yay! Many thanks!
          Hide
          Ruslan Kabalin added a comment -

          Thanks a lot. It was a pleasure working with you guys!

          Show
          Ruslan Kabalin added a comment - Thanks a lot. It was a pleasure working with you guys!
          Hide
          Helen Foster added a comment -

          Ruslan, thanks again for developing the messaging config options. As mentioned in MDL-27776, we have 3 new QA tests for testing messaging functionality: MDLQA-935, MDLQA-936 and MDLQA-937. Feedback on the QA tests is always welcome!

          Show
          Helen Foster added a comment - Ruslan, thanks again for developing the messaging config options. As mentioned in MDL-27776 , we have 3 new QA tests for testing messaging functionality: MDLQA-935 , MDLQA-936 and MDLQA-937 . Feedback on the QA tests is always welcome!
          Hide
          Ruslan Kabalin added a comment -

          Helen, another related question. You initially recommended to create a separate wiki page of this improvement. Now given that messaging made its way to 2.1 the content of this http://docs.moodle.org/dev/Messaging_2.0_improvements should probably replace the content of http://docs.moodle.org/dev/Messaging_2.0 as the latter one is not complete and the former contains all this stuff anyway.

          Show
          Ruslan Kabalin added a comment - Helen, another related question. You initially recommended to create a separate wiki page of this improvement. Now given that messaging made its way to 2.1 the content of this http://docs.moodle.org/dev/Messaging_2.0_improvements should probably replace the content of http://docs.moodle.org/dev/Messaging_2.0 as the latter one is not complete and the former contains all this stuff anyway.
          Hide
          Dan Poltawski added a comment -

          Not sure if there is a clever mediawiki way to do that?

          Show
          Dan Poltawski added a comment - Not sure if there is a clever mediawiki way to do that?
          Hide
          Helen Foster added a comment -

          Yes, there is a mediawiki way to replace Messaging 2.0 with Messaging 2.0 improvements. I just need to get admin rights to the wiki then I'll do it!

          Show
          Helen Foster added a comment - Yes, there is a mediawiki way to replace Messaging 2.0 with Messaging 2.0 improvements. I just need to get admin rights to the wiki then I'll do it!
          Hide
          Helen Foster added a comment -
          Show
          Helen Foster added a comment - Done: http://docs.moodle.org/dev/Messaging_2.0
          Hide
          Dan Poltawski added a comment -

          Hmm, I guess we also need to split some of those docs out into the user docs too

          Show
          Dan Poltawski added a comment - Hmm, I guess we also need to split some of those docs out into the user docs too

            People

            • Votes:
              5 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: