Feature: Change AI course assistance placement button location, resolves #894

[larsbonczek]

• Added a new "AI" tab on the "Feel" settings page
• Added a setting theme_boost_union/aiplacementcourseassistlocation to control the location of the AI button
• Added styles to remove padding and disable text wrapping on the AI button when it's a header action
• Code is designed to be future-proof (works with upcoming changes to aiplacement_courseassist in Moodle 5.0)
  The styles mentioned above will break in Moodle 5.0 :(
• Added Behat tests

[larsbonczek]

Note that this uses the function theme_boost_union_manipulate_hooks() to prevent the AI button from being added in the default location when its location has been changed in the Boost Union settings. However, as is mentioned here, this function is executed regardless of whether Boost Union is selected as the current theme or not. This means that if Boost Union is NOT selected as the current theme and the new setting to change the AI button location is set to "headeraction", then the AI button will NOT be added in the default location (because the aiplacement_courseassist hook is overriden in theme_boost_union_manipulate_hooks()) and it will also NOT be added is the new location (because Boost Union is not selected as the current theme). This means that the button disappears completely.

To prevent this, I suggest overriding the aiplacement_courseassist hook with a custom callback (instead of disabling it) which checks whether Boost Union is the current theme and calls the original aiplacement_courseassist callback if it is not. Do you agree with this solution @abias? I'm not sure where to place this new callback. It doesn't make sense to me to put it in the logical place for hook callbacks (i.e., classes/local/hook/output/after_http_headers.php) because it's not a real hook callback registered in the db/hooks.php file. It's only called if the Boost Union setting is set to "headeraction", after all. What about classes/local/hook_override/aiplacement_courseassist/hook_callbacks.php?

[larsbonczek]

I have updated the changes for Moodle 5.0 and fixed the problem outlined above. The solution looks a bit different than proposed initially because it is not actually possible to override a hook with a custom callback using $CFG->hooks_callback_overrides. You can only disable it or change its priority. I therefore added Boost Union's own callback for the after_http_headers hook in classes/local/hook/output/after_http_headers.php, which calls aiplacement_courseassist's hook if Boost Union has disabled that hook but is not currently active.

[larsbonczek]

The failed Behat tests don't seem to be related to my changes, if I'm not mistaken.

[abias]

Thank you @larsbonczek for working on this improvement.
We will review it and consider it for integration soon.

The failed Behat tests don't seem to be related to my changes, if I'm not mistaken.

No, they are not related. I am working on these failures in #973

[christianwolters]

Review

Following https://github.com/moodle-an-hochschulen/moodle-plugin-maintaining/wiki/Check-list-for-peer-reviewing-patches-and-pull-requests

Documentation

• Commit Message understandable and issue referenced (via resolves keyword)
• Patch author correct
• CHANGES.md --> appended in review
• README.md appended
• Credits appended
• Appropriate Comment quantity
• Successful Moodle PHPDoc check

version.php

• Checked if $plugin->version increment necessary (and increment done if necessary) -> incremented in review
• $plugin->release untouched

lib.php

• Only necessary functions

Languages

• Only english strings
• Necessary magic strings added (e.g. capabilities)

Automated tests

• New functionality covered
• No failing steps or scenarios

Mustache templates

• Not applicable Example context exists

CSS and styles.css

• Bootstrap styles used if possible

Duplicated code

• Duplicated code is marked properly

Github action

• All green

Commit history and scope

• Already single commit
• Focus on single purpose
• No surplus files

Additional aspects for Boost Union

• No usage of $theme->settings
• Usage of isset() checks when processing plugin settings in renderer / output code --> added in review
• Usage of admin_setting_configselect instead of admin_setting_configcheckbox
• Modified mustache templates properly marked and .upstream template in place

Review result

Hey @larsbonczek,

thank you for your contribution to Boost Union.

I made some minor changes, but the feature worked as advertised in my tests and review.

Cheers
Christian

[larsbonczek]

@christianwolters thank you for the review! The issue is not currently marked as approved because of your last force push. Could you maybe approve it again?