CTP-6340: View all feedback on one screen.

[cwarwicker]

This adds a page which lets someone with the correct permissions (probably an external assessor), view all marks and final agreed mark on one page, along with the student submission. Instead of some of them being split onto different pages.

[Copilot]

Pull request overview

Adds an “all feedback” view intended to let permitted users (e.g., external assessors) see all marking-stage feedback and the final agreed mark on a single page alongside the submission/PDF.

Changes:

• Adds a new /actions/feedbacks/all.php endpoint + controller/renderer/template to display all feedback for a submission in one screen.
• Extends grading report cell/template data to expose a “View all feedback” button/link when permitted.
• Enhances advanced-grading review UI by tagging marker columns with a stage-specific CSS class and updating JS to relabel final-agreed output.

Reviewed changes

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

Show a summary per file

File	Description
templates/submissions/tr/marking.mustache	Adds a conditional “View all feedback” button in the marking actions UI.
templates/marking/review.mustache	Adds stage-specific CSS class to marker columns for targeting in JS/CSS.
templates/feedback/all.mustache	New page template to render all feedback (and optionally PDF) in one layout.
styles.css	Fixes a trailing brace formatting issue.
renderers/page_renderer.php	Adds show_all_feedback_page() and augments comparison-view marker data with stage.
lang/en/coursework.php	Adds viewallfeedback language string.
classes/renderers/grading_report_renderer.php	Plumbs viewallfeedback data into template row context.
classes/render_helpers/grading_report/data/marking_cell_data.php	Adds “all feedback” URL generation and exposes it on the marking cell; updates URL builder logic.
classes/models/submission.php	Adds can_show_all_feedback() helper for access decisions.
classes/controllers/submissions_controller.php	Adds a feedback import (currently unused).
classes/controllers/feedback_controller.php	Adds show_all() action to serve the new “all feedback” page.
amd/src/agree_marks.js	Makes init async; relabels final agreed stage heading using core/str.
amd/build/agree_marks.min.js	Rebuilt AMD bundle for the updated agree_marks module.
amd/build/agree_marks.min.js.map	Rebuilt source map for the updated agree_marks module.
actions/feedbacks/all.php	New entrypoint script routing to feedback_controller::show_all().

Suppressed comments (1)

classes/render_helpers/grading_report/data/marking_cell_data.php:252

