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:
    • Rank:
      19264

      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.

        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: