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

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

    Details

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

      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.

        Gliffy Diagrams

          Activity

          Hide
          kabalin Ruslan Kabalin added a comment -

          Should cherry-pick cleanly to earlier branches

          Show
          kabalin Ruslan Kabalin added a comment - Should cherry-pick cleanly to earlier branches
          Hide
          andyjdavis 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
          andyjdavis 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
          kabalin 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
          kabalin 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
          kabalin Ruslan Kabalin added a comment -

          Ready for second go

          Show
          kabalin Ruslan Kabalin added a comment - Ready for second go
          Hide
          andyjdavis 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
          andyjdavis 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
          kabalin Ruslan Kabalin added a comment -

          Thanks again Andrew, done.

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

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

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

          Works as expected now.
          Thanks

          Show
          ankit_frenz Ankit Agarwal added a comment - Works as expected now. Thanks
          Hide
          stronk7 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
          stronk7 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:
                Fix Release Date:
                25/Jun/12