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

      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.

        Gliffy Diagrams

          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: