From 590d9fafb495eba9d815d3542241e0f19d510053 Mon Sep 17 00:00:00 2001 From: Matthew Lemon Date: Thu, 10 Dec 2020 13:02:53 -0700 Subject: [PATCH] create gql mutation for add conversation msg fixes VICE-1005 flag=react_inbox Test Plan: - Have an existing conversation between several users - Login as one of those users and navigate to /graphiql - Run the following mutation: ``` mutation MyMutation { __typename addConversationMessage( input: { conversationId: , body: "This is a test", recipients: [] } ) { conversationMessage { _id attachmentsConnection { nodes { displayName } } author { name } body conversationId mediaComment { _id title } } messageQueued errors { attribute message } } } ``` - The mutation should add a message to the conversation Change-Id: Iba5a8901408d8d6acc80d629f2a89298c39fc4ac Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/254849 Tested-by: Service Cloud Jenkins Reviewed-by: Caleb Guanzon QA-Review: Caleb Guanzon Product-Review: Caleb Guanzon --- app/.rubocop.yml | 4 + app/controllers/conversations_controller.rb | 26 +-- app/graphql/audit_log_field_extension.rb | 1 + .../mutations/add_conversation_message.rb | 101 ++++++++++ app/graphql/types/mutation_type.rb | 1 + app/helpers/conversations_helper.rb | 36 +++- .../add_conversation_message_spec.rb | 184 ++++++++++++++++++ 7 files changed, 326 insertions(+), 27 deletions(-) create mode 100644 app/.rubocop.yml create mode 100644 app/graphql/mutations/add_conversation_message.rb create mode 100644 spec/graphql/mutations/add_conversation_message_spec.rb diff --git a/app/.rubocop.yml b/app/.rubocop.yml new file mode 100644 index 00000000000..1c23f135e42 --- /dev/null +++ b/app/.rubocop.yml @@ -0,0 +1,4 @@ +inherit_from: ../.rubocop.yml + +Migration/Delay: + Enabled: false diff --git a/app/controllers/conversations_controller.rb b/app/controllers/conversations_controller.rb index f86b7fdc62f..30d057d6974 100644 --- a/app/controllers/conversations_controller.rb +++ b/app/controllers/conversations_controller.rb @@ -887,6 +887,7 @@ class ConversationsController < ApplicationController # } # def add_message + # TODO: VICE-1037 logic is mostly duplicated in AddConversationMessage get_conversation(true) context = @conversation.conversation.context @@ -908,26 +909,7 @@ class ConversationsController < ApplicationController # find included_messages message_ids = params[:included_messages] message_ids = message_ids.split(/,/) if message_ids.is_a?(String) - - # these checks could be folded into the initial ConversationMessage lookup for better efficiency - if message_ids - - # sanity check: are the messages part of this conversation? - db_ids = ConversationMessage.where(:id => message_ids, :conversation_id => @conversation.conversation_id).pluck(:id) - unless db_ids.count == message_ids.count - return render_error('included_messages', 'not for this conversation') - end - message_ids = db_ids - - # sanity check: can the user see the included messages? - found_count = 0 - Shard.partition_by_shard(message_ids) do |shard_message_ids| - found_count += ConversationMessageParticipant.where(:conversation_message_id => shard_message_ids, :user_id => @current_user).count - end - unless found_count == message_ids.count - return render_error('included_messages', 'not a participant') - end - end + validate_message_ids(message_ids, @conversation) message_args = build_message_args if @conversation.should_process_immediately? @@ -941,6 +923,10 @@ class ConversationsController < ApplicationController else render :json => {}, :status => :bad_request end + rescue ConversationsHelper::InvalidMessageForConversationError + render_error('included_messages', 'not for this conversation') + rescue ConversationsHelper::InvalidMessageParticipantError + render_error('included_messages', 'not a participant') end # @API Delete a message diff --git a/app/graphql/audit_log_field_extension.rb b/app/graphql/audit_log_field_extension.rb index 17123817120..b43986f4b7d 100644 --- a/app/graphql/audit_log_field_extension.rb +++ b/app/graphql/audit_log_field_extension.rb @@ -141,6 +141,7 @@ class AuditLogFieldExtension < GraphQL::Schema::FieldExtension next if mutation == Mutations::CreateConversation next if mutation == Mutations::DeleteConversationMessage next if mutation == Mutations::DeleteConversation + next if mutation == Mutations::AddConversationMessage logger = Logger.new(mutation, context, arguments) diff --git a/app/graphql/mutations/add_conversation_message.rb b/app/graphql/mutations/add_conversation_message.rb new file mode 100644 index 00000000000..b5990947a83 --- /dev/null +++ b/app/graphql/mutations/add_conversation_message.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +# +# Copyright (C) 2020 - 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 . +# + +class Mutations::AddConversationMessage < Mutations::BaseMutation + graphql_name 'AddConversationMessage' + + include ConversationsHelper + + argument :conversation_id, ID, required: true, prepare: GraphQLHelpers.relay_or_legacy_id_prepare_func('Conversation') + argument :body, String, required: true + argument :recipients, [ID], required: true, prepare: GraphQLHelpers.relay_or_legacy_ids_prepare_func('User') + argument :included_messages, [ID], required: false, prepare: GraphQLHelpers.relay_or_legacy_ids_prepare_func('ConversationMessage') + argument :attachment_ids, [ID], required: false, prepare: GraphQLHelpers.relay_or_legacy_ids_prepare_func('Attachment') + argument :media_comment_id, ID, required: false, prepare: GraphQLHelpers.relay_or_legacy_id_prepare_func('MediaObject') + argument :media_comment_type, String, required: false + argument :user_note, Boolean, required: false + + field :conversation_message, Types::ConversationMessageType, null: true + field :message_queued, Boolean, null: true + # TODO: VICE-1037 logic is mostly duplicated in ConversationsController + def resolve(input:) + conversation = get_conversation(input[:conversation_id]) + + context = conversation.conversation.context + if context.is_a?(Course) && context.workflow_state == 'completed' && !context.grants_right?(current_user, session, :read_as_admin) + return validation_error(I18n.t('Course concluded, unable to send messages')) + end + + if conversation.conversation.replies_locked_for?(current_user) + return validation_error(I18n.t('Unauthorized, unable to add messages to conversation')) + end + + recipients = normalize_recipients( + recipients: input[:recipients], + context_code: conversation.conversation.context.asset_string, + conversation_id: conversation.conversation_id, + current_user: current_user + ) + if recipients && !conversation.conversation.can_add_participants?(recipients) + return validation_error(I18n.t('Too many participants for group conversation')) + end + + tags = infer_tags( + recipients: conversation.conversation.participants.pluck(:id), + context_code: conversation.conversation.context.asset_string + ) + + message_ids = input[:included_messages] + validate_message_ids(message_ids, conversation, current_user: current_user) + + message_args = build_message_args( + body: input[:body], + attachment_ids: input[:attachment_ids], + domain_root_account_id: self.context[:domain_root_account].id, + media_comment_id: input[:media_comment_id], + media_comment_type: input[:media_comment_type], + user_note: input[:user_note], + current_user: current_user + ) + if conversation.should_process_immediately? + message = conversation.process_new_message(message_args, recipients, message_ids, tags) + return {conversation_message: message} + else + conversation.delay(strand: "add_message_#{conversation.global_conversation_id}"). + process_new_message(message_args, recipients, message_ids, tags) + return {message_queued: true} + end + rescue ActiveRecord::RecordNotFound + raise GraphQL::ExecutionError, 'not found' + rescue ActiveRecord::RecordInvalid => e + errors_for(e.record) + rescue ConversationsHelper::InvalidMessageForConversationError + validation_error(I18n.t('included_messages not for this conversation')) + rescue ConversationsHelper::InvalidMessageParticipantError + validation_error('Current user is not a participant of the included_messages') + end + + def get_conversation(id) + conversation = current_user.all_conversations.find_by(conversation_id: id) + raise ActiveRecord::RecordNotFound unless conversation + + conversation + end +end \ No newline at end of file diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index 234e915849f..970945337f2 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -38,6 +38,7 @@ class Types::MutationType < Types::ApplicationObjectType super(*args, **kwargs, extensions: [PostgresTimeoutFieldExtension, AuditLogFieldExtension]) end + field :add_conversation_message, mutation: Mutations::AddConversationMessage field :create_conversation, mutation: Mutations::CreateConversation field :create_group_in_set, mutation: Mutations::CreateGroupInSet field :hide_assignment_grades, mutation: Mutations::HideAssignmentGrades diff --git a/app/helpers/conversations_helper.rb b/app/helpers/conversations_helper.rb index 5a723543435..49edd2894ad 100644 --- a/app/helpers/conversations_helper.rb +++ b/app/helpers/conversations_helper.rb @@ -48,7 +48,7 @@ module ConversationsHelper result end - def normalize_recipients(recipients: nil, context_code: nil, conversation_id: nil) + def normalize_recipients(recipients: nil, context_code: nil, conversation_id: nil, current_user: @current_user) if defined?(params) recipients ||= params[:recipients] context_code ||= params[:context_code] @@ -69,15 +69,15 @@ module ConversationsHelper end users, contexts = AddressBook.partition_recipients(recipients) - known = @current_user.address_book.known_users( + known = current_user.address_book.known_users( users, context: context, conversation_id: conversation_id, - strict_checks: !Account.site_admin.grants_right?(@current_user, session, :send_messages) + strict_checks: !Account.site_admin.grants_right?(current_user, session, :send_messages) ) - contexts.each { |c| known.concat(@current_user.address_book.known_in_context(c)) } + contexts.each { |c| known.concat(current_user.address_book.known_in_context(c)) } @recipients = known.uniq(&:id) - @recipients.reject! { |u| u.id == @current_user.id } unless @recipients == [@current_user] && recipients.count == 1 + @recipients.reject! { |u| u.id == current_user.id } unless @recipients == [current_user] && recipients.count == 1 @recipients end @@ -134,7 +134,8 @@ module ConversationsHelper domain_root_account_id: nil, media_comment_id: nil, media_comment_type: nil, - user_note: nil + user_note: nil, + current_user: @current_user ) if defined?(params) body ||= params[:body] @@ -146,7 +147,7 @@ module ConversationsHelper user_note = params[:user_note] if user_note.nil? end [ - @current_user, + current_user, body, { attachment_ids: attachment_ids, @@ -208,6 +209,23 @@ module ConversationsHelper end end + def validate_message_ids(message_ids, conversation, current_user: @current_user) + if message_ids + # sanity check: are the messages part of this conversation? + db_ids = ConversationMessage.where(id: message_ids, conversation_id: conversation.conversation_id).pluck(:id) + raise InvalidMessageForConversationError unless db_ids.count == message_ids.count + + message_ids = db_ids + + # sanity check: can the user see the included messages? + found_count = 0 + Shard.partition_by_shard(message_ids) do |shard_message_ids| + found_count += ConversationMessageParticipant.where(conversation_message_id: shard_message_ids, user_id: current_user).count + end + raise InvalidMessageParticipantError unless found_count == message_ids.count + end + end + class InvalidContextError < StandardError; end class InvalidContextPermissionsError < StandardError; end @@ -215,4 +233,8 @@ module ConversationsHelper class CourseConcludedError < StandardError; end class InvalidRecipientsError < StandardError; end + + class InvalidMessageForConversationError < StandardError; end + + class InvalidMessageParticipantError < StandardError; end end diff --git a/spec/graphql/mutations/add_conversation_message_spec.rb b/spec/graphql/mutations/add_conversation_message_spec.rb new file mode 100644 index 00000000000..2d8d5c8439c --- /dev/null +++ b/spec/graphql/mutations/add_conversation_message_spec.rb @@ -0,0 +1,184 @@ +# frozen_string_literal: true + +# +# Copyright (C) 2020 - 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 . +# + +require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper') +require_relative '../graphql_spec_helper' + +RSpec.describe Mutations::AddConversationMessage do + before(:once) do + course_with_teacher(active_all: true) + student_in_course(active_all: true) + end + + def conversation(opts = {}) + num_other_users = opts[:num_other_users] || 1 + course = opts[:course] || @course + user_data = num_other_users.times.map { {name: 'User'} } + users = opts[:users] || create_users_in_course(course, user_data, account_associations: true, return_type: :record) + @conversation = @user.initiate_conversation(users) + @conversation.add_message(opts[:message] || 'test') + @conversation.conversation.update_attribute(:context, course) + @conversation + end + + # rubocop:disable Metrics/ParameterLists + def mutation_str( + conversation_id: nil, + body: nil, + recipients: nil, + included_messages: nil, + attachment_ids: nil, + media_comment_id: nil, + media_comment_type: nil, + user_note: nil + ) + <<~GQL + mutation { + addConversationMessage(input: { + conversationId: "#{conversation_id}" + body: "#{body}" + recipients: #{recipients} + #{"includedMessages: #{included_messages}" if included_messages} + #{"attachmentIds: #{attachment_ids}" if attachment_ids} + #{"mediaCommentId: \"#{media_comment_id}\"" if media_comment_id} + #{"mediaCommentType: \"#{media_comment_type}\"" if media_comment_type} + #{"userNote: #{user_note}" unless user_note.nil?} + }) { + conversationMessage { + _id + attachmentsConnection { + nodes { + displayName + } + } + author { + name + } + body + conversationId + mediaComment { + _id + title + } + } + messageQueued + errors { + attribute + message + } + } + } + GQL + end + # rubocop:enable Metrics/ParameterLists + + def run_mutation(opts = {}, current_user = @student) + result = CanvasSchema.execute( + mutation_str(opts), + context: { + current_user: current_user, + domain_root_account: @course.account.root_account, + request: ActionDispatch::TestRequest.create + } + ) + result.to_h.with_indifferent_access + end + + it 'should add a message' do + conversation + result = run_mutation(conversation_id: @conversation.conversation_id, body: 'This is a neat message', recipients: [@teacher.id.to_s]) + + expect(result.dig('errors')).to be nil + expect(result.dig('data', 'addConversationMessage', 'errors')).to be nil + expect( + result.dig('data', 'addConversationMessage', 'conversationMessage', 'body') + ).to eq 'This is a neat message' + cm = ConversationMessage.find(result.dig('data', 'addConversationMessage', 'conversationMessage', '_id')) + expect(cm).to_not be nil + expect(cm.conversation_id).to eq @conversation.conversation_id + end + + it 'should require permissions' do + conversation + @course.account.role_overrides.create!(permission: :send_messages, role: student_role, enabled: false) + + result = run_mutation(conversation_id: @conversation.conversation_id, body: 'need some perms yo', recipients: [@teacher.id.to_s]) + expect(result.dig('errors')).to be nil + expect(result.dig('data', 'addConversationMessage', 'conversationMessage')).to be nil + expect( + result.dig('data', 'addConversationMessage', 'errors', 0, 'message') + ).to eq 'Unauthorized, unable to add messages to conversation' + end + + it 'should queue a job if needed' do + # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(ConversationParticipant).to receive(:should_process_immediately?).and_return(false) + # rubocop:enable RSpec/AnyInstance + conversation + result = run_mutation(conversation_id: @conversation.conversation_id, body: 'This should be delayed', recipients: [@teacher.id.to_s]) + + expect(result.dig('errors')).to be nil + expect(result.dig('data', 'addConversationMessage', 'conversationMessage')).to be nil + expect(result.dig('data', 'addConversationMessage', 'messageQueued')).to be true + expect(@conversation.reload.messages.count(:all)).to eq 1 + run_jobs + expect(@conversation.reload.messages.count(:all)).to eq 2 + end + + it 'should generate a user note when requested' do + Account.default.update_attribute(:enable_user_notes, true) + conversation(users: [@teacher]) + + result = run_mutation({conversation_id: @conversation.conversation_id, body: 'Have a note', recipients: [@student.id.to_s]}, @teacher) + expect(result.dig('errors')).to be nil + cm = ConversationMessage.find(result.dig('data', 'addConversationMessage', 'conversationMessage', '_id')) + student = cm.recipients.first + expect(student.user_notes.size).to eq 0 + + result = run_mutation({conversation_id: @conversation.conversation_id, body: 'Have a note', recipients: [@student.id.to_s], user_note: true}, @teacher) + expect(result.dig('errors')).to be nil + cm = ConversationMessage.find(result.dig('data', 'addConversationMessage', 'conversationMessage', '_id')) + student = cm.recipients.first + expect(student.user_notes.size).to eq 1 + end + + it 'should not allow new messages in concluded courses for students' do + conversation + @course.update!(workflow_state: 'completed') + + result = run_mutation(conversation_id: @conversation.conversation_id, body: 'uh uh uh', recipients: [@teacher.id.to_s]) + expect(result.dig('errors')).to be nil + expect(result.dig('data', 'addConversationMessage', 'conversationMessage')).to be nil + expect( + result.dig('data', 'addConversationMessage', 'errors', 0, 'message') + ).to eq 'Course concluded, unable to send messages' + end + + it 'should allow new messages in concluded courses for teachers' do + conversation(users: [@teacher]) + @course.update!(workflow_state: 'completed') + + result = run_mutation({conversation_id: @conversation.conversation_id, body: 'I have the power', recipients: [@student.id.to_s]}, @teacher) + expect(result.dig('errors')).to be nil + expect( + result.dig('data', 'addConversationMessage', 'conversationMessage', 'body') + ).to eq 'I have the power' + end +end