#3633710: Address review feedback provided on initial LMS MR.

Marlene Wanbergrequested to merge
issue/lms_focus_mode-3633710:3633710-address-review-feedback into 1.0.x

Closes #3633710

This MR addresses the feedback provided for the original Focus Mode module draft idea.

I used AI to assist with the coding, tests and documentation.

Before testing locally:

  1. Requires LMS 1.2.4 or later
  2. drush updb if you had it installed already (the MR contains some update hooks)

Things that are different from what the review requested (as summarized by Claude):

  • Project files. composer.json, .gitlab-ci.yml, phpcs.xml and phpstan.neon are kept, since focus mode is its own project.
  • Alter hook. Building the start screen in the answer form needs an alter point in LmsLessonHandlerBase::getAnswerForm(), which lms doesn't have yet. Until then, the start screen needs to stay a placeholder, and the mode needs to be resolved twice per page.
  • Course status lookup. The lookup is still needed: it gives the lesson revision the student is pinned to, which the route doesn't. The current-activity check was removed.
  • "Started"/browser storage. The server flag is now used for every activity type except xAPI. lms_xapi saves the answer when the activity is viewed, so xAPI needs a browser record, keyed by the answer ID. If lms_xapi could report whether a student has started, the browser record wouldn't be needed. Without either, xAPI in Required mode would show "Resume activity" before the student ever starts, using the server flag, or always "Start activity", using no record.
  • Uninstall hook. It's still needed: base fields also leave field_deleted_data_* tables on uninstall. It now empties both the base field columns and the course field's tables.
  • Toggle position default. The default is now in install config and in the Twig |default, because core doesn't apply component prop defaults at render time.
  • isActive(). It reads the <html> class rather than the store. The store records focus mode per course, and the class per page. They differ on a Required activity that hasn't been started.
  • Update path. The stored values couldn't be renamed before data existed, because sites testing the first MR already have data. An update hook and a post-update now migrate it, so testers need to run drush updb after updating.
  • Browser test fixtures. The new browser tests build their own small course. LmsTestHelperTrait::createTestContent() imports lms's whole test data set, which these tests don't need.

Merge request reports