Moodle
  1. Moodle
  2. MDL-33618

A way to hide messages that were configured to never be sent

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.2.3, 2.4.4, 2.5.1
    • Fix Version/s: 2.7
    • Component/s: Messages
    • Testing Instructions:
      Hide

      UI Test (Admin should be able to disable any provider)

      1. Log in as admin
      2. Go to Site administration -> Plugins -> Message outputs -> Default message outputs
      3. You should see enabled column for each message provider and each provider should be enabled
      4. Uncheck enabled for Assignment (2.2) notifications
        • Popup notifications and email should be set to disallowed and should be disabled
        • online/not online should be unchecked and should be disabled
      5. Save changes and go to My profile settings -> Messaging
      6. You should not see Assignment 2.2 related settings
      7. Go back to Site administration -> Plugins -> Message outputs -> Default message outputs
      8. Check enabled for Assignment (2.2) notifications
        • Popup notifications and email should be set to Permitted and should not be disabled
        • online/not online should be unchecked but enabled
      9. Save changes and go to My profile settings -> Messaging
      10. You should see Assignment 2.2 related settings
      11. Try above with JS disabled and make sure if Enabled is unchceked, then after saving the provider should be set to "Disallow"

      Functionality test

      1. Log in as admin
      2. Go to Site administration -> Plugins -> Message outputs -> Default message outputs
      3. Uncheck enabled for "Badge creator notifications" and "Badge recipient notifications"
      4. Go to /admin/settings.php?section=messagesettingemail admin settings and make sure that "Allow attachments" is set to NO.
      5. Create a new badge (Site administration -> Badges -> Add a new badge) and set criteria to manually issue by role (Teacher)
      6. Under badge -> Message tab set "Notify badge creator" to "Every time"
      7. Enable badge and Award badge to user.
      8. Check mail/messages and you should not receive any notification.
      9. Go to Site administration -> Plugins -> Message outputs -> Default message outputs
      10. Enable "Badge creator notifications" and "Badge recipient notifications" and check online/not online for both processors.
      11. Award badge to user again and check mail/messages and you should see badge award notification.

      Unit test

      1. Run phpunit lib/tests/messagelib_test.php
      Show
      UI Test (Admin should be able to disable any provider) Log in as admin Go to Site administration -> Plugins -> Message outputs -> Default message outputs You should see enabled column for each message provider and each provider should be enabled Uncheck enabled for Assignment (2.2) notifications Popup notifications and email should be set to disallowed and should be disabled online/not online should be unchecked and should be disabled Save changes and go to My profile settings -> Messaging You should not see Assignment 2.2 related settings Go back to Site administration -> Plugins -> Message outputs -> Default message outputs Check enabled for Assignment (2.2) notifications Popup notifications and email should be set to Permitted and should not be disabled online/not online should be unchecked but enabled Save changes and go to My profile settings -> Messaging You should see Assignment 2.2 related settings Try above with JS disabled and make sure if Enabled is unchceked, then after saving the provider should be set to "Disallow" Functionality test Log in as admin Go to Site administration -> Plugins -> Message outputs -> Default message outputs Uncheck enabled for "Badge creator notifications" and "Badge recipient notifications" Go to /admin/settings.php?section=messagesettingemail admin settings and make sure that "Allow attachments" is set to NO. Create a new badge (Site administration -> Badges -> Add a new badge) and set criteria to manually issue by role (Teacher) Under badge -> Message tab set "Notify badge creator" to "Every time" Enable badge and Award badge to user. Check mail/messages and you should not receive any notification. Go to Site administration -> Plugins -> Message outputs -> Default message outputs Enable "Badge creator notifications" and "Badge recipient notifications" and check online/not online for both processors. Award badge to user again and check mail/messages and you should see badge award notification. Unit test Run phpunit lib/tests/messagelib_test.php
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull Master Branch:
      wip-mdl-33618
    • Story Points:
      13
    • Rank:
      54815
    • Sprint:
      BACKEND Sprint 7

      Description

      A user can view all messages sent to him/her via Recent Notifications under My Profile > Messages. Even messages that the user has configured to not receive or the admin as forced that all users should not receive. There should be a way to suppress messages that where never actually delivered to the user.

        Activity

        Hide
        Mark Nielsen added a comment -

        One solution we were thinking of was to add a new field to the message_read table to flag is a message was never delivered. It could also store multiple values like:

        • 0 = Was delivered and was read
        • 1 = Wasn't delivered and user configured to not receive it
        • 2 = Wasn't delivered and admin configured the site to not receive it

        The values don't have to be 0-2 of course.

        Then, along with the field, offer a configuration to hide or show those messages in Recent Notifications.

        Show
        Mark Nielsen added a comment - One solution we were thinking of was to add a new field to the message_read table to flag is a message was never delivered. It could also store multiple values like: 0 = Was delivered and was read 1 = Wasn't delivered and user configured to not receive it 2 = Wasn't delivered and admin configured the site to not receive it The values don't have to be 0-2 of course. Then, along with the field, offer a configuration to hide or show those messages in Recent Notifications.
        Hide
        Michael de Raadt added a comment -

        Putting this on the DEV backlog as it will involve a DB change.

        Show
        Michael de Raadt added a comment - Putting this on the DEV backlog as it will involve a DB change.
        Hide
        Andrew Davis added a comment -

        This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

        For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

        Show
        Andrew Davis added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
        Hide
        Chris Follin added a comment -

        Increasing priority. Students are seeing messages that were configured for them to not see, which is/can be a severe problem.

        Show
        Chris Follin added a comment - Increasing priority. Students are seeing messages that were configured for them to not see, which is/can be a severe problem.
        Hide
        Martin Dougiamas added a comment -

        Can we have some more information about the problem here, such as example messages that should not be shown?

        To my way of thinking, if a message was sent to you at any time then it should show up here. This is the one place you can see all your messages. Whether it was delivered or not via some output plugin is irrelevant (unless it was from a user you blocked or something).

        Show
        Martin Dougiamas added a comment - Can we have some more information about the problem here, such as example messages that should not be shown? To my way of thinking, if a message was sent to you at any time then it should show up here. This is the one place you can see all your messages. Whether it was delivered or not via some output plugin is irrelevant (unless it was from a user you blocked or something).
        Hide
        Mike Wilday added a comment -

        If a person used notifications - and marked not to receive them via messaging... or they were admin locked so as to not be able to be received, then they should not appear in that messages link.

        Show
        Mike Wilday added a comment - If a person used notifications - and marked not to receive them via messaging... or they were admin locked so as to not be able to be received, then they should not appear in that messages link.
        Hide
        Kris Stokking added a comment -

        To my way of thinking, if a message was sent to you at any time then it should show up here. This is the one place you can see all your messages. Whether it was delivered or not via some output plugin is irrelevant (unless it was from a user you blocked or something).

        The problem is not whether a message was delivered to a user, it's whether the message was intended to be sent to the user. If an administrator disallows all message outputs for an event, end users will still receive those messages. There's no way to configure message outputs within site administration such that the user won't receive messages for the event. However, this is different than how we should treat user messaging defaults - in that case, the user may simply prefer not to receive messages via their message output options, but should still receive the message in My Messages. This is why Mark suggested adding a flag with the 3 values above - that way, we can know whether the message was intended to be sent to the user so we can suppress the messages that were not.

        Show
        Kris Stokking added a comment - To my way of thinking, if a message was sent to you at any time then it should show up here. This is the one place you can see all your messages. Whether it was delivered or not via some output plugin is irrelevant (unless it was from a user you blocked or something). The problem is not whether a message was delivered to a user, it's whether the message was intended to be sent to the user. If an administrator disallows all message outputs for an event, end users will still receive those messages. There's no way to configure message outputs within site administration such that the user won't receive messages for the event. However, this is different than how we should treat user messaging defaults - in that case, the user may simply prefer not to receive messages via their message output options, but should still receive the message in My Messages. This is why Mark suggested adding a flag with the 3 values above - that way, we can know whether the message was intended to be sent to the user so we can suppress the messages that were not.
        Hide
        Martin Dougiamas added a comment -

        if messages were never meant to be sent then why are they in the table in the first place? Surely it's a bug in whatever is sending those messages?

        Show
        Martin Dougiamas added a comment - if messages were never meant to be sent then why are they in the table in the first place? Surely it's a bug in whatever is sending those messages?
        Hide
        Kris Stokking added a comment -

        If message providers were able to make that determination then I would absolutely agree with you. However, they only have a few items at their disposal to make that assessment, none of which are great options.

        In the case of Joule, we have a message provider for course_grade_change. This provider works great for notifying students whenever an instructor has graded their work, except in the case of sum of grades - for courses using that aggregation method, the student is constantly sent a message that they are failing the course (at least until they've approached the end). There's no way we can determine whether the message should be sent or not because the message record contains no useful information to look it up. Amanda Doughty has already raised this in a similar issue (MDL-38405), but that idea was shot down. It's possible that the new event system handles this better, it would be great if someone could clarify that.

        Regardless, administrators should still have the ability to disable message providers should they choose to.

        Show
        Kris Stokking added a comment - If message providers were able to make that determination then I would absolutely agree with you. However, they only have a few items at their disposal to make that assessment, none of which are great options. In the case of Joule, we have a message provider for course_grade_change. This provider works great for notifying students whenever an instructor has graded their work, except in the case of sum of grades - for courses using that aggregation method, the student is constantly sent a message that they are failing the course (at least until they've approached the end). There's no way we can determine whether the message should be sent or not because the message record contains no useful information to look it up. Amanda Doughty has already raised this in a similar issue ( MDL-38405 ), but that idea was shot down. It's possible that the new event system handles this better, it would be great if someone could clarify that. Regardless, administrators should still have the ability to disable message providers should they choose to.
        Hide
        Martin Dougiamas added a comment - - edited

        I should probably look at the code, but I'm short on time.

        Just for documentation of this issue, one more question: if an admin has disabled a provider, then shouldn't we be blocking the providers messages from ever getting into the database? It doesn't have to be up to the provider itself. Otherwise it seems to be just wasting space and cycles.

        Show
        Martin Dougiamas added a comment - - edited I should probably look at the code, but I'm short on time. Just for documentation of this issue, one more question: if an admin has disabled a provider, then shouldn't we be blocking the providers messages from ever getting into the database? It doesn't have to be up to the provider itself. Otherwise it seems to be just wasting space and cycles.
        Hide
        Kris Stokking added a comment -

        That's basically why we created the ticket in the first place - Moodle doesn't have a way to disable message providers, only message outputs (at least via the UI as of 2.5). In fact, message providers are not even listed in the Plugins Overview.

        Show
        Kris Stokking added a comment - That's basically why we created the ticket in the first place - Moodle doesn't have a way to disable message providers, only message outputs (at least via the UI as of 2.5). In fact, message providers are not even listed in the Plugins Overview.
        Hide
        Martin Dougiamas added a comment -

        Quickest solution would be a CFG option to choose between:

        • store all messages regardless of notifications
        • store only messages that had some notification configured

        but a better solution such as above might be possible.

        Show
        Martin Dougiamas added a comment - Quickest solution would be a CFG option to choose between: store all messages regardless of notifications store only messages that had some notification configured but a better solution such as above might be possible.
        Hide
        Rajesh Taneja added a comment -

        Added config option on Default message outputs page to save all notifications regardless of message processor status or if message processor is available.

        This should solve this issue and is backward compatible.

        Show
        Rajesh Taneja added a comment - Added config option on Default message outputs page to save all notifications regardless of message processor status or if message processor is available. This should solve this issue and is backward compatible.
        Hide
        Petr Škoda added a comment -

        Hi, we have discussed this more today. I believe that the current patch introduces confusing setting, I guess a new checkbox next to each notification place that disables the notification completely would be easier to understand for ordinary admins.

        Show
        Petr Škoda added a comment - Hi, we have discussed this more today. I believe that the current patch introduces confusing setting, I guess a new checkbox next to each notification place that disables the notification completely would be easier to understand for ordinary admins.
        Hide
        Rajesh Taneja added a comment -

        Thanks Petr,

        As discussed, I have modified the patch to support disabling of message provider. Which seems more cleaner approach to solve this issue.

        How this will work:

        1. Message provider can be disabled by admin from Default message outputs page
        2. If provider is disabled, then it won't appear in list of messaging option for user under "My profile settings -> Messaging" (To avoid any confusion)
        3. If provider is disabled, then no message will be stored/sent

        Requesting review again.

        Show
        Rajesh Taneja added a comment - Thanks Petr, As discussed, I have modified the patch to support disabling of message provider. Which seems more cleaner approach to solve this issue. How this will work: Message provider can be disabled by admin from Default message outputs page If provider is disabled, then it won't appear in list of messaging option for user under "My profile settings -> Messaging" (To avoid any confusion) If provider is disabled, then no message will be stored/sent Requesting review again.
        Hide
        Petr Škoda added a comment -

        +1 for master, thanks a lot

        IN my opinion this is not a blocker, it is a nice new feature. I do not think we should backport this.

        Show
        Petr Škoda added a comment - +1 for master, thanks a lot IN my opinion this is not a blocker, it is a nice new feature. I do not think we should backport this.
        Hide
        Kris Stokking added a comment -

        I have a quick question about the UI Test. Steps 4 and 8 seem to indicate that the value for the message output and defaults would change depending on whether the message provider was enabled/disabled. Wouldn't we want to retain the values, but simply disable the form fields? Thanks very much for working on this!

        Show
        Kris Stokking added a comment - I have a quick question about the UI Test. Steps 4 and 8 seem to indicate that the value for the message output and defaults would change depending on whether the message provider was enabled/disabled. Wouldn't we want to retain the values, but simply disable the form fields? Thanks very much for working on this!
        Hide
        Rajesh Taneja added a comment - - edited

        Thanks Petr,

        Kris, 4-8 point have been added to avoid confusion for normal user.
        When message provider is disabled, user should not see that option as he/she will never receive any msg from that provider.

        Pushing it for integration.

        Show
        Rajesh Taneja added a comment - - edited Thanks Petr, Kris, 4-8 point have been added to avoid confusion for normal user. When message provider is disabled, user should not see that option as he/she will never receive any msg from that provider. Pushing it for integration.
        Hide
        Dan Poltawski added a comment -

        Holding this issue whilst we are on-sync

        Show
        Dan Poltawski added a comment - Holding this issue whilst we are on-sync
        Hide
        Sam Hemelryk added a comment -

        Thanks Raj - this has been integrated now.

        Show
        Sam Hemelryk added a comment - Thanks Raj - this has been integrated now.
        Hide
        David Monllaó added a comment -

        Hi Jason, assigning you a new test as you offered your helpful services Michael told me that he's busy with other stuff

        Show
        David Monllaó added a comment - Hi Jason, assigning you a new test as you offered your helpful services Michael told me that he's busy with other stuff
        Hide
        Petr Škoda added a comment -

        works as described without problems, passing

        Show
        Petr Škoda added a comment - works as described without problems, passing
        Hide
        Dan Poltawski added a comment -

        Thanks for your contributions, this change is now upstream!

        “ If debugging is the process of removing software bugs, then programming must be the process of putting them in. ” - Edsger Dijkstra

        Show
        Dan Poltawski added a comment - Thanks for your contributions, this change is now upstream! “ If debugging is the process of removing software bugs, then programming must be the process of putting them in. ” - Edsger Dijkstra

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Agile