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

          Attachments

            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