upgrade.txt

This file describes API changes for the local_olximport plugin, information here
is intended especially for developers.

=== 0.1.0 (2026-08-25 fix: missing mod_assign field broke openassessment/edx_sga on MySQL) ===

* Reported: a real large-course web import ended with "A course with shortname ... already
  exists" — a confusing error given the shortname had deliberately been freed up beforehand.
  Root cause was upstream of that message: `assign_defaults::settings()` (shared by
  `openassessment_converter` and `edx_sga_converter`) never set `gradingduedate`, a `NOTNULL`
  column on `mod_assign` with no working default on this MySQL install. The very first
  `openassessment` block in a course throws a `dml_write_exception` on the `assign` insert,
  which `add_moduleinfo()` (`course/modlib.php`) had already wrapped in its own delegated
  transaction — an exception escaping a Moodle-core transaction like that has to be allowed to
  propagate to Moodle's own top-level handler, or the transaction is left open for the rest of
  the request. This plugin's `execute()` was catching it internally instead
  (`catch (\Throwable $e) { $run->mark_failed(...); }`), so nothing rolled it back; Moodle's own
  adhoc-task runner then caught the resulting `"Task left transaction open"` coding error one
  level up and force-rolled-back everything still pending — silently discarding this plugin's own
  `mark_failed()` write along with the real error, and leaving the run looking like it was still
  "running". Course sections/modules created *before* the openassessment block (each finalised
  through its own already-closed nested transaction) survived the rollback, so the course existed
  but stopped partway through (44 of what should have been 101 modules). Moodle's task queue then
  auto-retried the same adhoc task (standard behaviour for a failed task); the retry redid the
  whole import from scratch and immediately collided with the shortname its own first attempt had
  already taken — which is the error actually seen. Fixed with the one missing field:
  `assign_defaults::settings()` now sets `'gradingduedate' => 0`, matching
  `\mod_assign\testing\generator`'s own default (the same reference this file already mirrors for
  every other field) — confirmed against `mod/assign/db/install.xml`
  (`NOTNULL="true" DEFAULT="0"`), which was never actually reachable through
  `$DB->insert_record()` without the caller supplying it explicitly.
* Verified two ways: (1) a direct instrumented reproduction against the real archive that
  originally failed, checking `$DB->is_transaction_started()` before/after
  `course_importer::import()` — open before the fix, clean after; (2) re-running the *exact* run
  through the real `import_course_task::execute()` path end-to-end, producing a genuinely
  complete course (7 sections, 101 modules, 12 quizzes, 3 assignments) with no leaked transaction.
* Also hardened `import_course_task::execute()`'s inner `catch (\Throwable $e)` so this class of
  bug can't recur under a *different* trigger: it now checks `$DB->is_transaction_started()` and
  calls `$DB->force_transaction_rollback()` before `mark_failed()`, for any exception, not just
  this one field. Without it, any other unexpected exception thrown from inside a transactional
  core call (a different missing/invalid field on some other module type, a DB constraint this
  plugin hasn't hit yet) would reproduce the exact same cascade — mark_failed()'s own write
  silently discarded by core's later force-rollback, run left looking merely "still running",
  and an auto-retried task colliding with its own first attempt's partial course. Verified by
  temporarily re-removing the `gradingduedate` fix above and re-running the real `execute()`
  path: previously this produced the misleading stuck-then-"shortname exists" cascade; with the
  rollback in place it instead cleanly reports `status=failed` with the real underlying error
  message and no leaked transaction, on the first attempt.

=== 0.1.0 (2026-08-25 fix for a real reported bug: overlapping cron on a live site) ===

* Reported: a large web upload's import report ended "Failed — the uploaded file is no longer
  available", but the course showed up partially imported anyway. Root cause: the reporting
  site's cron trigger fires overlapping `admin/cli/cron.php` processes with no mutex between
  them, so two processes could claim and run the *same* queued import at once — one completing
  it for real, the other finding the first one's already-consumed archive file gone and
  overwriting the real "complete" result with a bogus "failed" one (last write wins on the same
  `local_olximport_run` row). `import_course_task::execute()` now takes an explicit, non-blocking
  lock keyed on the run id (`\core\lock\lock_config::get_lock_factory('local_olximport')`)
  before touching anything; a second process that can't get the lock is a silent no-op instead
  of a data-clobbering race. The archive is now only deleted after a confirmed
  `mark_complete()`, not unconditionally, so a process that dies before that point leaves a real,
  re-importable file behind instead of poisoning every future attempt on the same run.
* Follow-on bug found while verifying the above: with only a `STATUS_QUEUED` guard, a run whose
  sole process is hard-killed (crash, OOM, `kill -9`) after `mark_running()` but before
  `mark_complete()`/`mark_failed()` got stuck at `STATUS_RUNNING` forever — nothing would ever
  retry it, since the guard rejected every status except `STATUS_QUEUED`, and Moodle's own task
  queue removes the adhoc task row as soon as `execute()` returns normally (which it always does
  here, every failure path being caught internally), regardless of what actually happened. Fixed
  by recognising that a `STATUS_RUNNING` row encountered *while this process holds that same run's
  lock* can only be stale: any lock factory (DB advisory lock, file lock) releases automatically
  when its holding process dies, so a still-genuinely-running holder would still own the lock and
  this process would never have gotten past `get_lock()` in the first place. `execute()` now
  treats `STATUS_RUNNING` the same as `STATUS_QUEUED` (only `STATUS_COMPLETE`/`STATUS_FAILED`
  block a retry) — a self-healing recovery with no time-based threshold needed.
* Both fixes verified with real, separate OS processes (not just PHPUnit — a single PHPUnit run
  keeps one DB connection for its whole duration, which makes DB-advisory-lock tests like these
  pass trivially for the wrong reason): one process holding the lock correctly blocks a second
  from doing any work; a process that self-SIGKILLs right after `mark_running()` (simulating a
  real crash) leaves a run a later, independent process then successfully recovers and completes.
  Confirmed against both lock factories this plugin will actually run under in practice —
  Postgres (`postgres_lock_factory`, Totara's dev environment) and MySQL
  (`mysql_lock_factory`, the reporting site) — both documented in core as
  connection-scoped/auto-releasing, which is what the recovery logic depends on.

=== 0.1.0 (2026-08-24 fixes found testing against real vanilla Moodle) ===

* `classes/compat.php`'s Moodle-branch methods (`admin_page_setup()`, `external_page()`,
  `admin_category()`) now `require_once($CFG->libdir . '/adminlib.php')` before touching
  `admin_externalpage`/`admin_category`/`admin_externalpage_setup()`. Those are legacy globals,
  not autoloaded — nothing in a normal Moodle request pulls that file in on its own. Every
  Moodle-branch caller fataled with a class/function-not-found error before this fix; confirmed
  against a real Moodle 4.2 site (`class_exists('\admin_externalpage')` is false immediately
  after `require(config.php)`, only true once something has actually required this file).
* `create_module()` returns the *submitted* moduleinfo object, not a fresh `course_modules` row
  — the real cmid comes back on `->coursemodule` (set by `add_moduleinfo()`,
  `course/modlib.php`), not `->id`. Every converter that calls `create_module()`
  (`chunk_page_builder`, `quiz_builder`, `lti_converter`, `discussion_converter`,
  `edx_sga_converter`, `openassessment_converter`) was reading `->id` instead — on Totara this
  happened to not error, so it went unnoticed through months of testing there; on real vanilla
  Moodle it threw `dml_missing_record_exception` ("Invalid course module ID") the moment
  anything looked that id up in `course_modules` (the very next line, in every one of these
  call sites). Fixed to read `->coursemodule` throughout. `->instance` (used separately for the
  quiz's own row) was already correct — `add_moduleinfo()` sets that one explicitly.
* Both fixes confirmed end-to-end against a real Moodle 4.2 instance: admin page renders, form
  submits, adhoc task completes, and the resulting quiz is attemptable with the imported
  question rendering and gradeable correctly.
* Known non-blocking follow-up, not fixed here: `cli/import.php`'s `cron_setup_user()` and
  `quiz_builder::finalize()`'s `quiz_update_sumgrades()` both log `DEBUG_DEVELOPER` deprecation
  notices on Moodle 4.2 (replaced by `\core\cron::setup_user()`/`reset_user_cache()` and
  `grade_calculator::recompute_quiz_sumgrades()` respectively) — functionally fine today on
  every version this plugin claims to support, but worth swapping to the non-deprecated form
  next time this file is touched, the same way `classes/compat.php` already prefers the
  non-deprecated path on each platform where one exists.

=== 0.1.0 (2026-08-24 Moodle portability) ===

* This plugin now runs unmodified on vanilla Moodle 4.1+ as well as Totara 20 — previously it
  depended on four Totara-only subsystems with no Moodle equivalent under the same name.
  `$plugin->requires` is now `2022112800` (Moodle 4.1 LTS; comfortably below Totara 20's own
  core version, so one number satisfies the install check on either platform). Do not raise the
  effective floor to Moodle 5.0: `quiz_add_quiz_question()`/`quiz_add_random_questions()`, which
  `quiz_builder` depends on, are removed there.
  * `totara_mvc` controllers are gone. `index.php`/`view.php` are now plain procedural admin
    pages (`compat::admin_page_setup()` + `$OUTPUT->header()`/`render_from_template()`/`footer()`),
    the pattern already used elsewhere in this codebase for non-Totara-MVC admin pages (e.g.
    `admin/tool/usagedata/index.php`). `classes/controller/` is deleted. `import_form` no longer
    implements `\totara_mvc\viewable`; the import_page template's `form` value is now a
    pre-rendered string (`$form->render()`).
  * The CSS-registration hook (`db/hooks.php`, `classes/watcher/output_watcher.php`, both
    deleted) turned out to be entirely redundant on *both* platforms: core's own
    `theme_config::get_css_files()` already auto-loads every plugin's root `styles.css` into the
    theme bundle with no registration code required. Confirmed empirically — `.olx-content`
    rules are present in the served theme CSS with the hook removed.
  * `classes/entity/` (Totara's `core\orm` entity layer) is deleted. `classes/model/import_run.php`
    is now a plain class wrapping `$DB` calls against `local_olximport_run` directly, keeping the
    exact same public method names/constants so callers didn't need to change. One real
    regression risk in this rewrite: `timecreated` has no DB default and must be set explicitly
    in `create()` — the ORM did this implicitly.
  * `settings.php` and the two `\coursecat` call sites (`import_form.php`, `course_importer.php`)
    now go through the three places no single class name exists on both platforms:
    `admin_externalpage`/`admin_category` (Moodle) vs. Totara's `core\setting\*` renames, and
    `\core_course_category` (Moodle) vs. Totara's still-`\coursecat`. See the new
    `classes/compat.php` — four small `class_exists()`-gated static methods, the only
    feature-detected part of this whole port.
  * `question_importer::import()` now returns the imported questions' ids (`int[]`) instead of a
    count. It used to find them by counting `question` rows filtered by `category` before/after
    each import — Moodle 4.x's question-bank restructure moved `category` off that table
    entirely (`question_bank_entries`/`question_versions`), which would have made that query
    silently return nothing there. ids now come from `$qformat->questionids`
    (`question/format.php`), a plain array untouched by that restructure, read only when
    `importprocess()` itself reports success (a later validation failure can roll back an insert
    while leaving its id in that array regardless — see the method's docblock).
    `course_importer::flush_problem_run()` uses the returned ids directly instead of re-querying.
  * PHPUnit tests now extend `\local_olximport\testing\testcase` (a `class_alias()` shim over
    `core_phpunit\testcase` on Totara / `advanced_testcase` on Moodle) instead of
    `core_phpunit\testcase` directly. The behat feature drops the Totara-only
    `Given I am on a totara site` step and swaps `I run the adhoc scheduled tasks "..."` for the
    portable `I run all adhoc tasks`.

=== 0.1.0 (2026-08-24 quality-of-life additions) ===

* The import form (and CLI, for parity) now takes a course full name override, course format,
  and visibility, alongside the existing shortname/category overrides — `course_importer::import()`'s
  `$options` array grew `fullname`, `format` and `visible` keys accordingly. Format defaults to
  'topics' (what this importer's chapter/section structure is built around) but any enabled
  format is selectable; visibility defaults to visible, matching create_course()'s own default.
  CLI equivalents: `--fullname=NAME`, `--format=FORMAT`, `--hidden`.
* Quiz names now end in "Quiz" (e.g. "Basic Assessment Tools Quiz") — they otherwise shared their
  sequential's plain name with the page/label sitting right next to them in the course nav, with
  nothing but the activity icon to tell them apart at a glance.

=== 0.1.0 (2026-08-24 further fixes) ===

* Internal links (jump targets, quiz/page/forum/lti/assign URLs) are now stored root-relative
  (`out_as_local_url()`) instead of absolute (`out(false)`). The old absolute form baked in
  whatever `$CFG->wwwroot` the import happened to run under — typically different from the
  wwwroot a CLI import resolves to versus the one a browser request resolves to for the same
  site, since wwwroot is computed per-request. Root-relative links resolve correctly regardless
  of which host/path the course is later viewed under.
* Quizzes could never be attempted or previewed: `quiz_builder` never called
  `quiz_update_sumgrades()` after adding questions, so every quiz's `sumgrades` column stayed at
  its creation-time 0 forever while `grade` stayed at the default 100 — a mismatch Totara refuses
  to let anyone attempt ("cannotstartgradesmismatch"). `quiz_builder::finalize()` now recomputes
  `sumgrades` from the actual slots, and zeroes `grade` too on a quiz that legitimately ends up
  with nothing gradable in it (e.g. every problem in that run turned out to be an unsupported
  response type, imported as a non-gradable `description` placeholder).
* edX's "rate this page" feedback widget (`id="feedback-container"`, `sendFeedbackToAPI(...)`) —
  authoring boilerplate appended after most units in the reference export, non-functional in
  Totara regardless (the function it calls is never defined, and it posts to an edX-only API) —
  is now stripped by `html_converter` the same way pure `<script>`/`<style>` content already was,
  instead of counting as real page content. A vertical containing nothing else no longer produces
  an empty page holding just this widget.
* `problem`/`library_content` blocks are no longer flattened into one quiz stuck at the very end
  of their sequential. `problem_run` now tracks the *current contiguous run* of them, and
  `course_importer::flush_problem_run()` turns each run into its own quiz (and question category)
  positioned right where that run occurred — as soon as a page or any other activity breaks the
  run, and again for whatever's left at the end of the sequential. A sequential that alternates
  problem/html/problem/... throughout now gets several small quizzes, each next to the material
  it tests, rather than one quiz with every question in the sequential dumped at the end of it —
  closer to how the source course paced problems against instructional content. The common case
  (all a sequential's problems clustered together, nothing splitting them) is unaffected: still
  one quiz, still named after the sequential.

=== 0.1.0 (2026-08-24 fixes) ===

* Fixed four rendering problems found after importing the reference course, all caused by
  Totara's HTML sanitiser (HTMLPurifier) silently stripping things a naive HTML port relies on:
  * Videos render as a real player again: the OLX video block used to emit a raw `<iframe>`,
    which HTMLPurifier always strips (Totara has no config path that allows it, "noclean" or
    otherwise). It now emits an `<a href="..." data-embed="1">` link, the one attribute
    HTMLPurifier does whitelist for this, which the `mediaplugin` filter turns into a real
    player at render time — the same path any other YouTube link in Moodle/Totara goes through.
  * Images no longer overflow the page and force horizontal scroll. `.generalbox` (what
    mod_page wraps content in) has no width-constraining CSS of its own, and a percentage-based
    `max-width` in an inline `style` attribute gets silently dropped by HTMLPurifier's img
    length validator — so this had to be fixed with real CSS, not inline styles: see
    `styles.css`, loaded automatically by core's own theme CSS aggregation (no registration
    code of this plugin's own needed) and scoped to `.olx-content` (the wrapper
    `chunk_page_builder` now puts around every mod_page).
  * The "click to reveal" accordion pattern used throughout the reference export
    (`div.hint-wrapper`/`hint-title`/`hint-body`, toggled by `onclick` + a `<script>`-defined
    function) is rewritten by `classes/convert/block/click_to_reveal_rewriter.php` into a
    native `<details>`/`<summary>` element — the only way to keep this pattern interactive,
    since `<script>` tags and `on*` attributes are always stripped, unconditionally, with no
    config flag to allow them.
  * A missing local image referenced from "The Open edX Video Player" page turned out to be a
    gap in the source export itself (`Video_Player_Elements_Image-100.png` isn't present under
    `course/static/` in the reference tarball) — already correctly reported by the importer as
    "static asset not found, link left broken" in the run's import log, not a plugin bug.
* Custom per-course CSS/JS authored directly in OLX `html` blocks (`<link>`/`<style>`/`<script>`
  tags) is not portable to Totara and is dropped for the same reason the accordion pattern's
  script was — `styles.css` only covers a small, known set of edX authoring classes
  (`.best-practice`, `.blue-text`) taken from the reference export's own style sheet.

=== 0.1.0 ===

* Initial release.
  * CLI: `php local/olximport/cli/import.php --file=<archive.tar.gz> [--dry-run] [--shortname=]`.
  * Admin UI: Site administration > Courses > Open edX OLX course import.
  * Converts course/chapter/sequential/vertical structure, html/video page content
    (with static assets and internal `/jump_to_id/` links resolved), problem blocks
    (multichoice, shortanswer, numerical, with unsupported response types imported
    as non-gradable description questions) assembled into quizzes for graded
    sequentials, and lti/discussion/openassessment/edx_sga blocks mapped to
    mod_lti/mod_forum/mod_assign (with a rubric for openassessment).
  * Not yet converted: poll, survey, annotatable, wiki, drag-and-drop-v2, and
    multi-dropdown (cloze/multianswer) problems — logged in the import report
    rather than silently dropped.