From 31957106b87e16c00d4fde3862b8c06d9025c097 Mon Sep 17 00:00:00 2001 From: Rob Orton Date: Fri, 11 Sep 2020 10:59:47 -0600 Subject: [PATCH] make notification endpoints multi-shard test plan - have user on shard1 with push notifications - merge into user on shard2 - push should work - deleting push channel should work fixes VICE-790 flag=none Change-Id: Ib98b907017bb8d5dcee18d0346c4240d8381cfaf Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/247468 Tested-by: Service Cloud Jenkins Reviewed-by: Cody Cutrer Product-Review: Cody Cutrer QA-Review: Rob Orton --- .../communication_channels_controller.rb | 4 +- app/models/user.rb | 4 +- .../v1/communication_channels_api_spec.rb | 45 ++++++++++++++----- .../communication_channels_controller_spec.rb | 16 +++++++ 4 files changed, 55 insertions(+), 14 deletions(-) diff --git a/app/controllers/communication_channels_controller.rb b/app/controllers/communication_channels_controller.rb index 3c9a4b6094a..30c73d18c86 100644 --- a/app/controllers/communication_channels_controller.rb +++ b/app/controllers/communication_channels_controller.rb @@ -165,7 +165,7 @@ class CommunicationChannelsController < ApplicationController end NotificationEndpoint.unique_constraint_retry do - unless @current_user.notification_endpoints.where("lower(token) = lower(?)", params[:communication_channel][:token]).exists? + unless @access_token.notification_endpoints.where("lower(token) = lower(?)", params[:communication_channel][:token]).exists? @access_token.notification_endpoints.create!(token: params[:communication_channel][:token]) end end @@ -530,7 +530,7 @@ class CommunicationChannelsController < ApplicationController @cc = @current_user.communication_channels.unretired.of_type(CommunicationChannel::TYPE_PUSH).take raise ActiveRecord::RecordNotFound unless @cc - endpoints = @current_user.notification_endpoints.where("lower(token) = ?", params[:push_token].downcase) + endpoints = @current_user.notification_endpoints.shard(@current_user).where("lower(token) = ?", params[:push_token].downcase) if endpoints&.destroy_all @current_user.touch return render json: {success: true} diff --git a/app/models/user.rb b/app/models/user.rb index 8872961d239..76d76ccecee 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -106,9 +106,9 @@ class User < ActiveRecord::Base has_many :associated_root_accounts, -> { order("user_account_associations.depth"). where(accounts: { parent_account_id: nil }) }, source: :account, through: :user_account_associations has_many :developer_keys - has_many :access_tokens, -> { where(:workflow_state => "active").preload(:developer_key) }, inverse_of: :user + has_many :access_tokens, -> { where(:workflow_state => "active").preload(:developer_key) }, inverse_of: :user, multishard: true has_many :masquerade_tokens, -> { where(:workflow_state => "active").preload(:developer_key) }, class_name: 'AccessToken', inverse_of: :real_user - has_many :notification_endpoints, :through => :access_tokens + has_many :notification_endpoints, :through => :access_tokens, multishard: true has_many :context_external_tools, -> { order(:name) }, as: :context, inverse_of: :context, dependent: :destroy has_many :lti_results, inverse_of: :user, class_name: 'Lti::Result', dependent: :destroy diff --git a/spec/apis/v1/communication_channels_api_spec.rb b/spec/apis/v1/communication_channels_api_spec.rb index d54dae3fffe..a70ea2cb70c 100644 --- a/spec/apis/v1/communication_channels_api_spec.rb +++ b/spec/apis/v1/communication_channels_api_spec.rb @@ -150,6 +150,7 @@ describe 'CommunicationChannels API', type: :request do it "doesn't error if the user already has a login with the same e-mail address" do @someone.pseudonyms.create!(unique_id: 'new+api@example.com') api_call(:post, @path, @path_options, @post_params.merge(skip_confirmation: 1)) + expect(response).to be_successful end context 'a site admin' do @@ -217,21 +218,28 @@ describe 'CommunicationChannels API', type: :request do expect(response.code).to eql '401' end - context 'push' do - before { @post_params.merge!(communication_channel: {token: 'registration_token', type: 'push'}) } - + context 'not configured push' do it 'should complain about sns not being configured' do + @post_params.merge!(communication_channel: {token: 'registration_token', type: 'push'}) raw_api_call(:post, @path, @path_options, @post_params) expect(response.code).to eql '400' end + end - it "should work" do - client = double() - allow(DeveloperKey).to receive(:sns).and_return(client) + context 'push' do + before { @post_params.merge!(communication_channel: {token: 'registration_token', type: 'push'}) } + + let(:client) { double() } + let(:dk) do dk = DeveloperKey.default dk.sns_arn = 'apparn' dk.save! + dk + end + + it "should work" do + allow(DeveloperKey).to receive(:sns).and_return(client) $spec_api_tokens[@user] = @user.access_tokens.create!(developer_key: dk).full_token expect(client).to receive(:create_platform_endpoint).once.and_return(endpoint_arn: 'endpointarn') @@ -242,11 +250,7 @@ describe 'CommunicationChannels API', type: :request do end it "shouldn't create two push channels regardless of case" do - client = double() allow(DeveloperKey).to receive(:sns).and_return(client) - dk = DeveloperKey.default - dk.sns_arn = 'apparn' - dk.save! $spec_api_tokens[@user] = @user.access_tokens.create!(developer_key: dk).full_token expect(client).to receive(:create_platform_endpoint).once.and_return(endpoint_arn: 'endpointarn') @post_params[:communication_channel][:token].upcase! @@ -255,6 +259,27 @@ describe 'CommunicationChannels API', type: :request do api_call(:post, @path, @path_options, @post_params) expect(@user.notification_endpoints.count).to eq 1 end + + context 'shards' do + specs_require_sharding + + it "should not have unique constraint error for push channel" do + allow(DeveloperKey).to receive(:sns).and_return(client) + $spec_api_tokens[@user] = @user.access_tokens.create!(developer_key: dk).full_token + expect(client).to receive(:create_platform_endpoint).once.and_return(endpoint_arn: 'endpointarn') + api_call(:post, @path, @path_options, @post_params) + @shard1.activate { @new_user = User.create!(name: 'shard one') } + # this is faster than a user_merge + @new_user.associate_with_shard(Shard.current) + @user.access_tokens.update_all(user_id: @new_user.id) + $spec_api_tokens[@new_user] = $spec_api_tokens[@user] + @user = @new_user + @path_options[:user_id] = @user.id + api_call(:post, "/api/v1/users/#{@user.id}/communication_channels", @path_options, @post_params) + expect(response).to be_successful + end + end + end end end diff --git a/spec/controllers/communication_channels_controller_spec.rb b/spec/controllers/communication_channels_controller_spec.rb index dd44224e4e6..6f205f642a2 100644 --- a/spec/controllers/communication_channels_controller_spec.rb +++ b/spec/controllers/communication_channels_controller_spec.rb @@ -1387,6 +1387,22 @@ describe CommunicationChannelsController do let(:fake_token) { 'insttothemoon' } before(:each) { sns_access_token.notification_endpoints.create!(token: fake_token) } + context "cross-shard user" do + specs_require_sharding + + it 'should delete endpoints from all_shards', type: :request do + @shard1.activate { @new_user = User.create!(name: 'shard one') } + UserMerge.from(@user).into(@new_user) + @user = @new_user + json = api_call(:delete, "/api/v1/users/self/communication_channels/push", + {controller: 'communication_channels', action: 'delete_push_token', format: 'json', + push_token: fake_token}, {push_token: fake_token}) + expect(json['success']).to eq true + endpoints = @user.notification_endpoints.shard(@user).where("lower(token) = ?", fake_token) + expect(endpoints.length).to eq 0 + end + end + it 'should delete a push_token', type: :request do json = api_call(:delete, "/api/v1/users/self/communication_channels/push", {controller: 'communication_channels', action: 'delete_push_token', format: 'json',