Skip to content

CTP-5899: Add internal markers comment to agreed grade page. - #309

Open
cwarwicker wants to merge 1 commit into
ucl-isd:mainfrom
cwarwicker:CTP-5899
Open

CTP-5899: Add internal markers comment to agreed grade page.#309
cwarwicker wants to merge 1 commit into
ucl-isd:mainfrom
cwarwicker:CTP-5899

Conversation

@cwarwicker

Copy link
Copy Markdown
Collaborator

This is only visible on this page, and never by the student.

Copilot AI review requested due to automatic review settings August 7, 2026 12:05
@cwarwicker

cwarwicker commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

The XMLDB editor fixed some whitespace stuff in the install.xml as well, when I added new fields.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an internal “marker notes” field intended for the final agreed grading stage, backed by new DB columns on coursework_feedbacks, and wires it into the feedback edit form/controller flow.

Changes:

  • Bumps plugin version and adds an English language string for the new internal notes label.
  • Adds internalcomment / internalcommentformat columns to coursework_feedbacks via install + upgrade steps.
  • Adds form/controller/model support to edit and persist the internal notes (agreement stage).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
version.php Plugin version bump to trigger upgrade.
lang/en/coursework.php Adds label text for the internal marker notes editor.
db/upgrade.php Adds new feedback columns during upgrade (needs NOT NULL + backup/restore follow-up).
db/install.xml Adds new feedback columns for fresh installs (needs NOT NULL alignment).
classes/models/feedback.php Introduces new model properties for internal notes (needs safe initialisation).
classes/forms/assessor_feedback_mform.php Adds the internal notes editor on the final agreed stage and saves submitted values.
classes/controllers/feedback_controller.php Prepares internal notes editor data for editing (needs safe format fallback).
Suppressed comments (1)

classes/models/feedback.php:179

  • $internalcommentformat is a non-nullable typed property without a default. If the DB value is NULL (possible with the current schema/upgrade), apply_data() will not assign it and accessing it will fatally error. Initialise it to a sensible default (e.g. FORMAT_HTML) to match feedbackcommentformat.
    /**
     * @var int Format of the internal comment.
     */
    public int $internalcommentformat;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread classes/models/feedback.php
Comment thread db/upgrade.php Outdated
Comment thread db/install.xml Outdated
Comment thread classes/controllers/feedback_controller.php
Comment thread db/upgrade.php
Copilot AI review requested due to automatic review settings August 7, 2026 12:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (2)

classes/controllers/feedback_controller.php:235

  • This unconditionally reads $teacherfeedback->internalcomment / internalcommentformat. Because table_base::apply_data() skips NULL values (isset() check), either property can be uninitialised and this can fatal when editing existing feedback rows created before the new columns were populated. Use isset() guards and fall back to sane defaults when constructing the editor array.
        $teacherfeedback->internalcomment = [
            'text' => $teacherfeedback->internalcomment,
            'format' => $teacherfeedback->internalcommentformat,
        ];

classes/models/feedback.php:175

  • table_base::apply_data() only assigns columns when isset($data[$columnname]) is true, so a NULL internalcomment DB value leaves this typed property uninitialised. Any read (e.g. in the controller) can then fatal with "Typed property ... must not be accessed before initialization". Initialise the property (and give internalcommentformat a default) so they are always safe to read.
    /**
     * @var mixed Internal comment, not visible to student.
     */
    public mixed $internalcomment;

Copilot AI review requested due to automatic review settings August 7, 2026 12:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (2)

db/upgrade.php:417

  • The new coursework_feedbacks.internalcomment data is user-related free-text, but the plugin privacy provider does not declare or export it (see classes/privacy/provider.php where feedbackcomment is included but internalcomment is not). This makes the privacy metadata/export incomplete after this schema change; please update the privacy provider metadata and export formatting to include internalcomment (and its format where appropriate), plus any needed language strings.
        // Define field internalcomment to be added to coursework_feedbacks.
        $table = new xmldb_table('coursework_feedbacks');
        $field = new xmldb_field('internalcomment', XMLDB_TYPE_TEXT, null, null, null, null, null, 'finalised');

        // Conditionally launch add field internalcomment.

classes/forms/assessor_feedback_mform.php:196

  • This introduces a new editor field (internalcomment) and persistence logic for the final agreed stage, but there is no automated coverage ensuring it appears only for final_agreed_1, saves correctly, and is not shown to students. There are existing Behat scenarios covering feedbackcomment editing (e.g. tests/behat/feedback_single_marking.feature); adding a similar scenario for internalcomment would prevent regressions.
        // Is this the agreement stage?
        if ($this->feedback->get_stage()->identifier() === final_agreed::STAGE_FINAL_AGREED_1) {
            $this->_form->addElement('editor', 'internalcomment', get_string('internalcomments', 'mod_coursework'));
            $this->_form->setType('internalcomment', PARAM_RAW);
        }

This is only visible on this page, and never by the student.
Copilot AI review requested due to automatic review settings August 7, 2026 13:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (1)

db/upgrade.php:424

  • New persistent fields internalcomment / internalcommentformat are added to coursework_feedbacks, but the plugin’s GDPR/privacy provider does not describe or export this data. classes/privacy/provider.php only declares/exports feedbackcomment (e.g. get_metadata() and format_submissions_feedback()) so internalcomment will be omitted from user data export and the metadata registry will be incomplete.
    if ($oldversion < 2026070602) {
        // Define field internalcomment to be added to coursework_feedbacks.
        $table = new xmldb_table('coursework_feedbacks');
        $field = new xmldb_field('internalcomment', XMLDB_TYPE_TEXT, null, null, null, null, null, 'finalised');

        // Conditionally launch add field internalcomment.
        if (!$dbman->field_exists($table, $field)) {
            $dbman->add_field($table, $field);
        }

        $field = new xmldb_field('internalcommentformat', XMLDB_TYPE_INTEGER, 1, null, true, null, 1, 'internalcomment');
        if (!$dbman->field_exists($table, $field)) {
            $dbman->add_field($table, $field);
        }

$this->_form->addElement('editor', 'internalcomment', get_string('internalcomments', 'mod_coursework'));
$this->_form->setType('internalcomment', PARAM_RAW);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the JIRA ticket: 'As a pair of markers agreeing marks, we want to document the outcome of our discussion and final agreed marking internally and to the external examiner.'

I'm not sure about this, but it looks like internalcomment is editable on this page to those with. So an initial marker, a agreeing marker or an administrator (mod/coursework:administergrades) can see the comment.

How do external examiners see the internalcomments?

This one is read-only: mod/coursework:viewallgradesatalltimes - but it's a write form

@cwarwicker - might be worth asking Eliot who the external examiners are, what capabilities they have, and where in moodle they're supposed to be able to see these comments?

Is that functionality supposed to be part of this ticket?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be paused until CTP-6340 is merged, as then I can add that internal comment to that page.

@cwarwicker cwarwicker self-assigned this Aug 13, 2026
@cwarwicker cwarwicker added the do not merge For some reasons this change is not ready to be considered label Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge For some reasons this change is not ready to be considered

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants