Moodle
  1. Moodle
  2. MDL-30506

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

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      33199

      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.

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Sam Hemelryk added a comment -

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

          Show
          Sam Hemelryk added a comment - Putting this up for peer-review (thanks for the branches Jonathon)
          Hide
          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
          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
          Michael de Raadt added a comment -

          Assigning Raj as peer reviewer and Sam as integrator.

          Show
          Michael de Raadt added a comment - Assigning Raj as peer reviewer and Sam as integrator.
          Hide
          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
          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
          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
          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
          Jonathon Fowler added a comment -

          Acknowledged. I'll see what I can do.

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

          Thanks Jonathon

          Show
          Rajesh Taneja added a comment - Thanks Jonathon
          Hide
          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
          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
          Rajesh Taneja added a comment -

          Thanks Jonathon,
          Pushing it for integration review

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

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

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

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

          Show
          Jonathon Fowler added a comment - Branches updated with Sam's suggestion, updated test plugin attached (mdl30506tester-r2.zip).
          Hide
          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
          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
          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
          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
          Sam Hemelryk added a comment -

          Tested during integration review

          Show
          Sam Hemelryk added a comment - Tested during integration review
          Hide
          Eloy Lafuente (stronk7) added a comment -

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

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

          Many thanks for your collaboration!

          Ciao

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