Fix for Moodle 4.1 [#306]

[sarahjcotton]

Fixes #306

[danmarsden]

thanks @sarahjcotton - calling set_config inside settings.php like this feels very wrong to me...

Do you have the debugging warnings the plugin is throwing that you can add to the issue to show where the errors are being triggered?

I'm guessing the default values aren't set on install due to the is_enabled_auth('outage')) call on this line: https://github.com/catalyst/moodle-auth_outage/blob/MOODLE_39_STABLE/settings.php#L34

I wonder if that's actually needed and if we can remove that instead?

[brendanheywood]

There is already code to read config items and apply default values if missing, if there is code directly reading config it should use this instead:

https://github.com/catalyst/moodle-auth_outage/blob/12f43447b767e955741ac57193f2ca606d2b265d/classes/local/outagelib.php#L160-L195

[sarahjcotton]

@danmarsden Yes, that's exactly why the errors are thrown initially after install, however defaults are set when the settings page is saved as per @brendanheywood comment.

Mark J advised on this one but happy to fix however you see fit... just let me know your preferred approach.

[brendanheywood]

My preference is to not set the config indirectly and to just back fill the data if it happens to be missing using the local function get_config() function. If any code is using core get_config directly then it should be swapped to use the local on instead

[sarahjcotton]

I've done some testing and the defaults are not being set on install due to ($hassiteconfig && is_enabled_auth('outage')) { on the settings page. As defaults are only set on plugin install these never get set, including auth_outage|remove_selectors, auth_outage | default_title, auth_outage | default_description, auth_outage | remove_selectors although these don't throw any errors.

Mark had the same issue here which is related to admin_setting_configduration : https://github.com/catalyst/moodle-tool_objectfs/issues/530

We discussed the use of the plugin's get_config() but couldn't see how that would help in this instance.

As Dan mentioned, another solution would be to remove the plugin enabled check and let the plugin load the default settings on install, perhaps removing auth_outage|default_autostart if that's deemed to be an issue, although if the plugin is installed disabled this shouldn't pose a problem anyway; I've left the removal of this setting on install in place for now.

I've submitted a changed to reflect this approach for your consideration.

A message is shown in the config settings if the plugin isn't enabled; I've also moved this to the top of the settings so it's easily visible instead of being half way down the page.

[sarahjcotton]

Also updated ci.yaml, README badge and bumped version.

[danmarsden]

Thanks @sarahjcotton - I like this approach a lot better - although I'm not quite sure what the issue is with default_autostart being set on plugin install? - (but I haven't gone looking at the code to remind me...) - can you add a comment to point me in the right direction there?

[sarahjcotton]

I don't think it would be an issue to have that setting shown as you still have to enable the outage in a separate page even once the settings have been configured.

I think my train of thought around being careful with that option is that the only way to take it out of maintenance mode again is via cli which, from previous roles I've had, could potentially only be performed by someone in another department. So I think perhaps that could be made clearer? Happy to add that to the README/lang for the setting if need be.

[sarahjcotton]

Removed the check around auth_outage|default_autostart please let me know if you need me to do anything else.

[danmarsden]

looks ok to me - although I do see you've removed the $hassiteconfig test along with the is_enabled('outage') part - was that intentional? - I don't remember off the top of my head the specific reasons where hassiteconfig or admin->fulltree checks in that file are needed/used.

[sarahjcotton]

looks ok to me - although I do see you've removed the $hassiteconfig test along with the is_enabled('outage') part - was that intentional? - I don't remember off the top of my head the specific reasons where hassiteconfig or admin->fulltree checks in that file are needed/used.

Thanks for spotting that, I've added the check back in.

[danmarsden]

Thanks @sarahjcotton lgtm - merged.