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

Let message outputs have some say in what the defaults are

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: 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

          Attachments

            Activity

            Hide
            timhunt Tim Hunt added a comment -

            This is my proposed implementation.

            I have not had time to test it yet.

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

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

            Show
            timhunt Tim Hunt added a comment - Now that I have written the testing instructions, you will see why it has to wait until tomorrow
            Hide
            timhunt 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
            timhunt 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
            timhunt 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
            timhunt 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
            bushido Mark Nielsen added a comment -

            I like this.

            Show
            bushido Mark Nielsen added a comment - I like this.
            Hide
            stronk7 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
            stronk7 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
            timhunt 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
            timhunt 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
            dougiamas 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
            dougiamas 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
            poltawski Dan Poltawski added a comment -

            Adding Ruslan here.

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

            Commits amended to remove the unused $mustexist parameter.

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

            Integrated, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
            Hide
            rwijaya 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
            rwijaya 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
            timhunt Tim Hunt added a comment -

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

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

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

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

            Ok. The testing took some doing but this looks great

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

            While testing I noticed this MDL-29681

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

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

            Show
            stronk7 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:
                  Fix Release Date:
                  10/Oct/11