Moodle
  1. Moodle
  2. MDL-29548

Let message outputs have some say in what the defaults are

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1, 2.2
    • Fix Version/s: 2.1.2
    • Component/s: Messages
    • Labels:
    • Testing Instructions:
      Hide

      1. Do a clean install. Make sure the messaging defaults are correct. (email and popup permitted for everything. Email on and popup off for everything except personal messaging where popup is on when online.)

      2. If possible, make a new messaging output (copy an existing one and rename?), and override the get_default_messaging_settings method to return MESSAGE_FORCED, or something. Make sure this default is used for all message type.

      3. If possible, try installing a plugin that defines some new message_providers in its messages.php file. Make sure that these messages get the right defaults (from messages.php if defined there, otherwise the ones defined by the outputs.)

      (One way to do 3. is to go to Site administration / Plugins / Activity modules / Manage activities and use the delete link to uninstall a module that has message providers, then let it re-install itself.)

      Show
      1. Do a clean install. Make sure the messaging defaults are correct. (email and popup permitted for everything. Email on and popup off for everything except personal messaging where popup is on when online.) 2. If possible, make a new messaging output (copy an existing one and rename?), and override the get_default_messaging_settings method to return MESSAGE_FORCED, or something. Make sure this default is used for all message type. 3. If possible, try installing a plugin that defines some new message_providers in its messages.php file. Make sure that these messages get the right defaults (from messages.php if defined there, otherwise the ones defined by the outputs.) (One way to do 3. is to go to Site administration / Plugins / Activity modules / Manage activities and use the delete link to uninstall a module that has message providers, then let it re-install itself.)
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      At the OU, we are making a new message output. What we would to happen is that for every type of message provider, the default is set to MESSAGE_DISALLOWED (so we can selectively turn it on just for the things we want).

      I am about to make patch to implement this. It would be really great if this could get into 2.1 stable branch, but we realise that may not be possible.

        Gliffy Diagrams

          Activity

          Hide
          Tim Hunt added a comment -

          This is my proposed implementation.

          I have not had time to test it yet.

          Show
          Tim Hunt added a comment - This is my proposed implementation. I have not had time to test it yet.
          Hide
          Tim Hunt added a comment -

          Now that I have written the testing instructions, you will see why it has to wait until tomorrow

          Show
          Tim Hunt added a comment - Now that I have written the testing instructions, you will see why it has to wait until tomorrow
          Hide
          Tim Hunt added a comment -

          Related developer chat logs at http://moodle.org/mod/cvsadmin/view.php?conversationid=8432#c299733. Thanks Eloy for the peer review.

          I have now done the necessary testing and fixed the bugs I found. Pushing for integration.

          Show
          Tim Hunt added a comment - Related developer chat logs at http://moodle.org/mod/cvsadmin/view.php?conversationid=8432#c299733 . Thanks Eloy for the peer review. I have now done the necessary testing and fixed the bugs I found. Pushing for integration.
          Hide
          Tim Hunt added a comment -

          Given the possibly imminent 2.1.2 release, I would understand if you delay integrating this until 2.1.2+.

          Show
          Tim Hunt added a comment - Given the possibly imminent 2.1.2 release, I would understand if you delay integrating this until 2.1.2+.
          Hide
          Mark Nielsen added a comment -

          I like this.

          Show
          Mark Nielsen added a comment - I like this.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          It seems that the get_message_processor() does not observe/use the $mustexist parameter at all.

          Apart from that... I think the patch looks perfect and fully BC with older harcoded defaults.

          My only concern is if this, as improvement, should be considered to land into stable ever, so initially I'm aiming to integrate it only to master.

          Explicit approval from MD will be required to add this to stable, in case you are really interested / need it.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - It seems that the get_message_processor() does not observe/use the $mustexist parameter at all. Apart from that... I think the patch looks perfect and fully BC with older harcoded defaults. My only concern is if this, as improvement, should be considered to land into stable ever, so initially I'm aiming to integrate it only to master. Explicit approval from MD will be required to add this to stable, in case you are really interested / need it. Ciao
          Hide
          Tim Hunt added a comment -

          Eloy, you are right about the get_message_processor API. That function is only called in one place, and that needs the $mustexist = true, so I propose we get rid of that parameter. Is that OK with you? I can do another commit tomorrow morning that fixes this, or it can wait for next week.

          Martin (or Michael), what are the odds of getting this into 2.1? I need this at the OU, so it is much easier for me if it gets into the stable branch. As Eloy says, fully backwards-compatible. On the other hand, it is kind-of a new feature, so I can see why in principle you would say no on a stable branch. What are your thoughts?

          Show
          Tim Hunt added a comment - Eloy, you are right about the get_message_processor API. That function is only called in one place, and that needs the $mustexist = true, so I propose we get rid of that parameter. Is that OK with you? I can do another commit tomorrow morning that fixes this, or it can wait for next week. Martin (or Michael), what are the odds of getting this into 2.1? I need this at the OU, so it is much easier for me if it gets into the stable branch. As Eloy says, fully backwards-compatible. On the other hand, it is kind-of a new feature, so I can see why in principle you would say no on a stable branch. What are your thoughts?
          Hide
          Martin Dougiamas added a comment -

          If it's truly backward compatible as you say then I think it's OK to land in stable as well. New message processors are not exactly crowding http://moodle.org/plugins/browse.php?list=category&id=24

          +1

          Show
          Martin Dougiamas added a comment - If it's truly backward compatible as you say then I think it's OK to land in stable as well. New message processors are not exactly crowding http://moodle.org/plugins/browse.php?list=category&id=24 +1
          Hide
          Dan Poltawski added a comment -

          Adding Ruslan here.

          Show
          Dan Poltawski added a comment - Adding Ruslan here.
          Hide
          Tim Hunt added a comment -

          Commits amended to remove the unused $mustexist parameter.

          Show
          Tim Hunt added a comment - Commits amended to remove the unused $mustexist parameter.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Rossiani Wijaya added a comment -

          Testing instruction #1 works as it stated.

          I'm still in process on figuring out test instruction #2 and #3. I will provide more update later.

          Show
          Rossiani Wijaya added a comment - Testing instruction #1 works as it stated. I'm still in process on figuring out test instruction #2 and #3. I will provide more update later.
          Hide
          Tim Hunt added a comment -

          Let me know (in dev chat, for example) if I can help you with the testing at all.

          Show
          Tim Hunt added a comment - Let me know (in dev chat, for example) if I can help you with the testing at all.
          Hide
          Rossiani Wijaya added a comment -

          Re-assign tester to Andrew who has more experience with messaging.

          Show
          Rossiani Wijaya added a comment - Re-assign tester to Andrew who has more experience with messaging.
          Hide
          Andrew Davis added a comment -

          Ok. The testing took some doing but this looks great

          Show
          Andrew Davis added a comment - Ok. The testing took some doing but this looks great
          Hide
          Andrew Davis added a comment -

          While testing I noticed this MDL-29681

          Show
          Andrew Davis added a comment - While testing I noticed this MDL-29681
          Hide
          Eloy Lafuente (stronk7) added a comment -

          git repositories have been updated with your awesome changes, thanks! Closing.

          Show
          Eloy Lafuente (stronk7) added a comment - git repositories have been updated with your awesome changes, thanks! Closing.

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: