From be09f7b2d8a8a5a951d8230cd543552594d7c457 Mon Sep 17 00:00:00 2001 From: Rajmund Csehil Date: Fri, 6 Feb 2026 15:06:11 +0100 Subject: [PATCH] [GraphQL] Add status filter to submission comments The first filter is to show all comments visible by the curent user. flag=none refs EVAL-6310 test plan: - set up an assignment - have 2-3 teachers - in speedgrader add published and draft comments as different teachers - open graphiql - try out commentsConnection(filter: {status: [ALL]}) expect to see all comments as teacher as student, expect to see published comments only Change-Id: Ie0a6d405409ef2361e3b26140eb9439d704a8c49 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/400321 Reviewed-by: Chris Soto Tested-by: Service Cloud Jenkins Reviewed-by: Derek Williams QA-Review: Derek Williams Product-Review: Rajmund Csehil --- .../interfaces/submission_interface.rb | 19 +- .../submission_comment_filter_input_type.rb | 6 + .../types/submission_comment_status_type.rb | 26 +++ schema.graphql | 11 + spec/graphql/types/submission_type_spec.rb | 212 ++++++++++++++++++ 5 files changed, 268 insertions(+), 6 deletions(-) create mode 100644 app/graphql/types/submission_comment_status_type.rb diff --git a/app/graphql/interfaces/submission_interface.rb b/app/graphql/interfaces/submission_interface.rb index f891240d0e9..d293860f41f 100644 --- a/app/graphql/interfaces/submission_interface.rb +++ b/app/graphql/interfaces/submission_interface.rb @@ -189,10 +189,12 @@ module Interfaces::SubmissionInterface end def comments_connection(filter:, sort_order:, include_draft_comments:, include_drafts_from_others:, include_provisional_comments:) filter = filter.to_h - filter => all_comments:, for_attempt:, peer_review: + filter => all_comments:, for_attempt:, peer_review:, status: + use_visible_comments = status&.include?("ALL") || (include_draft_comments && include_drafts_from_others) - # Preload provisional comments if needed - provisional_comments_promise = if include_provisional_comments + # Preload provisional comments only when needed (not using visible_submission_comments_for + # which already includes provisional comments via all_submission_comments) + provisional_comments_promise = if include_provisional_comments && !use_visible_comments Loaders::AssociationLoader .for(Submission, :provisional_grades) .load(submission) @@ -215,7 +217,7 @@ module Interfaces::SubmissionInterface load_association(:assignment).then do load_association(:submission_comments).then do provisional_comments_promise.then do |provisional_comments| - comments = if include_draft_comments && include_drafts_from_others + comments = if use_visible_comments submission.visible_submission_comments_for(current_user) elsif include_draft_comments submission.comments_including_drafts_for(current_user) @@ -224,7 +226,7 @@ module Interfaces::SubmissionInterface end comments = comments.to_a - if include_provisional_comments + if include_provisional_comments && !use_visible_comments comments.concat(submission.visible_provisional_comments(current_user, provisional_comments:)) end @@ -233,7 +235,12 @@ module Interfaces::SubmissionInterface comments = comments.sort_by { |comment| [comment.created_at.to_i, comment.id] } if sort_order.present? comments.reverse! if sort_order.to_s.casecmp("desc").zero? - comments.select { |comment| comment.grants_right?(current_user, :read) } + if use_visible_comments + # Permissions checks are already applied in visible_submission_comments_for + comments + else + comments.select { |comment| comment.grants_right?(current_user, :read) } + end end end end diff --git a/app/graphql/types/submission_comment_filter_input_type.rb b/app/graphql/types/submission_comment_filter_input_type.rb index fb8ba569606..75a33676630 100644 --- a/app/graphql/types/submission_comment_filter_input_type.rb +++ b/app/graphql/types/submission_comment_filter_input_type.rb @@ -36,5 +36,11 @@ module Types Whether the current user is completing a peer review and should only see comments authored by themselves. MD + + argument :status, [Types::SubmissionCommentStatusType], <<~MD, required: false, default_value: nil + Filter comments by status type. + - ALL: Returns all comments visible to the current user (published + drafts based on permissions) + When set, includeDraftComments, includeDraftsFromOthers and includeProvisionalComments will be ignored. + MD end end diff --git a/app/graphql/types/submission_comment_status_type.rb b/app/graphql/types/submission_comment_status_type.rb new file mode 100644 index 00000000000..c5abc6dc42a --- /dev/null +++ b/app/graphql/types/submission_comment_status_type.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +# +# Copyright (C) 2026 - present Instructure, Inc. +# +# This file is part of Canvas. +# +# Canvas is free software: you can redistribute it and/or modify it under +# the terms of the GNU Affero General Public License as published by the Free +# Software Foundation, version 3 of the License. +# +# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +# details. +# +# You should have received a copy of the GNU Affero General Public License along +# with this program. If not, see . +# + +module Types + class SubmissionCommentStatusType < Types::BaseEnum + graphql_name "SubmissionCommentStatusType" + value "ALL" + end +end diff --git a/schema.graphql b/schema.graphql index 3b4033573f3..c0077ffb950 100644 --- a/schema.graphql +++ b/schema.graphql @@ -10353,6 +10353,17 @@ input SubmissionCommentFilterInput { comments authored by themselves. """ peerReview: Boolean = false + + """ + Filter comments by status type. + - ALL: Returns all comments visible to the current user (published + drafts based on permissions) + When set, includeDraftComments, includeDraftsFromOthers and includeProvisionalComments will be ignored. + """ + status: [SubmissionCommentStatusType!] = null +} + +enum SubmissionCommentStatusType { + ALL } enum SubmissionCommentsSortOrderType { diff --git a/spec/graphql/types/submission_type_spec.rb b/spec/graphql/types/submission_type_spec.rb index fdebbbcb1ff..dfb5c0bc96e 100644 --- a/spec/graphql/types/submission_type_spec.rb +++ b/spec/graphql/types/submission_type_spec.rb @@ -539,6 +539,18 @@ describe Types::SubmissionType do resolver.resolve("commentsConnection(filter: {allComments: true}) { nodes { _id }}", current_user: @student2) ).to eq [student_2_comment.id.to_s] end + + it "returns only reviewer's own comments with status ALL and peerReview true" do + student_2_comment = @peer_review_submission.add_comment(author: @student2, comment: "student2 peer review") + student_3_comment = @peer_review_submission.add_comment(author: @student3, comment: "student3 peer review") + + result = GraphQLTypeTester.new(@peer_review_submission, current_user: @student2).resolve( + "commentsConnection(filter: { status: [ALL], peerReview: true }) { nodes { _id } }" + ) + + expect(result).to eq [student_2_comment.id.to_s] + expect(result).not_to include(student_3_comment.id.to_s) + end end context "draft comments" do @@ -579,6 +591,190 @@ describe Types::SubmissionType do ).to eq [@comment2.id.to_s] end end + + context "status filter argument" do + before(:once) do + @filter_student = student_in_course(course: @course, active_all: true).user + @teacher2 = teacher_in_course(course: @course, active_all: true).user + @filter_submission = @assignment.submit_homework(@filter_student, body: "student work") + @teacher_comment = @filter_submission.add_comment(author: @teacher, comment: "teacher comment") + @teacher_draft = @filter_submission.add_comment(author: @teacher, comment: "teacher draft", draft_comment: true) + @teacher2_comment = @filter_submission.add_comment(author: @teacher2, comment: "teacher2 comment") + @teacher2_draft = @filter_submission.add_comment(author: @teacher2, comment: "teacher2 draft", draft_comment: true) + @student_comment = @filter_submission.add_comment(author: @filter_student, comment: "student comment") + end + + it "returns published comments for student with status ALL" do + result = GraphQLTypeTester.new(@filter_submission, current_user: @filter_student).resolve( + "commentsConnection(filter: { status: [ALL] }) { nodes { _id } }" + ) + + visible_comments = @filter_submission.visible_submission_comments_for(@filter_student) + expect(result).to match_array(visible_comments.map { |c| c.id.to_s }) + end + + it "returns published and draft comments for teacher with status ALL" do + result = GraphQLTypeTester.new(@filter_submission, current_user: @teacher).resolve( + "commentsConnection(filter: { status: [ALL] }) { nodes { _id } }" + ) + + visible_comments = @filter_submission.visible_submission_comments_for(@teacher) + expect(result).to match_array(visible_comments.map { |c| c.id.to_s }) + end + + it "delegates to visible_submission_comments_for when status is ALL" do + expect_any_instance_of(Submission).to receive(:visible_submission_comments_for).with(@teacher).and_call_original + + GraphQLTypeTester.new(@filter_submission, current_user: @teacher).resolve( + "commentsConnection(filter: { status: [ALL] }) { nodes { _id } }" + ) + end + + it "falls back to published comments only when status array is empty" do + result = GraphQLTypeTester.new(@filter_submission, current_user: @teacher).resolve( + "commentsConnection(filter: { status: [] }) { nodes { _id } }" + ) + + expect(result).to match_array([ + @teacher_comment.id.to_s, + @teacher2_comment.id.to_s, + @student_comment.id.to_s + ]) + expect(result).not_to include(@teacher_draft.id.to_s) + expect(result).not_to include(@teacher2_draft.id.to_s) + end + + it "falls back to published comments only when status is null" do + result = GraphQLTypeTester.new(@filter_submission, current_user: @teacher).resolve( + "commentsConnection(filter: { status: null }) { nodes { _id } }" + ) + + expect(result).to match_array([ + @teacher_comment.id.to_s, + @teacher2_comment.id.to_s, + @student_comment.id.to_s + ]) + expect(result).not_to include(@teacher_draft.id.to_s) + expect(result).not_to include(@teacher2_draft.id.to_s) + end + + it "overrides includeDraftComments parameter when status is ALL" do + result_with_false = GraphQLTypeTester.new(@filter_submission, current_user: @teacher).resolve( + "commentsConnection(filter: { status: [ALL] }, includeDraftComments: false) { nodes { _id } }" + ) + result_with_true = GraphQLTypeTester.new(@filter_submission, current_user: @teacher).resolve( + "commentsConnection(filter: { status: [ALL] }, includeDraftComments: true) { nodes { _id } }" + ) + + visible_comments = @filter_submission.visible_submission_comments_for(@teacher) + expect(result_with_false).to match_array(visible_comments.map { |c| c.id.to_s }) + expect(result_with_true).to match_array(visible_comments.map { |c| c.id.to_s }) + end + + it "filters to specified attempt only when combined with forAttempt" do + @filter_submission.update!(attempt: 2) + attempt_1_comment = @filter_submission.add_comment(author: @teacher, comment: "attempt 1 comment", attempt: 1) + attempt_2_comment = @filter_submission.add_comment(author: @teacher, comment: "attempt 2 comment", attempt: 2) + + result = GraphQLTypeTester.new(@filter_submission, current_user: @teacher).resolve( + "commentsConnection(filter: { status: [ALL], forAttempt: 2 }) { nodes { _id } }" + ) + + expect(result).to include(attempt_2_comment.id.to_s) + expect(result).not_to include(attempt_1_comment.id.to_s) + end + + it "sorts comments newest first when sortOrder is desc" do + result = GraphQLTypeTester.new(@filter_submission, current_user: @teacher).resolve( + "commentsConnection(filter: { status: [ALL] }, sortOrder: desc) { nodes { _id } }" + ) + + visible_comments = @filter_submission.visible_submission_comments_for(@teacher) + expected_order = visible_comments.sort_by { |c| [c.created_at.to_i, c.id] }.reverse.map { |c| c.id.to_s } + expect(result).to eq(expected_order) + end + + it "sorts comments oldest first when sortOrder is asc" do + result = GraphQLTypeTester.new(@filter_submission, current_user: @teacher).resolve( + "commentsConnection(filter: { status: [ALL] }, sortOrder: asc) { nodes { _id } }" + ) + + visible_comments = @filter_submission.visible_submission_comments_for(@teacher) + expected_order = visible_comments.sort_by { |c| [c.created_at.to_i, c.id] }.map { |c| c.id.to_s } + expect(result).to eq(expected_order) + end + + it "returns comments from all attempts when allComments overrides forAttempt" do + @filter_submission.update!(attempt: 2) + attempt_1_comment = @filter_submission.add_comment(author: @teacher, comment: "attempt 1 comment", attempt: 1) + attempt_2_comment = @filter_submission.add_comment(author: @teacher, comment: "attempt 2 comment", attempt: 2) + + result = GraphQLTypeTester.new(@filter_submission, current_user: @teacher).resolve( + "commentsConnection(filter: { status: [ALL], allComments: true, forAttempt: 2 }) { nodes { _id } }" + ) + + expect(result).to include(attempt_1_comment.id.to_s) + expect(result).to include(attempt_2_comment.id.to_s) + end + + context "with provisional comments" do + before(:once) do + @final_grader = @teacher + @provisional_grader1 = teacher_in_course(course: @course, active_all: true).user + @provisional_grader2 = teacher_in_course(course: @course, active_all: true).user + + @moderated_assignment = @course.assignments.create!( + name: "moderated assignment", + moderated_grading: true, + grader_count: 3, + final_grader: @final_grader + ) + @moderated_submission = @moderated_assignment.submit_homework(@filter_student, body: "student work") + + @moderated_assignment.grade_student(@filter_student, grade: 8, grader: @final_grader, provisional: true) + @moderated_assignment.grade_student(@filter_student, grade: 9, grader: @provisional_grader1, provisional: true) + @moderated_assignment.grade_student(@filter_student, grade: 7, grader: @provisional_grader2, provisional: true) + + @final_grader_comment = @moderated_submission.add_comment( + author: @final_grader, + comment: "final grader provisional comment", + provisional: true + ) + @provisional_comment1 = @moderated_submission.add_comment( + author: @provisional_grader1, + comment: "provisional grader 1 comment", + provisional: true + ) + @provisional_comment2 = @moderated_submission.add_comment( + author: @provisional_grader2, + comment: "provisional grader 2 comment", + provisional: true + ) + end + + it "returns provisional comments regardless of includeProvisionalComments parameter when status is ALL" do + result_with_false = GraphQLTypeTester.new(@moderated_submission, current_user: @provisional_grader1).resolve( + "commentsConnection(filter: { status: [ALL] }, includeProvisionalComments: false) { nodes { _id } }" + ) + result_with_true = GraphQLTypeTester.new(@moderated_submission, current_user: @provisional_grader1).resolve( + "commentsConnection(filter: { status: [ALL] }, includeProvisionalComments: true) { nodes { _id } }" + ) + + visible_comments = @moderated_submission.visible_submission_comments_for(@provisional_grader1) + expect(result_with_false).to match_array(visible_comments.map { |c| c.id.to_s }) + expect(result_with_true).to match_array(visible_comments.map { |c| c.id.to_s }) + expect(result_with_false).to include(@provisional_comment1.id.to_s) + end + + it "returns no provisional comments to students when grades are unpublished" do + result = GraphQLTypeTester.new(@moderated_submission, current_user: @filter_student).resolve( + "commentsConnection(filter: { status: [ALL] }) { nodes { _id } }" + ) + + expect(result).to eq([]) + end + end + end end describe "submission_drafts" do @@ -1051,6 +1247,22 @@ describe Types::SubmissionType do submission_type.resolve(query) end + it "does not call visible_provisional_comments when includeProvisionalComments is true and filter status [ALL]" do + expect_any_instance_of(Submission).not_to receive(:visible_provisional_comments).with(@teacher, provisional_comments: [@provisional_comment]).and_call_original + + submission_type = GraphQLTypeTester.new(@submission, current_user: @teacher) + query = "commentsConnection(filter: {status: [ALL]}, includeProvisionalComments: true) { nodes { _id } }" + submission_type.resolve(query) + end + + it "does not call visible_provisional_comments when includeProvisionalComments is true and includeDraftComments and includeDraftsFromOthers are true" do + expect_any_instance_of(Submission).not_to receive(:visible_provisional_comments).with(@teacher, provisional_comments: [@provisional_comment]).and_call_original + + submission_type = GraphQLTypeTester.new(@submission, current_user: @teacher) + query = "commentsConnection(filter: {}, includeDraftComments: true, includeDraftsFromOthers: true, includeProvisionalComments: true) { nodes { _id } }" + submission_type.resolve(query) + end + it "does not call visible_provisional_comments when includeProvisionalComments is false" do expect_any_instance_of(Submission).not_to receive(:visible_provisional_comments)