Moodle
  1. Moodle
  2. MDL-33019

Messaging: listed providers are not checked against their plugin existence and enabling

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.2, 2.3
    • Fix Version/s: 2.3
    • Component/s: Messages
    • Labels:
      None
    • Testing Instructions:
      Hide
      1. Open Site administration > Plugins > Message outputs > Default message outputs
      2. In another tab open My profile settings > Messaging
      3. Check that assignment (for instance) provider is listed on both pages.
      4. Go to Manage Activities and disable Assignment
      5. Refresh Site administration > Plugins > Message outputs > Default message outputs
      6. In another tab refresh My profile settings > Messaging
      7. Check that assignment (for instance) provider is not listed on both pages.
      Show
      Open Site administration > Plugins > Message outputs > Default message outputs In another tab open My profile settings > Messaging Check that assignment (for instance) provider is listed on both pages. Go to Manage Activities and disable Assignment Refresh Site administration > Plugins > Message outputs > Default message outputs In another tab refresh My profile settings > Messaging Check that assignment (for instance) provider is not listed on both pages.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-33019-master-2
    • Rank:
      40195

      Description

      The list of providers on the systems messaging default outputs grid (Site administration > Plugins > Message outputs > Default message outputs) and on the user messaging preferences grid (My profile settings > Messaging) list and allows configuring providers whose plugins are disabled or does not exists.

        Activity

        Hide
        Ruslan Kabalin added a comment -

        Should cherry-pick cleanly to earlier branches

        Show
        Ruslan Kabalin added a comment - Should cherry-pick cleanly to earlier branches
        Hide
        Andrew Davis added a comment -

        Hi.

        // Remove all the providers whose plugins are disabled or not exists

        should be something like

        // Remove all the providers whose plugins are disabled or dont exist

        2 things about this bit.

        if (!$plugin->is_enabled()) {
            unset($providers[$providerid]);   // Not allowed to see this
            continue;
        }
        

        "// Not allowed to see this" implies that there is some sort of capability check going on rather than the plugin being disabled for the whole site. "// Plugin is disabled" perhaps.

        Reading the phpdocs of is_enabled() in lib/pluginlib.php it sounds like it can return true, false or null. Null apparently means that the plugin does not support being disabled and is thus always available. For example, the forum module cannot be disabled and so does not implement is_enabled(). The if statement probably wants to be $plugin->isenabled()===false.

        Steps 1 and 5 seem like they can just be removed from the testing instructions. They are talking about default message processors while this issue is about message providers.

        Show
        Andrew Davis added a comment - Hi. // Remove all the providers whose plugins are disabled or not exists should be something like // Remove all the providers whose plugins are disabled or dont exist 2 things about this bit. if (!$plugin->is_enabled()) { unset($providers[$providerid]); // Not allowed to see this continue ; } "// Not allowed to see this" implies that there is some sort of capability check going on rather than the plugin being disabled for the whole site. "// Plugin is disabled" perhaps. Reading the phpdocs of is_enabled() in lib/pluginlib.php it sounds like it can return true, false or null. Null apparently means that the plugin does not support being disabled and is thus always available. For example, the forum module cannot be disabled and so does not implement is_enabled(). The if statement probably wants to be $plugin->isenabled()===false. Steps 1 and 5 seem like they can just be removed from the testing instructions. They are talking about default message processors while this issue is about message providers.
        Hide
        Ruslan Kabalin added a comment -

        Hello Andrew, thanks a lot for revision, I have updated the branch with suggested changes. Regarding steps 1 and 5 of testing instruction, Default message outputs page contains a grid of providers and processors. Providers that do not exist or disabled should not be listed there (the same as for disabled processors, which is already implemented).

        Show
        Ruslan Kabalin added a comment - Hello Andrew, thanks a lot for revision, I have updated the branch with suggested changes. Regarding steps 1 and 5 of testing instruction, Default message outputs page contains a grid of providers and processors. Providers that do not exist or disabled should not be listed there (the same as for disabled processors, which is already implemented).
        Hide
        Ruslan Kabalin added a comment -

        Ready for second go

        Show
        Ruslan Kabalin added a comment - Ready for second go
        Hide
        Andrew Davis added a comment -

        "Default message outputs page contains a grid of providers and processors."

        Doh. Of course. I'd still recommend two small tweaks to the testing instructions. Change "Check that assignment (for instance) provider is there." to "Check that assignment (for instance) provider is listed on both pages." and "Check that assignment (for instance) provider is not listed." to "Check that assignment (for instance) provider is not listed on both pages."

        Submit for integration whenever you're ready

        Show
        Andrew Davis added a comment - "Default message outputs page contains a grid of providers and processors." Doh. Of course. I'd still recommend two small tweaks to the testing instructions. Change "Check that assignment (for instance) provider is there." to "Check that assignment (for instance) provider is listed on both pages." and "Check that assignment (for instance) provider is not listed." to "Check that assignment (for instance) provider is not listed on both pages." Submit for integration whenever you're ready
        Hide
        Ruslan Kabalin added a comment -

        Thanks again Andrew, done.

        Show
        Ruslan Kabalin added a comment - Thanks again Andrew, done.
        Hide
        Dan Poltawski 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.

        TIA and ciao

        Show
        Dan Poltawski 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. TIA and ciao
        Hide
        Dan Poltawski added a comment -

        Thanks for this change, I think it will be particularly useful with the two assignment modules. (Hopefully most sites will disable the old ones)

        Show
        Dan Poltawski added a comment - Thanks for this change, I think it will be particularly useful with the two assignment modules. (Hopefully most sites will disable the old ones)
        Hide
        Dan Poltawski added a comment -

        I've integrated this now, thanks.

        I did not feel confident enough in backporting this to the stable branches since there have been a lot of changes in pluginlib recently, but this could be done in a new issue if necessary.

        Show
        Dan Poltawski added a comment - I've integrated this now, thanks. I did not feel confident enough in backporting this to the stable branches since there have been a lot of changes in pluginlib recently, but this could be done in a new issue if necessary.
        Hide
        Dan Poltawski added a comment -

        Ankit pointed out that I hadn't actually pushed this, oops!

        Show
        Dan Poltawski added a comment - Ankit pointed out that I hadn't actually pushed this, oops!
        Hide
        Ankit Agarwal added a comment -

        Works as expected now.
        Thanks

        Show
        Ankit Agarwal added a comment - Works as expected now. Thanks
        Hide
        Eloy Lafuente (stronk7) added a comment -

        We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3.

        Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish.

        Many thanks for your collaboration!

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3. Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish. Many thanks for your collaboration! Ciao

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: