Refactor 500 v2 - #759
Open
mchurchward wants to merge 264 commits into
Open
Conversation
mchurchward
force-pushed
the
REFACTOR_500_V2
branch
from
June 20, 2026 15:40
0552a7e to
73e7dfe
Compare
Replace raw \$DB calls in question.php and choice.php with the core\persistent-based record classes from classes/local/db/: - Constructor: question_record::get_record() replaces $DB->get_record() - get_choices(): choice_record::get_for_question() replaces $DB->get_records() - get_dependencies(): dependency_record::get_records() replaces $DB->get_records() - add(): new question_record()->save() replaces $DB->insert_record() - add_choice(): new choice_record()->save() replaces $DB->insert_record() - update_choice(): choice_record get+set+save replaces $DB->update_record() - add_dependency(): new dependency_record()->save() replaces $DB->insert_record() - update_dependency(): dependency_record get+set+save replaces $DB->update_record() - delete_dependency(): dependency_record->delete() replaces $DB->delete_records() - choice::create_from_id(): choice_record::get_record() replaces $DB->get_record() - choice::delete_from_db_by_id(): choice_record->delete() replaces $DB->delete_records() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove global $questionnaire from questionstart_survey_display() and thread it through the renderer → question call chain as an optional parameter - Move dependency HTML generation from inline PHP strings to a Mustache template (templates/dependencylist.mustache) rendered via $this->output - Fix sectiontext.php layering violation: replace require_once + new \questionnaire() with $this->questionnaire (set by question_output() before question_survey_display() is called); fall back gracefully when questionnaire is null Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- questionnaire.class.php: declare all DB-sourced columns (course, name, intro, introformat, qtype, respondenttype, respeligible, respview, notifications, opendate, closedate, resume, navigate, grade, sid, timemodified, completionsubmit, autonum, progressbar, removeafter) plus runtime properties ($survey, $cm, $context, $capabilities, $responses, $questionsbysec, $responsemanager, $rid, $usehtmleditor) - question.php: declare $typeid, $context, $responsetype, $qid, $resultid, $dependquestion, $dependchoice; rename $result_id → $resultid (no underscores) - questions_form.php: declare $moveq - section.php, feedback_section_form.php, behat_mod_questionnaire.php: remove attribute (all properties already declared) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add missing \$questionnaire = null parameter to match parent signature added in Phase 5. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add $sid, $dependquestionsand, $dependlogicand, $dependquestionsor, $dependlogicor to question class (no-underscore names per Moodle convention) - Update locallib.php to use the renamed no-underscore property names (form field names like 'dependquestions_and' in question.php are HTML element names and remain unchanged) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ponses from locallib All callers migrated to response\manager static or instance methods. questionnaire_delete_dependencies() also removed; callers use $DB directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- questionnaire_load_capabilities() and questionnaire_get_context() replaced by questionnaire::load_capabilities() instance method - questionnaire_get_survey_list() and questionnaire_get_survey_select() replaced by static questionnaire::get_survey_list/select() - settings_form.php now reads $questionnaire->capabilities directly - All four locallib functions removed Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
questionnaire::check_page_breaks() replaces the locallib function. All callers in questions.php (4x) and question.php (1x) updated. locallib function removed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ey to questionnaire class questionnaire_set_events() → \questionnaire::set_events() (static) questionnaire_delete_survey() → \questionnaire::delete_survey() (static) All callers in lib.php and locallib.php updated; functions removed from locallib.php. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
pagebreak::questionstart_survey_display() was missing ': stdClass' return type declaration and returning '' instead of new stdClass(), causing a fatal PHP error due to incompatibility with the parent method signature. test_user_time_daily_blocked_same_day used time() - 1800 which crosses midnight if CI runs between 00:00 and 00:30, making the submission appear to be yesterday. Changed to mktime(0,0,0,...) (today at midnight) which is always the same calendar day as time() and never a future timestamp. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Three get_user_responses_for_instance() calls had multiple arguments on one line in a multi-line call. Split each argument onto its own line. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Inside a namespaced file, bare 'stdClass' resolves to mod_questionnaire\local\question\stdClass. Must use \stdClass (global namespace) to match the parent method's return type. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
section.php: declare $sid and $sectionid (set by fbsections.php form data) questionnaire.class.php: declare $strquestionnaire and $strquestionnaires (set by complete.php for page heading strings) PHP 8.2 deprecates dynamic property creation; these must be declared. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The constructor copies all properties from the passed \$questionnaire object (DB record or form data) onto \$this. Form data includes standard Moodle fields like \$description that are not questionnaire properties, causing PHP 8.2 dynamic property deprecation warnings. Filter with property_exists() so only declared properties are set, preventing all future whack-a-mole dynamic property declarations. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Set by report.php and myreport.php after construction; must be declared to avoid PHP 8.2 dynamic property deprecation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
\$questionmap, \$choicemap, \$responsemap are set via \$this->{\$mapvar}
in add_data(). PHP 8.2 requires them to be declared explicitly.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
questionnaire_delete_instance() and questionnaire_reset_userdata() call \questionnaire::delete_survey() and \questionnaire::get_survey_list() (Phase 6d migrations) but were missing the require_once for the class file. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
These fields are set via the constructor object-copy loop (from Moodle's generator framework output and test helpers). The property_exists() guard introduced earlier silently dropped them; declaring them explicitly restores the prior behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ire.php Extracts survey CRUD logic from the legacy questionnaire.class.php monolith into static methods on the modern mod_questionnaire\questionnaire class. - Add update_survey(int $sid, stdClass $sdata): int|false — replaces survey_update(); removes $this->add_survey() side-effect; fixes the 'chart_type' → 'charttype' column name bug throughout - Add copy_survey(stdClass $survey, array $questions, int $owner): int|false — replaces survey_copy(); $this->survey and $this->questions become explicit parameters - Remove both methods from questionnaire.class.php - Update all 5 callers: lib.php (×2), qsettings.php, feedback.php, tests/generator/lib.php Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces all direct $DB calls in the two new static methods with the appropriate mod_questionnaire\local\db persistent classes: - update_survey(): survey_record::create()/update()/count_records() - copy_survey(): survey_record, question_record, choice_record, dependency_record, feedback_section_record, feedback_record Adds use statements for the four newly needed persistent classes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… class questionnaire_add_instance() and questionnaire_update_instance() in lib.php were orchestrating survey resolution, DB inserts, and event firing directly. They are now thin one-line delegates: - Add mod_questionnaire\questionnaire::add_instance(): resolves survey type (new/copy/public), loads questions via load_questions_for_survey() using the persistent layer, inserts questionnaire row via questionnaire_record, fires calendar and completion events - Add mod_questionnaire\questionnaire::update_instance(): updates survey realm via survey_record, updates questionnaire row via questionnaire_record, fires events - Move set_events() from legacy questionnaire.class.php to the new class; add_instance/update_instance call self::set_events() — no cross-class dep - Add private load_questions_for_survey(): replaces the legacy add_survey()/ add_questions() pair for the copy path, using question_record persistent - lib.php keeps questionnaire_grade_item_update() (Moodle gradebook API concern) and delegates everything else Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…duction - lib.php: guard ->id access in user_outline, user_complete, get_user_grades, and grade_item_update with instanceof checks; use ->instance ?? ->id for form-data stdClass passed to grade_item_update from update_instance - tests/generator/lib.php: pass addquestions=true so questionsbysec is populated when create_question_response calls response_insert; add TODO - tests/lib_test.php, responsetypes_test.php, csvexport_test.php: replace ->id / ->questions / ->course direct property access with ->id() / ->questions() / ->courseid() getter calls on new questionnaire class Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t fixes - questionnaire_record: add create_from_formdata() factory and before_create() hook - survey_record: add create_from_sdata() and update_from_sdata() factories - questionnaire.php: add $cmidnumber/$courseid properties; add intro(), grade(), add_questions() getters; refactor add_survey() to use survey_record factory - Tests: replace legacy ->id/->sid/->course/->cmid property access and old \questionnaire instantiation with new getter calls (->id(), ->surveyid(), ->courseid(), ->coursemodule()); use from_instanceid() factory where needed Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ionnaire.php - create_question_response(): one param per line, correct indentation - new \questionnaire() call: one arg per line - foreach array literals: PSR12 control structure spacing (expression on next line) - if condition in set_events: PSR12 control structure spacing Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
No callers exist; the function was added speculatively and is not needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…onnaire_get_completion_state() Both functions are deprecated in Moodle 4.x: - questionnaire_print_overview() removed in Moodle 4.0 (replaced by block_myoverview) - questionnaire_get_completion_state() deprecated in Moodle 3.11 (replaced by activity_custom_completion class) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e class Move get_user_grades(), grade_item_update(), update_grades() bodies into static methods on classes/questionnaire.php. lib.php retains thin one-line delegating wrappers to satisfy the Moodle API. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…rappers Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… stream_and_email Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- amd/src/attempt_form.js replaces M.mod_questionnaire.init_attempt_form - amd/src/sendmessage.js replaces M.mod_questionnaire.init_sendmessage - amd/src/printing.js replaces M.mod_questionnaire.init_printing - amd/src/slider.js replaces M.mod_questionnaire.init_slider PHP callers switched from js_init_call() to js_call_amd(): - show_nonrespondents.php, report.php, renderer.php::complete_formstart, slider.php (2 sites). Slider strings now registered explicitly via strings_for_js() instead of via the legacy js_module()['strings'] bag. module.js still gets required in complete_formstart while inline onclick= handlers in the rate/check/radio templates depend on the global other_check / other_rate_uncheck. Removed in the next commit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
New module mod_questionnaire/survey_inputs installs click delegation on the #phpesp_response form, replacing the inline onclick= references to the global other_check() / other_rate_uncheck() functions in question_check.mustache, question_radio.mustache, and rate question rendering. Mustache templates and rate.php now emit data attributes (data-questionnaire-other-check, data-questionnaire-rate-uncheck) read by the delegated handler. The stopgap module.js reload added in 1a is gone; module.js now has zero remaining consumers. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- New tests/output/mobile_test.php asserts that mobile::mobile_view_activity() returns an array with templates / javascript / otherdata / files in the expected shape. - Fixes a TypeError: require_capability() declared $cm as stdClass but the only caller passes a cm_info from $questionnaire->coursemodule(). Tightened the type hint to \cm_info. - Fixes a silent prod bug: mobile_view_activity used $CFG->dirroot without declaring `global $CFG;`, so file_get_contents() was being called with a bad path and the mobile WS payload received 'javascript' => false instead of the intended JS body. Added the missing global. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- module.js: deleted (all callers migrated to AMD in 1a/1b). - preview.php no longer loads module.js (the depend/dependdrop globals it relied on were dead code; other_check inline handlers are now data-attribute delegation registered by complete_formstart's survey_inputs init — preview is read-only and needs neither). - form_options::js_module() dropped along with its dedicated test. - 47 mustache template doc-comments updated: the stale "Classes required for JS: * /mod/questionnaire/module.js" line is now "* none" (the line was always inaccurate boilerplate). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The chart_renderer / build_scoreboard codepath is gated on the questionnaire/usergraph site setting plus a chart_type on a feedback section. No existing Behat scenario enabled usergraph, so the chart canvas branch was never exercised end-to-end. This feature flips usergraph on, sets chart_type_global to Bipolar bars, has a student submit a response, and asserts the <canvas id="cvs"> element appears in the feedback view. Serves as the safety net for upcoming Phase 2 work that moves the chart-rendering JS out of PHP string-building and into an AMD module. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
CI flagged amd/src/slider.js:99 for nested ternary expressions. Refactored the middle-label key selection into an if/else with a single ternary per branch. Behaviour preserved; minified build updated to match. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
PHP chart_renderer no longer accumulates HTML-mixed-with-<script> output.
It now:
- Emits one or two <canvas> elements with unique per-call ids
(questionnaire-chart-N-primary / -secondary).
- Registers the third-party RGraph scripts on $page via $page->requires->js().
- Queues mod_questionnaire/chart::render($spec) via $page->requires->js_call_amd.
The new AMD module amd/src/chart.js is a generic RGraph driver: it walks
the declarative spec, constructs new RGraph[type](canvasId, ...args),
applies each ['key', value] Set() pair, and calls Draw(). No chart-type
logic in JS — all data prep stays in PHP so we don't reimplement
core_text-aware unicode handling.
chart_renderer::render() gained a leading \moodle_page $page parameter;
the two feedback::build_scoreboard call sites pass $PAGE (already a
required global there). PHPUnit assertions for chart_renderer switched
from "expects <script> body content" to "expects canvas markup with the
new id pattern". The feedback_usergraph Behat scenario's xpath was
relaxed from id='cvs' to starts-with(@id, 'questionnaire-chart-').
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… them chart_renderer::render() queues the RGraph third-party scripts on $page via $page->requires->js() at the moment it emits a chart. The report_viewer::init_rgraph() preload, the $usergraph parameter that plumbed it through view_all_responses / view_individual_response, the matching report.php arguments, and the manual RGraph requires->js() chain in myreport.php are now all redundant — removed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The previously committed amd/build/*.min.js files were hand-written rather than produced by the Moodle grunt amd task, so the CI grunt check flagged them as stale and reported the .min.js.map sourcemaps missing. Regenerated all six built modules (attempt_form, chart, printing, sendmessage, slider, survey_inputs) plus their sourcemaps by running npx grunt amd --root=mod/questionnaire inside a node:22-alpine container against the local Moodle install. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
phpcs PSR2.Classes.ClassDeclaration.CloseBraceAfterBody — the deletion of init_rgraph() in the previous commit left an extra newline between the last method and the class's closing brace. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
CI's phpdocs sniff flagged build_bipolar / build_hbar / build_radar / build_rose / build_vprogress as having incomplete parameters lists. Added the full @param list above each helper signature. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
mkresavg (304 -> 57 lines) and mkrescount (215 -> 47 lines) now read as
top-down orchestrators that delegate the chunked subtasks (header config,
rank-column build, sort, per-content row build, no-data fallback) to
focused private helpers. Behaviour is preserved exactly.
Helpers for mkresavg:
- build_averages_headers, effective_length, build_rank_columns,
build_averages_label_row, sort_counts_by_avg,
build_averages_choice_row, make_text_column, make_chart_column,
build_averages_nodata
Helpers for mkrescount:
- fetch_count_choices, sort_rows_by_average, tally_ranks,
build_totals_headers, build_totals_choice_row
File grew by 107 lines (1109 -> 1216) because the data prep is now
named and docblocked rather than inlined. Net effect: zero methods over
150 lines (was three); the longest remaining method is get_results
(143 lines, pure SQL data prep).
PHPUnit 450/450 (test_rank_display_results_returns_tags covers the
end-to-end path); 5/5 rate_*.feature Behat scenarios pass.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The $isna / $osgood / $stravgrank parameters were derivable from $this->question->precise() and not used by any other caller, so the helper now reads them directly and only takes the externally-supplied $stravgvalue. mkresavg drops the matching pre-computation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…lpers Wraps $this->question->precise() == 1 / == 3 in named methods. All inline checks in mkresavg, mkrescount and their helpers now go through these accessors. The $osgood / $isna parameters dropped from build_averages_choice_row, build_totals_headers, and build_totals_choice_row — they read $this->is_*() directly. $isrestricted stays as a parameter because mkresavg and mkrescount derive it from different sources (count($this->counts) vs count($this->question->choices)). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The osgood branch always returned early and the isna check only ran
afterwards, so the two output shapes were threaded through one method
with interleaved shared prep. Split into:
- build_osgood_choice_row — pure osgood layout (text | bar | text)
- build_default_choice_row — default layout with optional N/A col;
returns null when there's nothing to show
with three tiny shared helpers:
- resolve_choice_avg — pull [avg, avgvalue, nbna] from a counts entry
- chart_bar_position — RTL-aware margin / marginpdf
- maybe_prefix_other — apply "Other:" prefix when applicable
mkresavg dispatches via $this->is_osgood() ? … : …. Dropped the dead
$width parameter (only used by an unused intermediate assignment).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
render() 308→27 lines. 13 named helpers own one tab or sub-row each; working state (row/row2/row3/inactive/activated/activetab) moved from locals into private mutable properties reset at top of render(). Introduces private tab_url() to collapse the noisy wwwroot+encoded URL pattern. Preserves one restricted-branch relative-URL quirk with an inline note rather than silently "fixing" it. Tests: 6→11 (preview-hidden-no-questions, nonrespondents-tab-present, vall-sort-subrow-order-tabs, myreport-subrow-for-student-with-multiple, restricted-allreport-branch-emits-sid). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkzhWiBcJQP8vTn2xrXobU
…rom prep_question_for_form Question domain object no longer carries the Moodle editor array as its content. survey::prep_question_for_form() now returns [$question, ['text','format','itemid']]; questions.php destructures and splices the payload into form_data()->content before set_data(). content() is now typed ?string; set_content() only accepts string; the constructor's is_array($value) branch for 'content' is gone. Also fixes a CI-flagged lowercase inline comment in tests/output/tabs_test.php from the Phase B commit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkzhWiBcJQP8vTn2xrXobU
…ndlers mobile_view_activity() 167→72 lines; three protected static handlers own one action-group each (handle_index_action / handle_page_action / handle_review_action) and each returns [data, template, responses] to the orchestrator. Existing add_index_data / add_pagequestion_data / require_capability helpers untouched. Tests: mobile_test 1→4 (respond and review action payload-shape guards plus unknown-action fall-through), so the refactor is covered end-to-end via the WS entry point rather than by mocking internals. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkzhWiBcJQP8vTn2xrXobU
…sponse_survey_display Guards the return-shape of the two large render methods before Phase E's method-length refactor. Invokes both (protected) methods via reflection and asserts on the populated $choicetags / $resptags objects — not the mustache HTML — so the canaries stay stable across template changes. Covers: normal-scale header/row counts, N/A column appended to the header, response render mirrors question shape, osgood sets the $resptags->osgood flag and the 45% first-branch sidecolwidth default. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkzhWiBcJQP8vTn2xrXobU
mchurchward
force-pushed
the
REFACTOR_500_V2
branch
from
August 4, 2026 19:50
02d211c to
b7fe095
Compare
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
1 participant
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.
Full refactoring of questionnaire