• get_mark_url() was changed to accept fewer parameters, but there’s still a call in this class (marking_cell_data.php:323) passing a 5th argument (used to distinguish final-grade creation). This will trigger an ArgumentCountError at runtime. Either update that call site or keep a compatible optional parameter here and use it to include isfinalgrade=1 for final feedback URLs.

    public function get_mark_url(string $action, submission $submission, ?stage $stage = null, ?feedback $feedback = null): string {
        if (!in_array($action, ['new', 'edit', 'show', 'all'])) {
            throw new invalid_parameter_exception("Unknown action $action");
        }

---

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

[Copilot]

Pull request overview

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

Suppressed comments (3)

classes/controllers/feedback_controller.php:126

• show_all() authorises access only via can_show_all_feedback(). Because can_show_all_feedback() returns true when there is no feedback, a user who can reach this endpoint could view the submission/PDF even if they cannot "show" the submission via the normal ability rules (viewpdf() explicitly checks ability->cannot('show', $submission)). Add an ability check for showing the submission before rendering the page.

    public function show_all() {
        global $PAGE, $USER;
        $PAGE->set_url('/mod/coursework/actions/feedbacks/all.php', [
            'submissionid' => $this->params['submissionid'],
        ]);
        $submission = new submission($this->params['submissionid']);
        // There's no specific ability for this, we loop through all feedback and see if we can see it all.
        if (!$submission->can_show_all_feedback()) {
            throw new access_denied($submission->get_coursework());
        }
        $renderer = $this->get_page_renderer();
        echo $renderer->show_all_feedback_page($submission);
    }

classes/render_helpers/grading_report/data/marking_cell_data.php:243

• The get_mark_url() PHPDoc still says $action is only 'edit', 'show' or 'new', but the implementation now also supports 'all'. Update the docblock to match so callers and static analysis don’t get misled.

    /**
     * Get the mark URL for a particular action.
     *
     * @param string $action 'edit', 'show' or 'new'
     * @param submission $submission the submission
     * @param stage|null $stage the stage of the row
     * @param feedback|null $feedback $feedback the feedback if editing existing
     * @return string

templates/feedback/all.mustache:29

• Template docs say "feedback" is a single HTML string, but the template iterates over it as a list (and the renderer builds an array of rendered blocks). Update the documented context so it reflects the actual expected type/shape.

    Context variables required for this template:
    * showpdf - Whether the PDF pane should be shown
    * feedback - HTML for the feedbacks

    Example context (json):
    {
        "showpdf": true,
        "feedback": "<p>This is where the feedback from the previous marking stage would appear. The student did a good job overall.</p>",
    }

[Copilot]

Pull request overview

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

Suppressed comments (4)

classes/models/submission.php:1564

• can_show_all_feedback() currently returns false when there is no feedback (empty array), but the new test expects it to be allowed as long as the user can view the submission. Returning false also suppresses the “View all feedback” link for submissions with no feedback.

        $feedbacks = $this->get_feedbacks();

        // If there is no feedback, we don't need to see anything.
        if (!$feedbacks) {
            return false;
        }

        $result = true;
        foreach ($feedbacks as $feedback) {
            $result = $result && $feedback->can_show($this->get_coursework(), $this);
        }
        return $result;

renderers/page_renderer.php:127

• show_all_feedback_page() only renders feedback when there are assessor feedbacks; if a submission has only final feedback (or assessor feedbacks were removed), the final feedback will not be shown at all on the “all feedback” page.

                foreach ($previousfeedbacks as $prev) {
                    $template->feedback[] = $objrenderer->render_feedback($prev, true);
                }
            }
        }

classes/render_helpers/grading_report/data/marking_cell_data.php:258

• get_mark_url() now allows $stage to be null, but the 'new' action still unconditionally dereferences $stage->identifier(), which will fatal if a caller passes null. Guard this explicitly to keep the API safe.

        if ($action === 'all') {
            $params = ['submissionid' => $submission->id()];
        } else if ($action === 'new') {
            $params = ['submissionid' => $submission->id(), 'stageidentifier' => $stage->identifier()];
        } else {
            $params = ['feedbackid' => $feedback ? $feedback->id() : null];
        }

classes/controllers/feedback_controller.php:116

• $USER is imported as a global in show_all() but never used.

    public function show_all() {
        global $PAGE, $USER;
        $PAGE->set_url('/mod/coursework/actions/feedbacks/all.php', [

[Copilot]

Pull request overview

Copilot reviewed 12 out of 15 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

renderers/page_renderer.php:127

• show_all_feedback_page() only renders output when get_assessor_feedbacks() is non-empty. If a submission has only final feedback (e.g., final agreed exists but no "others" feedback in the pool), the page will render with no feedback content even though $final is available. Render $final when $previousfeedbacks is empty.

                foreach ($previousfeedbacks as $prev) {
                    $template->feedback[] = $objrenderer->render_feedback($prev, true);
                }
            }
        }

[DavidUCL]

A couple things

Do we need behat tests?
Do we need a version bump?

[Copilot]

Pull request overview

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

Suppressed comments (4)

classes/render_helpers/grading_report/data/marking_cell_data.php:98

• viewallfeedback is computed unconditionally for multi-marker coursework rows, but the template only renders it inside the agreedmark block. When agreedmark is null (e.g. sampling still in progress), this triggers extra per-feedback permission checks for every row without any UI effect.

        if ($this->coursework->has_multiple_markers()) {
            $rowdata->agreedmark = $this->get_final_feedback_data($rowsbase);

            // Can the user view all the feedback in one place?
            $submission = $rowsbase->get_submission();
            if ($submission) {
                $rowdata->viewallfeedback = $submission->can_show_all_feedback() ? [

classes/models/submission.php:1564

• can_show_all_feedback() iterates all feedbacks using $result = $result && ..., which evaluates every can_show() even after one fails. Since this method is now used when building the grading report row data, short-circuiting will avoid unnecessary ability checks and DB/cache work on the common "not allowed" path.

        $result = true;
        foreach ($feedbacks as $feedback) {
            $result = $result && $feedback->can_show($this->get_coursework(), $this);
        }
        return $result;

tests/phpunit/models/submission_test.php:271

• In this test helper, $teacher/$manager are created but the feedback is created using $this->teacher/$this->otherteacher instead. This couples the test to side-effects of the factory helpers and makes the intent harder to follow.

        $teacher = $this->create_a_teacher();
        $manager = $this->create_another_teacher();
        $this->enrol_the_other_teacher_as_a_manager();
        $this->create_an_assessor_feedback_for_the_submission($this->teacher);
        $this->create_an_assessor_feedback_for_the_submission($this->otherteacher);

classes/render_helpers/grading_report/data/marking_cell_data.php:258

• get_mark_url() now allows $stage/$feedback to be null, but the implementation still dereferences $stage->identifier() for new and builds a feedbackid=null URL for show/edit. Adding explicit parameter validation prevents hard-to-debug fatals/invalid URLs if this helper is reused elsewhere.

        if ($action === 'all') {
            $params = ['submissionid' => $submission->id()];
        } else if ($action === 'new') {
            $params = ['submissionid' => $submission->id(), 'stageidentifier' => $stage->identifier()];
        } else {
            $params = ['feedbackid' => $feedback ? $feedback->id() : null];
        }

[cwarwicker]

A couple things

Do we need behat tests? Do we need a version bump?

I've added a version bump. I can do behat tests next week.

[Copilot]

Pull request overview

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

Suppressed comments (1)

classes/render_helpers/grading_report/data/marking_cell_data.php:247

• get_mark_url() signature was changed to accept fewer arguments, but there is still at least one call site in this class passing a 5th argument (e.g. get_mark_url('new', ..., $finalstage, null, true) in get_final_feedback_data()), which will now cause a fatal "Too many arguments" error. Also, $stage is now nullable but is dereferenced for the 'new' action without validation, which can also fatal if called incorrectly. Consider restoring a 5th optional parameter for backwards compatibility (or updating the call site) and add explicit validation for $stage/$feedback based on $action.

    public function get_mark_url(string $action, submission $submission, ?stage $stage = null, ?feedback $feedback = null): string {