Skip to content

Commit a184e65

Browse files
caswelltomclaude
caswelltom
and
claude
committed
v3.2.2: Security review fixes — install blocker, GDPR, code standards
Critical fixes from Catalyst security review (WR-483440): - Fix install.xml: embedding field TYPE="longtext" → "text" (XMLDB blocker) - GDPR: declare _feedback, _survey_resp, _ut_resp, _audit, _practice_scores in privacy provider metadata; delete and export all personal data tables - Fix delete_data_for_users() asymmetry (now mirrors delete_data_for_user) - Race condition: unique index on convs userid_courseid, try/catch in get_or_create_conversation() with duplicate message migration - Rename all camelCase PHP variables to snake_case (Moodle standard) - Add PHPDoc to all external function classes - Replace hardcoded English strings with get_string() in sse.php, tts.php, reminder_manager.php - Fix course_settings.php page context (syscontext → course context) - Remove create_demo_course.php from production package Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 9422b28 commit a184e65

24 files changed

+601
-747
lines changed

‎analytics.php‎

Lines changed: 70 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -65,9 +65,9 @@
6565
if ($action === 'bulktoggle' && confirm_sesskey()) {
6666
$enabled = required_param('enabled', PARAM_INT);
6767
$scope = optional_param('scope', 'all', PARAM_ALPHA); // 'all' or 'selected'.
68-
$selectedIds = optional_param('selected_ids', '', PARAM_RAW);
69-
if ($scope === 'selected' && !empty($selectedIds)) {
70-
$ids = array_map('intval', explode(',', $selectedIds));
68+
$selected_ids = optional_param('selected_ids', '', PARAM_RAW);
69+
if ($scope === 'selected' && !empty($selected_ids)) {
70+
$ids = array_map('intval', explode(',', $selected_ids));
7171
} else {
7272
$ids = array_map(function($c) { return (int)$c->id; },
7373
$DB->get_records_sql("SELECT c.id FROM {course} c WHERE c.id > 1 AND c.visible = 1"));
@@ -82,9 +82,9 @@
8282
if ($action === 'bulktoggleut' && confirm_sesskey()) {
8383
$utvalue = required_param('utvalue', PARAM_RAW);
8484
$scope = optional_param('scope', 'all', PARAM_ALPHA);
85-
$selectedIds = optional_param('selected_ids', '', PARAM_RAW);
86-
if ($scope === 'selected' && !empty($selectedIds)) {
87-
$ids = array_map('intval', explode(',', $selectedIds));
85+
$selected_ids = optional_param('selected_ids', '', PARAM_RAW);
86+
if ($scope === 'selected' && !empty($selected_ids)) {
87+
$ids = array_map('intval', explode(',', $selected_ids));
8888
} else {
8989
$ids = array_map(function($c) { return (int)$c->id; },
9090
$DB->get_records_sql("SELECT c.id FROM {course} c WHERE c.id > 1 AND c.visible = 1"));
@@ -111,7 +111,7 @@
111111

112112
// ── Build course list with stats ────────────────────────────────────────────
113113
// Get all courses that have at least one SOLA message.
114-
$coursesWithData = $DB->get_records_sql(
114+
$courses_with_data = $DB->get_records_sql(
115115
"SELECT DISTINCT m.courseid, c.fullname, c.shortname
116116
FROM {local_ai_course_assistant_msgs} m
117117
JOIN {course} c ON c.id = m.courseid
@@ -120,35 +120,35 @@
120120
);
121121

122122
// Also get all visible courses (for the enable/disable toggle even if no data yet).
123-
$allCourses = $DB->get_records_sql(
123+
$all_courses = $DB->get_records_sql(
124124
"SELECT c.id, c.fullname, c.shortname
125125
FROM {course} c
126126
WHERE c.id > 1 AND c.visible = 1
127127
ORDER BY c.fullname ASC"
128128
);
129129

130130
// Merge: courses with data + all visible courses.
131-
$courseList = [];
132-
foreach ($allCourses as $c) {
133-
$hasData = isset($coursesWithData[$c->id]);
134-
$enabledVal = get_config('local_ai_course_assistant', 'sola_enabled_course_' . $c->id);
135-
$isEnabled = ($enabledVal !== '0'); // Default: enabled.
136-
$utVal = get_config('local_ai_course_assistant', 'sola_usertesting_course_' . $c->id);
131+
$course_list = [];
132+
foreach ($all_courses as $c) {
133+
$has_data = isset($courses_with_data[$c->id]);
134+
$enabled_val = get_config('local_ai_course_assistant', 'sola_enabled_course_' . $c->id);
135+
$is_enabled = ($enabled_val !== '0'); // Default: enabled.
136+
$ut_val = get_config('local_ai_course_assistant', 'sola_usertesting_course_' . $c->id);
137137
// User testing state: '' = inherit, '1' = on, '0' = off.
138-
$utState = ($utVal === '1') ? 'on' : (($utVal === '0') ? 'off' : 'inherit');
139-
$utGlobal = (bool) get_config('local_ai_course_assistant', 'usertesting_enabled');
140-
$utEffective = ($utVal === '1') || ($utVal !== '0' && $utGlobal);
141-
$courseList[] = [
138+
$ut_state = ($ut_val === '1') ? 'on' : (($ut_val === '0') ? 'off' : 'inherit');
139+
$ut_global = (bool) get_config('local_ai_course_assistant', 'usertesting_enabled');
140+
$ut_effective = ($ut_val === '1') || ($ut_val !== '0' && $ut_global);
141+
$course_list[] = [
142142
'id' => (int) $c->id,
143143
'fullname' => $c->fullname,
144144
'shortname' => $c->shortname,
145-
'has_data' => $hasData,
146-
'enabled' => $isEnabled,
145+
'has_data' => $has_data,
146+
'enabled' => $is_enabled,
147147
'selected' => ((int) $c->id === $courseid),
148-
'ut_on' => $utState === 'on',
149-
'ut_off' => $utState === 'off',
150-
'ut_inherit'=> $utState === 'inherit',
151-
'ut_effective' => $utEffective,
148+
'ut_on' => $ut_state === 'on',
149+
'ut_off' => $ut_state === 'off',
150+
'ut_inherit'=> $ut_state === 'inherit',
151+
'ut_effective' => $ut_effective,
152152
'sesskey' => sesskey(),
153153
'range' => $range,
154154
'courseid_param' => $courseid,
@@ -157,29 +157,29 @@
157157
}
158158

159159
// ── Cross-course summary stats ──────────────────────────────────────────────
160-
$totalMsgsAll = $DB->count_records_sql(
160+
$total_msgs_all = $DB->count_records_sql(
161161
"SELECT COUNT(m.id) FROM {local_ai_course_assistant_msgs} m
162162
JOIN {course} c ON c.id = m.courseid WHERE c.id > 1"
163163
);
164-
$activeStudentsAll = $DB->count_records_sql(
164+
$active_students_all = $DB->count_records_sql(
165165
"SELECT COUNT(DISTINCT m.userid) FROM {local_ai_course_assistant_msgs} m
166166
JOIN {course} c ON c.id = m.courseid WHERE c.id > 1 AND m.role = 'user'"
167167
);
168-
$activeCourses = count($coursesWithData);
169-
$enabledCourses = 0;
170-
foreach ($courseList as $cl) {
168+
$active_courses = count($courses_with_data);
169+
$enabled_courses = 0;
170+
foreach ($course_list as $cl) {
171171
if ($cl['enabled']) {
172-
$enabledCourses++;
172+
$enabled_courses++;
173173
}
174174
}
175175

176176
// ── Per-course analytics (if a course is selected) ──────────────────────────
177-
$courseData = null;
178-
$courseName = '';
177+
$course_data = null;
178+
$course_name = '';
179179
if ($courseid > 0) {
180180
$course = $DB->get_record('course', ['id' => $courseid]);
181181
if ($course) {
182-
$courseName = $course->fullname;
182+
$course_name = $course->fullname;
183183
$overview = analytics::get_overview($courseid, $since);
184184
$dailyusage = analytics::get_daily_usage($courseid, $range > 0 ? $range : 90);
185185
$hotspots = analytics::get_hotspots($courseid, $since);
@@ -252,12 +252,12 @@
252252
? round((float) $feedbacksummary->avg_rating, 1) : 0;
253253

254254
// Survey results.
255-
$surveyData = null;
255+
$survey_data = null;
256256
try {
257-
$surveyResults = \local_ai_course_assistant\survey_manager::get_survey_results($courseid, $since);
258-
if ($surveyResults['total_responses'] > 0) {
259-
$surveyQuestions = [];
260-
foreach ($surveyResults['questions'] as $q) {
257+
$survey_results = \local_ai_course_assistant\survey_manager::get_survey_results($courseid, $since);
258+
if ($survey_results['total_responses'] > 0) {
259+
$survey_questions = [];
260+
foreach ($survey_results['questions'] as $q) {
261261
$sq = [
262262
'text' => $q['text'],
263263
'type' => $q['type'],
@@ -284,31 +284,31 @@
284284
$sq['distribution'] = $dist;
285285
}
286286
if ($q['type'] === 'long_text' && !empty($q['answers'])) {
287-
$textAnswers = [];
287+
$text_answers = [];
288288
foreach (array_slice($q['answers'], 0, 20) as $a) {
289-
$textAnswers[] = ['text' => htmlspecialchars($a)];
289+
$text_answers[] = ['text' => htmlspecialchars($a)];
290290
}
291-
$sq['answers'] = $textAnswers;
291+
$sq['answers'] = $text_answers;
292292
$sq['has_answers'] = true;
293293
}
294-
$surveyQuestions[] = $sq;
294+
$survey_questions[] = $sq;
295295
}
296-
$surveyData = [
297-
'total_responses' => $surveyResults['total_responses'],
298-
'questions' => $surveyQuestions,
296+
$survey_data = [
297+
'total_responses' => $survey_results['total_responses'],
298+
'questions' => $survey_questions,
299299
];
300300
}
301301
} catch (\Throwable $e) {
302302
// Survey tables may not exist yet on older installs.
303303
}
304304

305305
// User testing results.
306-
$utData = null;
306+
$ut_data = null;
307307
try {
308-
$utResults = \local_ai_course_assistant\usertesting_manager::get_results($courseid);
309-
if ($utResults['total_respondents'] > 0) {
310-
$utTasks = [];
311-
foreach ($utResults['tasks'] as $t) {
308+
$ut_results = \local_ai_course_assistant\usertesting_manager::get_results($courseid);
309+
if ($ut_results['total_respondents'] > 0) {
310+
$ut_tasks = [];
311+
foreach ($ut_results['tasks'] as $t) {
312312
$ut = [
313313
'instruction' => $t['instruction'],
314314
'type' => $t['type'],
@@ -343,20 +343,20 @@
343343
$ut['answers'] = $answers;
344344
$ut['has_answers'] = !empty($answers);
345345
}
346-
$utTasks[] = $ut;
346+
$ut_tasks[] = $ut;
347347
}
348-
$utData = [
349-
'total_respondents' => $utResults['total_respondents'],
350-
'tasks' => $utTasks,
348+
$ut_data = [
349+
'total_respondents' => $ut_results['total_respondents'],
350+
'tasks' => $ut_tasks,
351351
];
352352
}
353353
} catch (\Throwable $e) {
354354
// User testing tables may not exist yet.
355355
}
356356

357-
$courseData = [
357+
$course_data = [
358358
'courseid' => $courseid,
359-
'coursename' => $courseName,
359+
'coursename' => $course_name,
360360
'overview' => $overview,
361361
'has_data' => $overview['total_messages'] > 0,
362362
'daily_usage_json' => json_encode($dailyusage),
@@ -379,11 +379,11 @@
379379
'feedback_ratings' => $ratingrows,
380380
'feedback_entries' => $feedbackentries,
381381
'has_feedback' => $feedbacktotal > 0,
382-
'survey_data' => $surveyData,
383-
'has_survey_data' => ($surveyData !== null),
384-
'usertesting_data' => $utData,
385-
'has_usertesting_data' => ($utData !== null),
386-
'has_any_feedback_data' => ($feedbacktotal > 0 || $surveyData !== null || $utData !== null),
382+
'survey_data' => $survey_data,
383+
'has_survey_data' => ($survey_data !== null),
384+
'usertesting_data' => $ut_data,
385+
'has_usertesting_data' => ($ut_data !== null),
386+
'has_any_feedback_data' => ($feedbacktotal > 0 || $survey_data !== null || $ut_data !== null),
387387
'token_analytics_url' => (new moodle_url('/local/ai_course_assistant/token_analytics.php',
388388
['courseid' => $courseid, 'range' => $range]))->out(false),
389389
];
@@ -393,19 +393,19 @@
393393
// ── Build template data ─────────────────────────────────────────────────────
394394
$templatedata = [
395395
// Cross-course summary.
396-
'total_messages_all' => $totalMsgsAll,
397-
'active_students_all' => $activeStudentsAll,
398-
'active_courses' => $activeCourses,
399-
'enabled_courses' => $enabledCourses,
400-
'total_courses' => count($courseList),
396+
'total_messages_all' => $total_msgs_all,
397+
'active_students_all' => $active_students_all,
398+
'active_courses' => $active_courses,
399+
'enabled_courses' => $enabled_courses,
400+
'total_courses' => count($course_list),
401401

402402
// Course list for selector + toggle table.
403-
'courses' => $courseList,
404-
'has_courses' => !empty($courseList),
403+
'courses' => $course_list,
404+
'has_courses' => !empty($course_list),
405405

406406
// Per-course analytics (null if none selected).
407-
'has_course_selected' => ($courseData !== null),
408-
'course' => $courseData,
407+
'has_course_selected' => ($course_data !== null),
408+
'course' => $course_data,
409409

410410
// Time range.
411411
'range' => $range,

‎classes/context_builder.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ public static function build_system_prompt(
106106
}
107107

108108
// Dynamic display name from admin settings.
109-
$displayName = get_config('local_ai_course_assistant', 'display_name') ?: 'SOLA';
109+
$display_name = get_config('local_ai_course_assistant', 'display_name') ?: 'SOLA';
110110

111111
// Institution name from settings.
112112
$institution = get_config('local_ai_course_assistant', 'institution_name') ?: 'Saylor University';
@@ -119,7 +119,7 @@ public static function build_system_prompt(
119119
);
120120

121121
// Replace hardcoded "SOLA" identity with configurable display name.
122-
$prompt = str_replace('You are SOLA', 'You are ' . $displayName, $prompt);
122+
$prompt = str_replace('You are SOLA', 'You are ' . $display_name, $prompt);
123123

124124
// If the template doesn't include {{coursecontent}} but we have content, append it.
125125
if (!empty($coursecontent) && strpos($template, '{{coursecontent}}') === false) {

‎classes/conversation_manager.php‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,18 @@ public static function get_or_create_conversation(int $userid, int $courseid): \
5353
$conv->title = '';
5454
$conv->timecreated = $now;
5555
$conv->timemodified = $now;
56-
$conv->id = $DB->insert_record('local_ai_course_assistant_convs', $conv);
56+
try {
57+
$conv->id = $DB->insert_record('local_ai_course_assistant_convs', $conv);
58+
} catch (\dml_write_exception $e) {
59+
// Race condition: another request created the conversation first.
60+
$conv = $DB->get_record('local_ai_course_assistant_convs', [
61+
'userid' => $userid,
62+
'courseid' => $courseid,
63+
]);
64+
if (!$conv) {
65+
throw $e;
66+
}
67+
}
5768

5869
return $conv;
5970
}

‎classes/external/email_study_notes.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,18 +49,18 @@ public static function execute(int $courseid, string $notes): array {
4949
require_capability('local/ai_course_assistant:use', $context);
5050

5151
$course = $DB->get_record('course', ['id' => $params['courseid']], 'id,fullname', MUST_EXIST);
52-
$displayName = get_config('local_ai_course_assistant', 'display_name') ?: 'SOLA';
52+
$display_name = get_config('local_ai_course_assistant', 'display_name') ?: 'SOLA';
5353

5454
$message = new \core\message\message();
5555
$message->component = 'local_ai_course_assistant';
5656
$message->name = 'study_notes';
5757
$message->userfrom = \core_user::get_noreply_user();
5858
$message->userto = $USER;
59-
$message->subject = $displayName . ' Study Notes — ' . $course->fullname;
59+
$message->subject = $display_name . ' Study Notes — ' . $course->fullname;
6060
$message->fullmessage = $params['notes'];
6161
$message->fullmessageformat = FORMAT_PLAIN;
6262
$message->fullmessagehtml = '<div style="font-family:sans-serif;max-width:600px">'
63-
. '<h2>' . $displayName . ' Study Notes</h2>'
63+
. '<h2>' . $display_name . ' Study Notes</h2>'
6464
. '<p><strong>Course:</strong> ' . htmlspecialchars($course->fullname) . '</p>'
6565
. '<hr>'
6666
. '<div style="white-space:pre-wrap">' . htmlspecialchars($params['notes']) . '</div>'

‎classes/external/generate_insights.php‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -81,10 +81,10 @@ public static function execute(int $courseid): array {
8181
// Gather usability testing data.
8282
$uttext = '';
8383
try {
84-
$utResults = \local_ai_course_assistant\usertesting_manager::get_results($courseid);
85-
if ($utResults['total_respondents'] > 0) {
86-
$uttext = "## Usability Testing Results ({$utResults['total_respondents']} respondents)\n\n";
87-
foreach ($utResults['tasks'] as $t) {
84+
$ut_results = \local_ai_course_assistant\usertesting_manager::get_results($courseid);
85+
if ($ut_results['total_respondents'] > 0) {
86+
$uttext = "## Usability Testing Results ({$ut_results['total_respondents']} respondents)\n\n";
87+
foreach ($ut_results['tasks'] as $t) {
8888
$uttext .= "### Task: {$t['instruction']}\n";
8989
$uttext .= "Type: {$t['type']} | Responses: {$t['response_count']} | "
9090
. "Avg messages: {$t['avg_messages']} | Avg session: {$t['avg_session_minutes']} min\n";
@@ -120,10 +120,10 @@ public static function execute(int $courseid): array {
120120
// Gather survey data.
121121
$surveytext = '';
122122
try {
123-
$surveyResults = \local_ai_course_assistant\survey_manager::get_survey_results($courseid, 0);
124-
if ($surveyResults['total_responses'] > 0) {
125-
$surveytext = "## Survey Results ({$surveyResults['total_responses']} responses)\n\n";
126-
foreach ($surveyResults['questions'] as $q) {
123+
$survey_results = \local_ai_course_assistant\survey_manager::get_survey_results($courseid, 0);
124+
if ($survey_results['total_responses'] > 0) {
125+
$surveytext = "## Survey Results ({$survey_results['total_responses']} responses)\n\n";
126+
foreach ($survey_results['questions'] as $q) {
127127
$surveytext .= "### Q: {$q['text']}\n";
128128
$surveytext .= "Type: {$q['type']} | Responses: {$q['response_count']}\n";
129129

‎classes/external/generate_quiz.php‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@
3333
*/
3434
class generate_quiz extends external_api {
3535

36+
/**
37+
* Returns description of method parameters.
38+
*
39+
* @return external_function_parameters
40+
*/
3641
public static function execute_parameters(): external_function_parameters {
3742
return new external_function_parameters([
3843
'courseid' => new external_value(PARAM_INT, 'Course ID'),
@@ -42,6 +47,15 @@ public static function execute_parameters(): external_function_parameters {
4247
]);
4348
}
4449

50+
/**
51+
* Generate a practice quiz via AI for a given course.
52+
*
53+
* @param int $courseid The course ID.
54+
* @param int $count Number of questions to generate (1-10).
55+
* @param string $topic Topic string, '__guided__' for AI-guided, or empty for current page.
56+
* @param int $cmid Course module ID for current-page mode (0 if not applicable).
57+
* @return array Quiz data with success flag, error message, topic, and questions.
58+
*/
4559
public static function execute(int $courseid, int $count = 3, string $topic = '__guided__', int $cmid = 0): array {
4660
global $DB, $USER;
4761

@@ -155,6 +169,11 @@ public static function execute(int $courseid, int $count = 3, string $topic = '_
155169
];
156170
}
157171

172+
/**
173+
* Returns description of method result value.
174+
*
175+
* @return external_single_structure
176+
*/
158177
public static function execute_returns(): external_single_structure {
159178
return new external_single_structure([
160179
'success' => new external_value(PARAM_BOOL, 'Whether the quiz was generated successfully'),

0 commit comments

Comments
 (0)