Moodle
  1. Moodle
  2. MDL-43008

Calendar plugins can not display setting pages.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.6.1
    • Component/s: Calendar
    • Labels:
    • Testing Instructions:
      Hide
      1. Navigate the Administration block and expand "Site administration", "Plugins". Check that there is no heading for "Calendar types".
      2. Install the following calendar plugin in the calendar/type directory.
        • git clone git@github.com:abgreeve/moodle-calendartype_japanese.git japanese
      3. Navigate back through the administration block and check that there is now a heading under the "plugins" section called "Calendar types" and under that is a link for the Japanese settings.
      4. Clicking the link will take you to a settings page for the japanese calendar type.
      5. Un-install the plugin.
      6. Make sure that the "Calendar types" section is now removed from the Administration block.
      Show
      Navigate the Administration block and expand "Site administration", "Plugins". Check that there is no heading for "Calendar types". Install the following calendar plugin in the calendar/type directory. git clone git@github.com:abgreeve/moodle-calendartype_japanese.git japanese Navigate back through the administration block and check that there is now a heading under the "plugins" section called "Calendar types" and under that is a link for the Japanese settings. Clicking the link will take you to a settings page for the japanese calendar type. Un-install the plugin. Make sure that the "Calendar types" section is now removed from the Administration block.
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull 2.6 Branch:
      wip-MDL-43008-26
    • Pull Master Branch:
      wip-MDL-43008-master
    • Story Points (Obsolete):
      10
    • Sprint:
      BACKEND Sprint 7

      Description

      The new calendar types don't currently allow the addition of settings pages.
      For the standard Gregorian calendar this may be fine, but other calendar types may require certain settings to be configurable.

        Gliffy Diagrams

          Activity

          Hide
          Sam Hemelryk added a comment -

          Hi Adrian,

          Code looks spot on thanks.
          I suppose we should consider this for backporting as well, I'm not entirely sure when we expect to see the first calendar types arriving with settings but perhaps as we've only just released 2.6 this would be a candidate for backport (it is down as a bug).

          I do have one suggestion, but it is only a suggestion.
          The core_component class has a method get_plugin_list_with_file which would be perfect for your needs here.

          Feel free to push to integration when you are ready.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Adrian, Code looks spot on thanks. I suppose we should consider this for backporting as well, I'm not entirely sure when we expect to see the first calendar types arriving with settings but perhaps as we've only just released 2.6 this would be a candidate for backport (it is down as a bug). I do have one suggestion, but it is only a suggestion. The core_component class has a method get_plugin_list_with_file which would be perfect for your needs here. Feel free to push to integration when you are ready. Cheers Sam
          Hide
          Adrian Greeve added a comment -

          Hello Sam,

          Thank you for the review. Yes core_component::get_plugin_list_with_file is exactly what I required.
          With the call to that method, I know that you can set the file to be automatically included, but I ran into trouble trying to get my settings to show properly when set to true.

          As for calendar types that will arrive with settings, my Japanese calendar type will be the first one. I want to allow the administrator to decide which year format to use when displaying the date and I have a feeling that there will be more settings to come in the future.

          I'll add a branch for 2.6 (When the multi-calendar support was introduced).

          Thanks,

          Show
          Adrian Greeve added a comment - Hello Sam, Thank you for the review. Yes core_component::get_plugin_list_with_file is exactly what I required. With the call to that method, I know that you can set the file to be automatically included, but I ran into trouble trying to get my settings to show properly when set to true. As for calendar types that will arrive with settings, my Japanese calendar type will be the first one. I want to allow the administrator to decide which year format to use when displaying the date and I have a feeling that there will be more settings to come in the future. I'll add a branch for 2.6 (When the multi-calendar support was introduced). Thanks,
          Hide
          Damyon Wiese added a comment -

          Noting that for 2.6.0 this won't be supported - so if you write a calendar plugin with a settings page - and the plugin is broken unless those settings are set - you will have to set the requires version of your plugin to a moodle version with this support added. This will need documenting in the dev docs and in lib/upgrade.txt.

          Adrian - can you please add this to the lib/upgrade.txt in this patch?

          Show
          Damyon Wiese added a comment - Noting that for 2.6.0 this won't be supported - so if you write a calendar plugin with a settings page - and the plugin is broken unless those settings are set - you will have to set the requires version of your plugin to a moodle version with this support added. This will need documenting in the dev docs and in lib/upgrade.txt. Adrian - can you please add this to the lib/upgrade.txt in this patch?
          Hide
          Adrian Greeve added a comment -

          Thanks Damyon,

          I've added documentation to lib/upgrade.txt and also updated http://docs.moodle.org/dev/Multiple_calendar_support#Settings_page with information about this fix.

          Show
          Adrian Greeve added a comment - Thanks Damyon, I've added documentation to lib/upgrade.txt and also updated http://docs.moodle.org/dev/Multiple_calendar_support#Settings_page with information about this fix.
          Hide
          Damyon Wiese added a comment - - edited

          Thanks Adrian,

          Looking again - this seems to be missing the required functions in lib/classes/plugininfo/calendartype.php to show the links to the settings from the plugin overview page.

          I copy/pasted this and it seemed to work - can you check/clean this and amend your branch?

          +++ lib/classes/plugininfo/calendartype.php
          +
          +    public function get_settings_section_name() {
          +        return 'calendartype_' . $this->name . '_settings';
          +    }
          +
          +    public function load_settings(part_of_admin_tree $adminroot, $parentnodename, $hassiteconfig) {
          +        global $CFG, $USER, $DB, $OUTPUT, $PAGE; // In case settings.php wants to refer to them.
          +        $ADMIN = $adminroot; // May be used in settings.php.
          +        $plugininfo = $this; // Also can be used inside settings.php.
          +        $qtype = $this;      // Also can be used inside settings.php.
          +
          +        if (!$this->is_installed_and_upgraded()) {
          +            return;
          +        }
          +
          +        $section = $this->get_settings_section_name();
          +
          +        $settings = null;
          +        $systemcontext = \context_system::instance();
          +        if (($hassiteconfig) &&
          +            file_exists($this->full_path('settings.php'))) {
          +            $settings = new admin_settingpage($section, $this->displayname,
          +                'moodle/site:config', $this->is_enabled() === false);
          +            include($this->full_path('settings.php')); // This may also set $settings to null.
          +        }
          +        if ($settings) {
          +            $ADMIN->add($parentnodename, $settings);
          +        }
          +    }
          

          Show
          Damyon Wiese added a comment - - edited Thanks Adrian, Looking again - this seems to be missing the required functions in lib/classes/plugininfo/calendartype.php to show the links to the settings from the plugin overview page. I copy/pasted this and it seemed to work - can you check/clean this and amend your branch? +++ lib/classes/plugininfo/calendartype.php + + public function get_settings_section_name() { + return 'calendartype_' . $this->name . '_settings'; + } + + public function load_settings(part_of_admin_tree $adminroot, $parentnodename, $hassiteconfig) { + global $CFG, $USER, $DB, $OUTPUT, $PAGE; // In case settings.php wants to refer to them. + $ADMIN = $adminroot; // May be used in settings.php. + $plugininfo = $this; // Also can be used inside settings.php. + $qtype = $this; // Also can be used inside settings.php. + + if (!$this->is_installed_and_upgraded()) { + return; + } + + $section = $this->get_settings_section_name(); + + $settings = null; + $systemcontext = \context_system::instance(); + if (($hassiteconfig) && + file_exists($this->full_path('settings.php'))) { + $settings = new admin_settingpage($section, $this->displayname, + 'moodle/site:config', $this->is_enabled() === false); + include($this->full_path('settings.php')); // This may also set $settings to null. + } + if ($settings) { + $ADMIN->add($parentnodename, $settings); + } + }
          Hide
          Adrian Greeve added a comment -

          Oh!

          Sorry about that. I've now included that code as well. Thanks for spotting that.

          Show
          Adrian Greeve added a comment - Oh! Sorry about that. I've now included that code as well. Thanks for spotting that.
          Hide
          Damyon Wiese added a comment -

          Thanks Adrian,

          Looks fine now.

          Integrated to 26 and master.

          Show
          Damyon Wiese added a comment - Thanks Adrian, Looks fine now. Integrated to 26 and master.
          Hide
          David Monllaó added a comment -

          It passes. Tested in integration master

          Show
          David Monllaó added a comment - It passes. Tested in integration master
          Hide
          Dan Poltawski added a comment -

          Congratulations, this change has now made its way upstream. Thanks for your contribution!

          “ Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live. ” - Rick Osborne

          Show
          Dan Poltawski added a comment - Congratulations, this change has now made its way upstream. Thanks for your contribution! “ Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live. ” - Rick Osborne
          Hide
          Adrian Greeve added a comment -

          As the comment above that I posted says. I have already included dev docs for this change.

          Show
          Adrian Greeve added a comment - As the comment above that I posted says. I have already included dev docs for this change.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Agile