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

Give local plugins the ability to add to the "Course administration" navigation

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.3
    • Component/s: Navigation
    • Labels:
    • Testing Instructions:
      Hide

      1. Unpack the attached test local plugin ZIP to the 'local' directory and run Notifications.
      2. Visit site home, find "MDL-30506 front page admin node" under Settings > Front page settings.
      3. Visit my home, find "MDL-30506 user admin node" under Settings > My profile settings.
      4. Visit a course page, find "MDL-30506 course admin node" under Settings > Course administration.

      Show
      1. Unpack the attached test local plugin ZIP to the 'local' directory and run Notifications. 2. Visit site home, find " MDL-30506 front page admin node" under Settings > Front page settings. 3. Visit my home, find " MDL-30506 user admin node" under Settings > My profile settings. 4. Visit a course page, find " MDL-30506 course admin node" under Settings > Course administration.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      This patch adds the ability for local plugins to add nodes to the 'Course administration' navigation in the Settings block in much the same way that MDL-22209 did for the global navigation tree.

      Local plugins can then implement a

      {pluginname}_extends_course_settings_navigation(navigation_node $nav, $coursecontext) function in their /local/{pluginname}

      /lib.php file. And if they want the current course id, they can look at $coursecontext->instanceid.

        Gliffy Diagrams

        1. local-plugin-extend-course-settings-nav.patch
          1 kB
          Jonathon Fowler

          Issue Links

            Activity

            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Jonathon,

            I've just been having a look and a think about the settings navigation.
            In the case of settings it is primarily context driven, what ever the current context is has an opportunity to extend it, as do any parents of the current context.
            Things such as local plugins, and in fact most other plugin types except blocks and modules don't have contexts, and as such only ever have the opportunity to extend the settings once the user is viewing a page belonging to the plugin.
            This is very much the intended functionality of the settings to show only what is relevant to current page, plus maintain any parent state.

            The patch you have come up with identifies a need that is presently unmet in the settings navigation and that is to hook into the initialisation of the settings allowing other areas of code to modify the generated structure.
            For most plugins this is not something that we would want (no need for reports for example to hook into settings generation) however the local plugins are a little different, and certainly giving them the ability to modify the settings would open a few doors.

            In regards to your changes I think they are very good, however I wonder whether we should consider broadening the scope. Instead of creating a callback just for course settings why not create a callback after the admin settings have been generated that passes in the current context and the settings tree. Doing that would allow local plugins to extend not only the course settings but any other area as well (for sure my profile settings would be another area I can see people wanted to extend).
            Certainly broadening things means that developers will have to have a better understanding of how the settings navigation works as they will need to locate the area of interest, inspect it, and then modify it. However it will be advantageous because much more will be possible and we will avoid the pitfall of further callbacks being introduced in the future.
            How does that sound to you?

            Cheers
            Sam

            p.s. have left this for MdR to triage - sure he will be interested.

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Jonathon, I've just been having a look and a think about the settings navigation. In the case of settings it is primarily context driven, what ever the current context is has an opportunity to extend it, as do any parents of the current context. Things such as local plugins, and in fact most other plugin types except blocks and modules don't have contexts, and as such only ever have the opportunity to extend the settings once the user is viewing a page belonging to the plugin. This is very much the intended functionality of the settings to show only what is relevant to current page, plus maintain any parent state. The patch you have come up with identifies a need that is presently unmet in the settings navigation and that is to hook into the initialisation of the settings allowing other areas of code to modify the generated structure. For most plugins this is not something that we would want (no need for reports for example to hook into settings generation) however the local plugins are a little different, and certainly giving them the ability to modify the settings would open a few doors. In regards to your changes I think they are very good, however I wonder whether we should consider broadening the scope. Instead of creating a callback just for course settings why not create a callback after the admin settings have been generated that passes in the current context and the settings tree. Doing that would allow local plugins to extend not only the course settings but any other area as well (for sure my profile settings would be another area I can see people wanted to extend). Certainly broadening things means that developers will have to have a better understanding of how the settings navigation works as they will need to locate the area of interest, inspect it, and then modify it. However it will be advantageous because much more will be possible and we will avoid the pitfall of further callbacks being introduced in the future. How does that sound to you? Cheers Sam p.s. have left this for MdR to triage - sure he will be interested.
            Hide
            jonof Jonathon Fowler added a comment -

            It sounds like a very good idea. Much better than the very specific hook I've done. I'll see if I can create a new patch that does what you describe.

            Thanks

            Jonathon

            Show
            jonof Jonathon Fowler added a comment - It sounds like a very good idea. Much better than the very specific hook I've done. I'll see if I can create a new patch that does what you describe. Thanks Jonathon
            Hide
            jonof Jonathon Fowler added a comment -

            This new patch seems to work, though I'm still testing for side-effects: https://github.com/jonof/moodle/commit/5597356132fd7ce4d0f45f1630bb891006bdfc7b

            And this would be a basic template for injecting navigation into the various parts of the tree at the different context levels:

            function myplugin_extends_settings_navigation(settings_navigation $nav, $context)
            {
                if ($context->contextlevel >= CONTEXT_USER && ($branch = $nav->get('usercurrentsettings'))) {
                    $branch->add('New user admin node');
                }
                if ($context->contextlevel >= CONTEXT_COURSECAT) {
                    // more difficult because there's no handle given to the 'Category: xxx' node
                }
                if ($context->contextlevel >= CONTEXT_COURSE && ($branch = $nav->get('frontpage'))) {
                    $coursectx = get_course_context($context);
                    $branch->add('New front page admin node');
                } else if ($context->contextlevel >= CONTEXT_COURSE && ($branch = $nav->get('courseadmin'))) {
                    $coursectx = get_course_context($context);
                    $branch->add('New course admin node');
                }
            }

            Jonathon

            Show
            jonof Jonathon Fowler added a comment - This new patch seems to work, though I'm still testing for side-effects: https://github.com/jonof/moodle/commit/5597356132fd7ce4d0f45f1630bb891006bdfc7b And this would be a basic template for injecting navigation into the various parts of the tree at the different context levels: function myplugin_extends_settings_navigation(settings_navigation $nav, $context) { if ($context->contextlevel >= CONTEXT_USER && ($branch = $nav->get('usercurrentsettings'))) { $branch->add('New user admin node'); } if ($context->contextlevel >= CONTEXT_COURSECAT) { // more difficult because there's no handle given to the 'Category: xxx' node } if ($context->contextlevel >= CONTEXT_COURSE && ($branch = $nav->get('frontpage'))) { $coursectx = get_course_context($context); $branch->add('New front page admin node'); } else if ($context->contextlevel >= CONTEXT_COURSE && ($branch = $nav->get('courseadmin'))) { $coursectx = get_course_context($context); $branch->add('New course admin node'); } } Jonathon
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for sharing that.

            Certainly local plugins have potential that we need to encourage people to discover and this will give them more potential.

            My +1 for this.

            Sam, if you're keen, we could include this in the next sprint.

            Show
            salvetore Michael de Raadt added a comment - Thanks for sharing that. Certainly local plugins have potential that we need to encourage people to discover and this will give them more potential. My +1 for this. Sam, if you're keen, we could include this in the next sprint.
            Hide
            salvetore Michael de Raadt added a comment -

            Hi, Jono.

            I've elevated you in tracker so you can now send issues for peer review. If you'd like to you can share your changes across appropriate Git branches and then press the peer review button.

            Show
            salvetore Michael de Raadt added a comment - Hi, Jono. I've elevated you in tracker so you can now send issues for peer review. If you'd like to you can share your changes across appropriate Git branches and then press the peer review button.
            Hide
            jonof Jonathon Fowler added a comment -

            Finally prepared some branches for you folks:
            https://github.com/jonof/moodle/compare/master...MDL-30506
            https://github.com/jonof/moodle/compare/MOODLE_21_STABLE...MDL-30506_21
            https://github.com/jonof/moodle/compare/MOODLE_22_STABLE...MDL-30506_22

            There's an additional commit in there to add keys to the navigation nodes for block, category, and course module settings. These presently have no identifying handle which makes them rather difficult to extend.

            And this would be the example code I'd include in docs, etc:

            function myplugin_extends_settings_navigation(settings_navigation $nav, $context)
            {
                if (($branch = $nav->get('usercurrentsettings'))) {
                    $branch->add('New user admin node');
                }
                if (($branch = $nav->get('categoryadmin'))) {
                    $branch->add('New category admin node');
                }
                if (($branch = $nav->get('frontpage'))) {
                    $branch->add('New front page admin node');
                } else if (($branch = $nav->get('courseadmin'))) {
                    $branch->add('New course admin node');
                }
                if (($branch = $nav->get('moduleadmin'))) {
                    $branch->add('New course module admin node');
                }
                if (($branch = $nav->get('blockadmin'))) {
                    $branch->add('New block admin node');
                }
            }

            Enjoy

            Jonathon

            Show
            jonof Jonathon Fowler added a comment - Finally prepared some branches for you folks: https://github.com/jonof/moodle/compare/master...MDL-30506 https://github.com/jonof/moodle/compare/MOODLE_21_STABLE...MDL-30506_21 https://github.com/jonof/moodle/compare/MOODLE_22_STABLE...MDL-30506_22 There's an additional commit in there to add keys to the navigation nodes for block, category, and course module settings. These presently have no identifying handle which makes them rather difficult to extend. And this would be the example code I'd include in docs, etc: function myplugin_extends_settings_navigation(settings_navigation $nav, $context) { if (($branch = $nav->get('usercurrentsettings'))) { $branch->add('New user admin node'); } if (($branch = $nav->get('categoryadmin'))) { $branch->add('New category admin node'); } if (($branch = $nav->get('frontpage'))) { $branch->add('New front page admin node'); } else if (($branch = $nav->get('courseadmin'))) { $branch->add('New course admin node'); } if (($branch = $nav->get('moduleadmin'))) { $branch->add('New course module admin node'); } if (($branch = $nav->get('blockadmin'))) { $branch->add('New block admin node'); } } Enjoy Jonathon
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Putting this up for peer-review (thanks for the branches Jonathon)

            Show
            samhemelryk Sam Hemelryk added a comment - Putting this up for peer-review (thanks for the branches Jonathon)
            Hide
            jonof Jonathon Fowler added a comment -

            Looks like we both edited the issue at the same time, and mine put you back as the peer reviewer. Am I responsible for declaring the pull branches, or have I contravened your processes?

            Show
            jonof Jonathon Fowler added a comment - Looks like we both edited the issue at the same time, and mine put you back as the peer reviewer. Am I responsible for declaring the pull branches, or have I contravened your processes?
            Hide
            salvetore Michael de Raadt added a comment -

            Assigning Raj as peer reviewer and Sam as integrator.

            Show
            salvetore Michael de Raadt added a comment - Assigning Raj as peer reviewer and Sam as integrator.
            Hide
            salvetore Michael de Raadt added a comment -

            Oh, to answer your question, what you have done with the pull branches is great. Even at HQ, that is how it is done, in private repositories. It all meets at integration.

            Show
            salvetore Michael de Raadt added a comment - Oh, to answer your question, what you have done with the pull branches is great. Even at HQ, that is how it is done, in private repositories. It all meets at integration.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for all the hard work Jonathon.

            Patch looks good to me, although there are few cosmetic things you might want to consider:

            1. Commit should be "MDL-30506 Navigation: hook for local plugins to extend the settings_navigation tree" ( MDL-XXXXX {Component}

              : )

            2. curly bracket goes on the same line as function declaration "protected function load_local_plugin_settings() {"
            3. Please add testing instructions.
            4. IMO 2nd commit should go in another issue, as it's a different issue.

            I am not sure how it's going to affect performance. Should this go with another navigation config?

            FYI: Please re-base your branches, before it gets to integration.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for all the hard work Jonathon. Patch looks good to me, although there are few cosmetic things you might want to consider: Commit should be " MDL-30506 Navigation: hook for local plugins to extend the settings_navigation tree" ( MDL-XXXXX {Component} : ) curly bracket goes on the same line as function declaration "protected function load_local_plugin_settings() {" Please add testing instructions. IMO 2nd commit should go in another issue, as it's a different issue. I am not sure how it's going to affect performance. Should this go with another navigation config? FYI: Please re-base your branches, before it gets to integration.
            Hide
            jonof Jonathon Fowler added a comment -

            Acknowledged. I'll see what I can do.

            Show
            jonof Jonathon Fowler added a comment - Acknowledged. I'll see what I can do.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Jonathon

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Jonathon
            Hide
            jonof Jonathon Fowler added a comment -
            1. commit message is updated to the correct format
            2. curly bracket fixed
            3. I've attached a simple local plugin designed to add entries to site home, my home, and course pages. Testing instructions updated.
            4. MDL-32191 is created for adding the keys for block, module, and category navigation

            Branches rebased too.

            Show
            jonof Jonathon Fowler added a comment - commit message is updated to the correct format curly bracket fixed I've attached a simple local plugin designed to add entries to site home, my home, and course pages. Testing instructions updated. MDL-32191 is created for adding the keys for block, module, and category navigation Branches rebased too.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Jonathon,
            Pushing it for integration review

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Jonathon, Pushing it for integration review
            Hide
            salvetore Michael de Raadt added a comment -

            Good to see this getting integrated. Thanks for your efforts, Jonathon.

            Show
            salvetore Michael de Raadt added a comment - Good to see this getting integrated. Thanks for your efforts, Jonathon.
            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
            samhemelryk Sam Hemelryk added a comment -

            Hi guys,

            I'm sending this back presently sorry because there are a couple of things to change/consider.

            1. All new callbacks should start with the plugin component. e.g local_plugin_extend_settings_navigation. The only exeception is mod in some circumstances.
            2. There is a function especially for finding all plugins with a particular function and a particular file (by default lib.php).

            I had a quick look at this and the following should work:

             
            diff --git a/lib/navigationlib.php b/lib/navigationlib.php
            index eaf5218..b8beba7 100644
            --- a/lib/navigationlib.php
            +++ b/lib/navigationlib.php
            @@ -3308,6 +3308,9 @@ class settings_navigation extends navigation_node {
                         $this->add(get_string('returntooriginaluser', 'moodle', fullname($realuser, true)), $url, self::TYPE_SETTING, null, null, new pix_icon('t/left', ''));
                     }
             
            +        // At this point we give any local plugins the ability to extend/tinker with the navigation settings.
            +        $this->load_local_plugin_settings();
            +
                     foreach ($this->children as $key=>$node) {
                         if ($node->nodetype != self::NODETYPE_BRANCH || $node->children->count()===0) {
                             $node->remove();
            @@ -4393,6 +4396,17 @@ class settings_navigation extends navigation_node {
                 }
             
                 /**
            +     * This function gives local plugins an opportunity to modify the settings navigation.
            +     */
            +    protected function load_local_plugin_settings() {
            +        // Get all local plugins with an extend_settings_navigation function in their lib.php file
            +        foreach (get_plugin_list_with_function('local', 'extend_settings_navigation') as $function) {
            +            // Call each function providing this (the settings navigation) and the current context.
            +            call_user_func($function, $this, $this->context);
            +        }
            +    }
            +
            +    /**
                  * This function marks the cache as volatile so it is cleared during shutdown
                  */
                 public function clear_cache() {
             

            Really the only difference is that the callback will be local_pluginname_extend_settings_navigation.

            Jonathon are you happy with that approach?

            If so then I think we can still look at getting this in today.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi guys, I'm sending this back presently sorry because there are a couple of things to change/consider. All new callbacks should start with the plugin component. e.g local_plugin_extend_settings_navigation. The only exeception is mod in some circumstances. There is a function especially for finding all plugins with a particular function and a particular file (by default lib.php). I had a quick look at this and the following should work:   diff --git a/lib/navigationlib.php b/lib/navigationlib.php index eaf5218..b8beba7 100644 --- a/lib/navigationlib.php +++ b/lib/navigationlib.php @@ -3308,6 +3308,9 @@ class settings_navigation extends navigation_node { $this->add(get_string('returntooriginaluser', 'moodle', fullname($realuser, true)), $url, self::TYPE_SETTING, null, null, new pix_icon('t/left', '')); } + // At this point we give any local plugins the ability to extend/tinker with the navigation settings. + $this->load_local_plugin_settings(); + foreach ($this->children as $key=>$node) { if ($node->nodetype != self::NODETYPE_BRANCH || $node->children->count()===0) { $node->remove(); @@ -4393,6 +4396,17 @@ class settings_navigation extends navigation_node { } /** + * This function gives local plugins an opportunity to modify the settings navigation. + */ + protected function load_local_plugin_settings() { + // Get all local plugins with an extend_settings_navigation function in their lib.php file + foreach (get_plugin_list_with_function('local', 'extend_settings_navigation') as $function) { + // Call each function providing this (the settings navigation) and the current context. + call_user_func($function, $this, $this->context); + } + } + + /** * This function marks the cache as volatile so it is cleared during shutdown */ public function clear_cache() {   Really the only difference is that the callback will be local_pluginname_extend_settings_navigation. Jonathon are you happy with that approach? If so then I think we can still look at getting this in today. Cheers Sam
            Hide
            jonof Jonathon Fowler added a comment -

            Sounds perfect to me. I can have updated branches for you in a little over an hour's time.

            Cheers

            Jonathon

            Show
            jonof Jonathon Fowler added a comment - Sounds perfect to me. I can have updated branches for you in a little over an hour's time. Cheers Jonathon
            Hide
            jonof Jonathon Fowler added a comment -

            Branches updated with Sam's suggestion, updated test plugin attached (mdl30506tester-r2.zip).

            Show
            jonof Jonathon Fowler added a comment - Branches updated with Sam's suggestion, updated test plugin attached (mdl30506tester-r2.zip).
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Awesome thanks Jonathon, I'll give everything a test a little bit later on today and get this in.
            There was one other thing I forgot to mention earlier, as this is an improvement/new feature it will only land on master initially.
            Then if there is a good reason to do so we can look at backporting it. Either way I'll take care of all of that.

            Show
            samhemelryk Sam Hemelryk added a comment - Awesome thanks Jonathon, I'll give everything a test a little bit later on today and get this in. There was one other thing I forgot to mention earlier, as this is an improvement/new feature it will only land on master initially. Then if there is a good reason to do so we can look at backporting it. Either way I'll take care of all of that.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Jonathon, this has been integrated on master now.
            I did make one more commit during integration to document this callback within local/readme.txt and to change the callback back to local_pluginname_extends_settings_navigation in order to be more consistent with the current callback which I see has the extends.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Jonathon, this has been integrated on master now. I did make one more commit during integration to document this callback within local/readme.txt and to change the callback back to local_pluginname_extends_settings_navigation in order to be more consistent with the current callback which I see has the extends . Cheers Sam
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Tested during integration review

            Show
            samhemelryk Sam Hemelryk added a comment - Tested during integration review
            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
            Hide
            jleyva Juan Leyva added a comment -

            Just a quick note for versions prior to 2.3

            If you need the settings navigation object, and you are sure the $PAGE object is fully loaded you can do this:

            function local_myplugin_extends_navigation(global_navigation $navigation) {
            global $PAGE;

            if (!$settingsnav = $PAGE->__get('settingsnav'))

            { return; }
            Show
            jleyva Juan Leyva added a comment - Just a quick note for versions prior to 2.3 If you need the settings navigation object, and you are sure the $PAGE object is fully loaded you can do this: function local_myplugin_extends_navigation(global_navigation $navigation) { global $PAGE; if (!$settingsnav = $PAGE->__get('settingsnav')) { return; }

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12