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

Calendar plugins can not display setting pages.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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 Master Branch:
      wip-MDL-43008-master
    • Sprint:
      BACKEND Sprint 7
    • 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

          Issue Links

            Activity

            Hide
            samhemelryk 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
            samhemelryk 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
            abgreeve 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
            abgreeve 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 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 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
            abgreeve 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
            abgreeve 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 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 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
            abgreeve Adrian Greeve added a comment -

            Oh!

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

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

            Thanks Adrian,

            Looks fine now.

            Integrated to 26 and master.

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

            It passes. Tested in integration master

            Show
            dmonllao David Monllaó added a comment - It passes. Tested in integration master
            Hide
            poltawski 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
            poltawski 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
            abgreeve Adrian Greeve added a comment -

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

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

            This issue caused a regression MDL-52332
            Function core\plugininfo\calendartype::load_settings() has a wrong definition but it did not show up before PHP7 tests because it is never used

            Show
            marina Marina Glancy added a comment - This issue caused a regression MDL-52332 Function core\plugininfo\calendartype::load_settings() has a wrong definition but it did not show up before PHP7 tests because it is never used

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  13/Jan/14

                  Agile