# Code Suggestions for qbank_bulktags Plugin

This document outlines detailed improvements to the **qbank_bulktags** plugin
to address correctness bugs, security hardening, style and documentation, UX
enhancements, and testing coverage.

## 1. Correctness & Bug Fixes

### 1.1 Reset `$tags` per question to avoid accumulation

In `helper::bulk_tag_questions()`, the `$tags` array is initialized once before the
loop and then mutated on every iteration, causing previous tags to accumulate.

```php
// classes/helper.php:34-49
public static function bulk_tag_questions(\stdClass $fromform) {
    global $DB;
    $tags = $fromform->formtags;
    if ($fromform->selectedquestions) {
        $questions = self::get_selected_questions($fromform);
        foreach ($questions as $question) {
            if (!$fromform->replacetags) {
                $existingtags = \core_tag_tag::get_item_tags('core_question', 'question', $question->id);
                foreach ($existingtags as $tag) {
                    $tags[] = $tag->get_display_name();
                }
            }
            $context = \context::instance_by_id($question->contextid);
            \core_tag_tag::set_item_tags('core_question', 'question', $question->id, $context, $tags);
        }
    }
}
```
**Suggestion:** Move the initialization of `$tags` inside the loop so each question
starts with a fresh list:

```diff
*** Begin Patch
*** Update File: classes/helper.php
@@ public static function bulk_tag_questions(\stdClass $fromform) {
-        $tags = $fromform->formtags;
-        if ($fromform->selectedquestions) {
-            $questions = self::get_selected_questions($fromform);
-            foreach ($questions as $question) {
-                if (!$fromform->replacetags) {
-                    $existingtags = \core_tag_tag::get_item_tags('core_question', 'question', $question->id);
-                    foreach ($existingtags as $tag) {
-                        $tags[] = $tag->get_display_name();
-                    }
-                }
-                $context = \context::instance_by_id($question->contextid);
-                \core_tag_tag::set_item_tags('core_question', 'question', $question->id, $context, $tags);
-            }
-        }
+        if ($fromform->selectedquestions) {
+            $questions = self::get_selected_questions($fromform);
+            foreach ($questions as $question) {
+                $tags = $fromform->formtags;
+                if (!$fromform->replacetags) {
+                    $existingtags = \core_tag_tag::get_item_tags('core_question', 'question', $question->id);
+                    foreach ($existingtags as $tag) {
+                        $tags[] = $tag->get_display_name();
+                    }
+                }
+                $context = \context::instance_by_id($question->contextid);
+                \core_tag_tag::set_item_tags('core_question', 'question', $question->id, $context, $tags);
+            }
+        }
*** End Patch
```

### 1.2 Prevent empty‑IN SQL in `get_selected_questions()`

`explode(',', '')` returns `['']`, so the SQL can end up with `IN ('')` when no
IDs are provided.

```php
// classes/helper.php:62-73
public static function get_selected_questions(\stdClass $fromform): array {
    global $DB;
    if ($questionids = explode(',', $fromform->selectedquestions)) {
        [$usql, $params] = $DB->get_in_or_equal($questionids);
        $sql = "SELECT q.*, c.contextid
                    FROM {question} q
                    ...
                    WHERE q.id {$usql}";
        $questions = $DB->get_records_sql($sql, $params);
    }
    return $questions ?? [];
}
```
**Suggestion:** Explicitly check for a non-empty string or filter out empty entries
before constructing the `IN` clause.

### 1.3 Clean LLM prompt concatenation

Concatenating raw `questiontext` (which may contain HTML) can send markup to the LLM.

```php
// classes/helper.php:93-96
$action = new \core_ai\aiactions\generate_text(
    contextid: $ctx->id,
    userid: $USER->id,
-   prompttext: $prompt. $question->questiontext,
   prompttext: $prompt . "\n\n" . strip_tags($question->questiontext),
);
```
**Suggestion:** Use `strip_tags()` and insert clear separators (e.g., blank lines).

## 2. Security & Access Control

### 2.1 Add `require_sesskey()` to prevent CSRF

In `tag.php`, the sesskey is generated but never validated.

```php
// tag.php:103-107
if ($fromform = $form->get_data()) {
    if (isset($fromform->submitbutton)) {
        \qbank_bulktags\helper::bulk_tag_questions($fromform);
        redirect($returnurl);
    }
    ...
}
```
**Suggestion:** Call `require_sesskey();` before processing the form submission.

### 2.2 Enforce capability checks on the page

`tag.php` uses `require_login()` but never enforces `moodle/question:editall`.

```php
// tag.php:46-56
if ($cmid) {
    require_login($cm->course, false, $cm);
    $thiscontext = context_system::instance();
} else if ($courseid) {
    require_login($courseid, false);
    $thiscontext = context_system::instance();
} else {
    throw new moodle_exception('missingcourseorcmid','question');
}
```
**Suggestion:** Add:
```php
require_capability('moodle/question:editall', $thiscontext);
```
after establishing `$thiscontext`.

## 3. Style & Documentation

### 3.1 Remove leftover TODO and fix copyright

```php
// settings.php:17-23
/**
 * TODO describe file settings
 *
 * @package    qbank_bulktags
 * @copyright  2025 2024 Marcus Green
 * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
 */
```
**Suggestion:** Replace the TODO with a proper summary and correct the years.

### 3.2 Prune unused imports in form

```php
// classes/output/form/bulk_tags_form.php:9-12
require_once($CFG->dirroot . '/lib/formslib.php');
require_once($CFG->dirroot . '/lib/grouplib.php');
require_once($CFG->dirroot . '/lib/datalib.php');
```
**Suggestion:** Remove `grouplib.php` and `datalib.php` if not used.

### 3.3 Add missing return type hints

```php
// classes/output/form/bulk_tags_form.php:82-90
public function set_data($data) {
    ...
}
```
**Suggestion:** Declare `public function set_data($data): void` for consistency.

### 3.4 Refactor validation for clarity

```php
// classes/output/form/bulk_tags_form.php:99-103
public function validation($data, $files) {
    if (count($data['formtags']) < 1 && empty($data['getaisuggestions'])) {
        return ['formtags' => get_string('error:no_tags_selected','qbank_bulktags')];
    } else {
        return [];
    }
}
```
**Suggestion:** Simplify:
```php
$errors = [];
if (empty($data['formtags']) && empty($data['getaisuggestions'])) {
    $errors['formtags'] = get_string('error:no_tags_selected','qbank_bulktags');
}
return $errors;
```

## 4. Architecture & UX Enhancements

### 4.1 Remove or integrate unused confirmation parameter

```php
// tag.php:87-95
$bulktagsparams = [
    'selectedquestions' => $questionlist,
    'confirm'           => md5($questionlist),
    'sesskey'           => sesskey(),
    ...
];
```
**Suggestion:** Either wire `confirm` into a user confirmation step or remove it.

### 4.2 Enhance large‑batch UX

Consider using AJAX, background processing, or progress indicators to improve
responsiveness when tagging many questions or fetching AI suggestions.

## 5. Testing & Coverage

Add automated tests for edge cases:
- Tag accumulation bug (`replacetags = false`).
- Empty or invalid `selectedquestions` inputs (prevent bad SQL).
- LLM/API failure modes (timeouts, invalid responses, HTML stripping).

---  
Implementing these suggestions will significantly improve the plugin's robustness,
security, and maintainability.
