Moodle
  1. Moodle
  2. MDL-32009

Add missing administrative features to messaging plugins

    Details

    • Type: Improvement Improvement
    • 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:
    • Database:
      Any
    • Testing Instructions:
      Hide

      Open Plugins overview (Site Administration > Plugins > Plugins Overview), scroll down to Messaging processors. All testing scenarios begin from this point.

      Uninstall:

      1. Click uninstall link near message processor
      2. Go through uninstall process and ensure the plugin data has been removed
      3. Now you may go to Site Admin > Notifications and go through instillation process

      Settings:

      1. Ensure the messaging processors settings links point to correct location

      Plugin status:

      1. Ensure the messaging processors enable/disable status reflect whether message processor is enabled and configured (in Site Administration > Plugins > Plugins Overview > Message Outputs > Manage).
      Show
      Open Plugins overview (Site Administration > Plugins > Plugins Overview), scroll down to Messaging processors. All testing scenarios begin from this point. Uninstall: Click uninstall link near message processor Go through uninstall process and ensure the plugin data has been removed Now you may go to Site Admin > Notifications and go through instillation process Settings: Ensure the messaging processors settings links point to correct location Plugin status: Ensure the messaging processors enable/disable status reflect whether message processor is enabled and configured (in Site Administration > Plugins > Plugins Overview > Message Outputs > Manage).
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32009-master-3
    • Rank:
      38681

      Description

      Messaging plugins are missing some general administrative features, there is a room for refactoring:

      • get_settings_url (plugintype_message) may use get_message_processors() data for settings/availability estimation to avoid code duplication
      • Add plugintype_interface::is_enabled method to reflect plugin status
      • Since message_processor_uninstall function already exists, why not using it? Add uninstall links to plugin list.

        Issue Links

          Activity

          Show
          Ruslan Kabalin added a comment - Commit diffs for each feature: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=26d859e0ecd49ee32b5b0d414c9a119281db9b48 https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=6f4f0b5228bc9354cc95233806cf11421b4fc098 https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=0210ce10077f10fb0f92e4a0b50758e943eb838f
          Hide
          Ruslan Kabalin added a comment -

          This is the bit I have done the other day but never proposed for integration.

          Show
          Ruslan Kabalin added a comment - This is the bit I have done the other day but never proposed for integration.
          Hide
          Dan Poltawski added a comment -

          Hi Ruslan,

          Only just seen this - I only get a message about being peer reviewer once you assign me as peer reviewer. So sorry.

          Show
          Dan Poltawski added a comment - Hi Ruslan, Only just seen this - I only get a message about being peer reviewer once you assign me as peer reviewer. So sorry.
          Hide
          Dan Poltawski added a comment -

          The first patch includes message/lib.php, unfortunately that is going to cause problems since this isn't a 'clean' library file. See the $PAGE->set_popup_notification_allowed() as an example of that problem. I believe making this change wold cause that to be executed on every page.

          Show
          Dan Poltawski added a comment - The first patch includes message/lib.php, unfortunately that is going to cause problems since this isn't a 'clean' library file. See the $PAGE->set_popup_notification_allowed() as an example of that problem. I believe making this change wold cause that to be executed on every page.
          Hide
          Ruslan Kabalin added a comment -

          Hi Dan, thanks for review. It was on my local branch before I found time to push it yesterday, so it was not long on the tracker

          I believe making this change wold cause that to be executed on every page.

          Fortunately not, testing revealed that this is executed on admin/plugins.php page only.

          Show
          Ruslan Kabalin added a comment - Hi Dan, thanks for review. It was on my local branch before I found time to push it yesterday, so it was not long on the tracker I believe making this change wold cause that to be executed on every page. Fortunately not, testing revealed that this is executed on admin/plugins.php page only.
          Hide
          Michael de Raadt added a comment -

          Thanks for working on this, Ruslan.

          Show
          Michael de Raadt added a comment - Thanks for working on this, Ruslan.
          Hide
          Ruslan Kabalin added a comment -

          Requesting review again, since the function is not being executed on every page, just on the plugins management one.

          Show
          Ruslan Kabalin added a comment - Requesting review again, since the function is not being executed on every page, just on the plugins management one.
          Hide
          Dan Poltawski added a comment -

          Ok, submitted for integration

          Show
          Dan Poltawski added a comment - Ok, submitted for integration
          Hide
          Sam Hemelryk added a comment -

          Hi Ruslan,

          I've just been looking at this now and have made a few notes:

          1. admin/message.php doesn't need to require messagelib.php, that is always included by setup.php
          2. admin/message.php optional_param calls for uninstall and confirm should really be using INT, and BOOL defaults respectively. This doesn't need to happen itd just be nice.
          3. plugintype_message::get_settings_url doesn't always return a value. If a plugin exists but is disabled and/or doesn't have settings then the function won't return anything. Probably best that it returns parent::get_settings_url() if all else fails.

          Really those three things are completely trivial, if you don't have time to address them in the next day let me know and I'll fix them up during integration (providing you are happy with it of course!)

          I also think that the language strings should probably be run past our language expert Helen, so I've added her as a watcher on this issue. Helen could you please consider the following two new strings (associated with deleting a message processor).

          $string['processordeleteconfirm'] = 'You are about to completely delete message processor \'{$a}\'.  This will completely delete everything in the database associated with this processor. Are you SURE you want to continue?';
          $string['processordeletefiles'] = 'All data associated with the processor \'{$a->processor}\' has been deleted from the database.  To complete the deletion (and prevent the processor re-installing itself), you should now delete this directory from your server: {$a->directory}';
          

          I'll leave this as integration in review so it doesn't get forgotten about.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Ruslan, I've just been looking at this now and have made a few notes: admin/message.php doesn't need to require messagelib.php, that is always included by setup.php admin/message.php optional_param calls for uninstall and confirm should really be using INT, and BOOL defaults respectively. This doesn't need to happen itd just be nice. plugintype_message::get_settings_url doesn't always return a value. If a plugin exists but is disabled and/or doesn't have settings then the function won't return anything. Probably best that it returns parent::get_settings_url() if all else fails. Really those three things are completely trivial, if you don't have time to address them in the next day let me know and I'll fix them up during integration (providing you are happy with it of course!) I also think that the language strings should probably be run past our language expert Helen, so I've added her as a watcher on this issue. Helen could you please consider the following two new strings (associated with deleting a message processor). $string['processordeleteconfirm'] = 'You are about to completely delete message processor \'{$a}\'. This will completely delete everything in the database associated with this processor. Are you SURE you want to continue ?'; $string['processordeletefiles'] = 'All data associated with the processor \'{$a->processor}\' has been deleted from the database. To complete the deletion (and prevent the processor re-installing itself), you should now delete this directory from your server: {$a->directory}'; I'll leave this as integration in review so it doesn't get forgotten about. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Ok I've integrated this now. I've made the changes I noted, Helen if you have a moment and would like to review the strings that would be great.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Ok I've integrated this now. I've made the changes I noted, Helen if you have a moment and would like to review the strings that would be great. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Hi Ruslan,

          Sorry this has been reverted and the issue reopened.
          Turns out it breaks installation, the __construct method of plugintype_message is to blame.
          Needs more work sorry, you'll have to be sure to test installation and upgrade.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Ruslan, Sorry this has been reverted and the issue reopened. Turns out it breaks installation, the __construct method of plugintype_message is to blame. Needs more work sorry, you'll have to be sure to test installation and upgrade. Cheers Sam
          Hide
          Ruslan Kabalin added a comment - - edited

          Sam, are you sure the installation/upgrade issue has been caused by my (this) patch? Has it been merged with no conflicts? I have not manage to replicate it, both installation and upgrade from previous versions goes smoothly (tested upgrade situation as normal and the case when some message processors are uninstalled before upgrade). In addition the code areas I changed should not affect installation and upgrade processes.

          I have updated branch with your suggestions and rebased on most recent master.

          Show
          Ruslan Kabalin added a comment - - edited Sam, are you sure the installation/upgrade issue has been caused by my (this) patch? Has it been merged with no conflicts? I have not manage to replicate it, both installation and upgrade from previous versions goes smoothly (tested upgrade situation as normal and the case when some message processors are uninstalled before upgrade). In addition the code areas I changed should not affect installation and upgrade processes. I have updated branch with your suggestions and rebased on most recent master.
          Hide
          Dan Poltawski added a comment - - edited

          Hi Ruslan,

          Yes - here is the result from continuious integration server:

          Moodle 2.3dev (Build: 20120329) command line installation program
          Default exception handler: Error reading from database Debug: Table 'ci_installed_678_0.cii_message_processors' doesn't exist
          SELECT name, id, enabled FROM cii_message_processors   ORDER BY name DESC
          [array (
          )]
          * line 410 of /lib/dml/moodle_database.php: dml_read_exception thrown
          * line 872 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
          * line 1165 of /lib/dml/moodle_database.php: call to mysqli_native_moodle_database->get_records_sql()
          * line 1114 of /lib/dml/moodle_database.php: call to moodle_database->get_records_select()
          * line 2260 of /message/lib.php: call to moodle_database->get_records()
          * line 1544 of /lib/pluginlib.php: call to get_message_processors()
          * line 706 of /lib/pluginlib.php: call to plugintype_message->__construct()
          * line ? of unknownfile: call to plugintype_base::get_plugins()
          * line 118 of /lib/pluginlib.php: call to call_user_func()
          * line 70 of /lib/pluginlib.php: call to plugin_manager->get_plugins()
          * line 88 of /lib/pluginlib.php: call to plugin_manager->__construct()
          * line 680 of /admin/cli/install.php: call to plugin_manager::instance()
          
          !!! Error reading from database !!!
          Error installing master to test upgrade
          
          Show
          Dan Poltawski added a comment - - edited Hi Ruslan, Yes - here is the result from continuious integration server: Moodle 2.3dev (Build: 20120329) command line installation program Default exception handler: Error reading from database Debug: Table 'ci_installed_678_0.cii_message_processors' doesn't exist SELECT name, id, enabled FROM cii_message_processors ORDER BY name DESC [array ( )] * line 410 of /lib/dml/moodle_database.php: dml_read_exception thrown * line 872 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end() * line 1165 of /lib/dml/moodle_database.php: call to mysqli_native_moodle_database->get_records_sql() * line 1114 of /lib/dml/moodle_database.php: call to moodle_database->get_records_select() * line 2260 of /message/lib.php: call to moodle_database->get_records() * line 1544 of /lib/pluginlib.php: call to get_message_processors() * line 706 of /lib/pluginlib.php: call to plugintype_message->__construct() * line ? of unknownfile: call to plugintype_base::get_plugins() * line 118 of /lib/pluginlib.php: call to call_user_func() * line 70 of /lib/pluginlib.php: call to plugin_manager->get_plugins() * line 88 of /lib/pluginlib.php: call to plugin_manager->__construct() * line 680 of /admin/cli/install.php: call to plugin_manager::instance() !!! Error reading from database !!! Error installing master to test upgrade
          Hide
          Ruslan Kabalin added a comment -

          Ah, it only fails during command line installation. Then it might be the bug there in fact. Will investigate.

          Show
          Ruslan Kabalin added a comment - Ah, it only fails during command line installation. Then it might be the bug there in fact. Will investigate.
          Hide
          Dan Poltawski added a comment -

          Note unrelated but related: MDL-20438 (the fact that the two installers are going down different paths)

          But in this case because the plugintype_message shouldn't fundamentally depend on the db tables being installed.

          Show
          Dan Poltawski added a comment - Note unrelated but related: MDL-20438 (the fact that the two installers are going down different paths) But in this case because the plugintype_message shouldn't fundamentally depend on the db tables being installed.
          Hide
          Helen Foster added a comment -

          Hi Sam and Ruslan,

          Thanks for asking me to review the new strings and apologies for my delay in doing so.

          I think the strings are fine in that they are consistent with similar strings for deleting plugins, however I notice that in Site Administration > Plugins > Plugins Overview (/admin/plugins.php) the phrase 'Messaging processors' is used whereas elsewhere it's 'Message outputs'.

          Is there a good reason for using the two phrases 'Messaging processors' AND 'Message outputs'? (I found it confusing, as I thought they referred to the same stuff.) If not, then I would recommend that the phrase 'Message outputs' is used everywhere, including in the new strings.

          Ruslan, thanks a lot for your work on this issue. From looking at the plugins overview page I can see it will be a nice improvement

          Show
          Helen Foster added a comment - Hi Sam and Ruslan, Thanks for asking me to review the new strings and apologies for my delay in doing so. I think the strings are fine in that they are consistent with similar strings for deleting plugins, however I notice that in Site Administration > Plugins > Plugins Overview (/admin/plugins.php) the phrase 'Messaging processors' is used whereas elsewhere it's 'Message outputs'. Is there a good reason for using the two phrases 'Messaging processors' AND 'Message outputs'? (I found it confusing, as I thought they referred to the same stuff.) If not, then I would recommend that the phrase 'Message outputs' is used everywhere, including in the new strings. Ruslan, thanks a lot for your work on this issue. From looking at the plugins overview page I can see it will be a nice improvement
          Hide
          Sam Hemelryk added a comment -

          Hi Helen,
          Looks to me like it should be `Message outputs` as well, internally they are called processors however we always talk about them as output so I think you are right in suggesting that change.
          If you're happy with it Ruslan then we should go ahead and change it. (Just noting it will have to happen on a new issue however I can do that if you like)

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Helen, Looks to me like it should be `Message outputs` as well, internally they are called processors however we always talk about them as output so I think you are right in suggesting that change. If you're happy with it Ruslan then we should go ahead and change it. (Just noting it will have to happen on a new issue however I can do that if you like) Cheers Sam
          Show
          Ruslan Kabalin added a comment - Branch has been updated. Fixed language inconsistency as it was advised. Previous commits diffs remained the same: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=39aab6ea5c91b64e1e406e437a82afdc7fa43d9f https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=fea51b0aa508801fab3741bb8a69146431d6379b https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=88d5ed7288473ab3ce3c61653d534b96bad41645
          Hide
          Ruslan Kabalin added a comment -

          Notice that the installation issue is not fixed, so it is not ready for integration yet.

          Show
          Ruslan Kabalin added a comment - Notice that the installation issue is not fixed, so it is not ready for integration yet.
          Hide
          Ruslan Kabalin added a comment -

          Fixed cli installation issue, indeed it was caused by attempt to verify plugins using plugin_manager#all_plugins_ok, which caused crash due to the fact that class constructor called function that checked database.

          Three modified commits are:
          https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=f7fd1ce6c5ea586227e724d6f3088b86727861db
          https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=3de3e3fbecbf969f2bcf98145b533c6b4075d38e
          https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=48fb5fa8c8fb1765e2ec3941d99504bc1dd5cdb8

          Show
          Ruslan Kabalin added a comment - Fixed cli installation issue, indeed it was caused by attempt to verify plugins using plugin_manager#all_plugins_ok, which caused crash due to the fact that class constructor called function that checked database. Three modified commits are: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=f7fd1ce6c5ea586227e724d6f3088b86727861db https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=3de3e3fbecbf969f2bcf98145b533c6b4075d38e https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=48fb5fa8c8fb1765e2ec3941d99504bc1dd5cdb8
          Hide
          Ruslan Kabalin added a comment -

          Another review is required due to changes.

          Show
          Ruslan Kabalin added a comment - Another review is required due to changes.
          Hide
          Dan Poltawski added a comment -

          +1

          Show
          Dan Poltawski added a comment - +1
          Hide
          Ruslan Kabalin added a comment -

          cli installation issue has been fixed, Helen's language comments have been addressed. Ready for integration.

          Show
          Ruslan Kabalin added a comment - cli installation issue has been fixed, Helen's language comments have been addressed. Ready for integration.
          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
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Ruslan Kabalin added a comment -

          Rebased and re-pushed: MDL-32009-master-3. Ready for being integrated.

          Show
          Ruslan Kabalin added a comment - Rebased and re-pushed: MDL-32009 -master-3. Ready for being integrated.
          Hide
          Sam Hemelryk added a comment -

          Great - all looks good and has been integrated now

          Show
          Sam Hemelryk added a comment - Great - all looks good and has been integrated now
          Hide
          Andrew Davis added a comment -

          What branches was this integrated into?

          Show
          Andrew Davis added a comment - What branches was this integrated into?
          Hide
          Dan Poltawski added a comment -

          Master only (see the fix version)

          Show
          Dan Poltawski added a comment - Master only (see the fix version)
          Hide
          Andrew Davis added a comment -

          I've raised MDL-32717 about a strict standards warning I got while testing.

          I uninstalled the Jabber message processor.

          "and ensure the plugin data has been removed"

          Not entirely sure what this means. Everything that I could see that looked Jabber related was deleted from message_processors and config_plugins.

          Settings links look good. Passing.

          Show
          Andrew Davis added a comment - I've raised MDL-32717 about a strict standards warning I got while testing. I uninstalled the Jabber message processor. "and ensure the plugin data has been removed" Not entirely sure what this means. Everything that I could see that looked Jabber related was deleted from message_processors and config_plugins. Settings links look good. Passing.
          Hide
          Andrew Davis added a comment - - edited

          Please update the diff URL (if necessary) every time you update the code. It looks like the diff URL only contains a small subset of the code related to this issue.

          Show
          Andrew Davis added a comment - - edited Please update the diff URL (if necessary) every time you update the code. It looks like the diff URL only contains a small subset of the code related to this issue.
          Hide
          Dan Poltawski added a comment -

          Hi Andrew,

          Its a bit difficult with the gitweb interface to do that as it doesn't have a github style compare branches.

          Though I agree putting a specific diff is misleading if not all of it

          Show
          Dan Poltawski added a comment - Hi Andrew, Its a bit difficult with the gitweb interface to do that as it doesn't have a github style compare branches. Though I agree putting a specific diff is misleading if not all of it
          Show
          Ruslan Kabalin added a comment - Hi Andrew, there are four diffs, one for each feature: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=bc795b98e96743fa29654abbaa4f17a681303987 https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=bede23f7bc528ac22151f1ae89f1c8646e12a722 https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=3f9d9e285672967755f00859c00fc02e2ff9ae59 https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=c9e34994cbdcff9c5bb8318ed860d69e375c1891 They all have been mentioned above, though they had different hashes before the last rebase.
          Hide
          Ruslan Kabalin added a comment -

          We did some tricks with our gitweb, and now we are able to produce diffs between branches:

          https://git.luns.net.uk/moodle.git/commitdiff/master..0872ee1dd0b9cf8bc8106f847c445faa3ab4e6dd

          Show
          Ruslan Kabalin added a comment - We did some tricks with our gitweb, and now we are able to produce diffs between branches: https://git.luns.net.uk/moodle.git/commitdiff/master..0872ee1dd0b9cf8bc8106f847c445faa3ab4e6dd
          Hide
          Eloy Lafuente (stronk7) added a comment -
          UPDATE tracker_issues
             SET status = 'Closed',
                comment = 'Thanks!'
          WHEN participants = 'Did a gorgeous work'
          

          This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

          Show
          Eloy Lafuente (stronk7) added a comment - UPDATE tracker_issues SET status = 'Closed', comment = 'Thanks!' WHEN participants = 'Did a gorgeous work' This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

            People

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

              Dates

              • Created:
                Updated:
                Resolved: