Reduce the styles admin page to a pure action endpoint (UX-01, DEC-0067) - #110
Merged
Conversation
Styles were managed in two places at once: the plugin settings page, where the three widgets work in full (the upload control sits inside the settings form and installs on save), and admin/styles.php, a visible admin-menu page that re-rendered the same widgets bare. The duplicate was a trap — its upload control had no form or save button, so a dropped ZIP looked accepted, was never installed and vanished on reload without a message. That is finding UX-01 of the external validation of release 4.0.2, whose reviewers hit it themselves. The file cannot simply be deleted: the per-row enable/disable/delete buttons (including the ones on the settings page) are sesskey-protected GET links pointing here, because a form nested inside the settings-page form is invalid HTML and used to break the whole settings save. Keep exactly that role and nothing else: process the action, keep the server-side delete confirmation (the only screen it renders), and redirect every outcome — including direct visits — back to the settings page. The admin_externalpage registration stays (admin_externalpage_setup() resolves it) but hidden from the menu, and the docblock warns against growing a UI here again. UX-01 is resolved by removing the surface (the report's option b): one place to manage styles, where everything already worked. README and the user guide no longer advertise a dedicated page. The new test pins the contract: the page stays registered but hidden.
…action-endpoint # Conflicts: # README.md # research/docs/indices/adrs.yaml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #110 +/- ##
=========================================
Coverage 92.31% 92.31%
Complexity 824 824
=========================================
Files 51 51
Lines 3734 3734
=========================================
Hits 3447 3447
Misses 287 287
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Resolves finding UX-01 of the external validation of release 4.0.2 by removing the surface it lived on:
admin/styles.phpstops being a visible management page and becomes a pure action endpoint. Styles are now managed in exactly one place — the plugin settings page, where everything already worked.The problem
Styles were managed in two places at once. The settings page renders the three widgets inside the settings form, so the upload control installs on "Save changes".
admin/styles.phpre-rendered the same widgets bare, viaoutput_html('')— and its upload control had no form and no save button: a dropped ZIP looked accepted, was never installed, and vanished on reload without a message. The validation reviewers hit this themselves during testing.Why the file survives at all
The per-row enable/disable/delete buttons — including the ones rendered on the settings page — are sesskey-protected GET links pointing at
admin/styles.php, because a<form>nested inside the settings-page form is invalid HTML and used to break the whole settings save (fixed back in the audit round by turning the buttons into links). Those links need a handler outside the settings form, and this is it.Changes
admin/styles.phpkeeps only the action processing and the server-side delete confirmation (the one screen it still renders — a GET delete must not fire on a prefetch); every outcome, including a direct visit with no action, redirects to the settings page.admin_externalpageregistration stays —admin_externalpage_setup()resolves it — but hidden from the admin menu.Action-link URLs are unchanged, so nothing migrates; a bookmarked
admin/styles.phplands on the settings page instead of erroring.Tests
Written first and watched failing: the styles admin page must remain registered (an
admin_externalpage) but hidden. The existing render test — action buttons emitted as links intostyles.php, no nested form — still passes unchanged: 3/3 locally (Moodle 5.0.7 / PHP 8.3 / MariaDB),phpcs --standard=moodle0/0 on every touched file.Moodle Playground Preview
The changes in this pull request can be previewed and tested using a Moodle Playground instance.
ℹ️ The eXeLearning editor is fetched from the shared release and unpacked into the plugin when the playground boots, so the first load may take a few extra seconds. ELPX upload, viewer and preview work normally.