Skip to content

Commit

Permalink
change GradebookService to find an active user properly
Browse files Browse the repository at this point in the history
closes INTEROP-6110
flag=none

Change GradebookService to find the active user when the specified
was merged.

When user A is merged to user B, the name and roles API
(api/lti/courses/:course_id/names_and_roles) returns the user B (active)
with the lti_id of user A.

Using this lti_id, and attempting to grade a line item using the score
api (api/lti/courses/:course_id/line_items/:line_item_id/scores), an
unauthorization error is raised.

If the user is deleted we need to check to see if there has been a
user_merge. If there has been a merge, return the merged user, otherwise
we return `nil`, so we'll not not return the deleted user anymore.

test plan:
* In Course context, we need to have two users with Student enrollment;
* Accessing the name and roles API, we will see the two active members with
their lti_id;
* Merge one user in the other one (User A -> User B);
* Accessing the name and roles API, we will see the user B active with
the lti_id from user A;
* If the course doesn't have line items, we can use the line items endpoint
(api/lti/courses/:course_id/line_items) to create one;
* Accessing the scores API using the user A lti_id, the response of the
request should  be successful instead of unauthorized (as is happening
today);

Change-Id: I3d23b9ce96c725b91e920e73705ba952140090fb
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/255789
Tested-by: Service Cloud Jenkins <[email protected]>
Reviewed-by: Mysti Lilla <[email protected]>
QA-Review: Mysti Lilla <[email protected]>
Product-Review: Wagner Goncalves <[email protected]>
  • Loading branch information
Wagner Gonçalves committed Jan 14, 2021
1 parent 95fad76 commit 12619c3
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 2 deletions.
15 changes: 14 additions & 1 deletion app/controllers/lti/ims/concerns/gradebook_services.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,20 @@ def context
end

def user
@_user ||= User.where(lti_id: params[:userId]).where.not(lti_id: nil).or(User.where(id: user_id)).take
@user ||= begin
active_user = User.
active.
where(lti_id: params[:userId]).
where.not(lti_id: nil).
or(User.where(id: user_id)).
take

# If the user is an active user, we'll use it.
# If the user is a deleted user, we need to check if it was a merged user.
# If the user was merged, we'll return the merged user, otherwise we return `nil`.
# So, we won't return a deleted user anymore
active_user || context.user_past_lti_ids.find_by(user_lti_id: params[:userId])&.user
end
end

def pagination_args
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/lti/ims/scores_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ class ScoresController < ApplicationController
#
# @argument userId [Required, String]
# The lti_user_id or the Canvas user_id.
# Returns a 412 if user not found in Canvas or is not a student.
# Returns a 422 if user not found in Canvas or is not a student.
#
# @argument activityProgress [Required, String]
# Indicate to Canvas the status of the user towards the activity's completion.
Expand Down
39 changes: 39 additions & 0 deletions spec/controllers/lti/ims/concerns/gradebook_services_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,45 @@ def output
end
end

context 'when two students with enrollments were merged' do
let_once(:user_to_merge) { student_in_course(course: context).user }
let(:lti_id) { user_to_merge.lti_id }
let(:valid_params) do
{ course_id: context.id, userId: lti_id, line_item_id: line_item.id }
end

before do
user.enrollments.first.update!(workflow_state: 'active')
user_to_merge.enrollments.first.update!(workflow_state: 'active')

UserMerge.from(user_to_merge).into(user)
end

it 'successfuly find the active user using the user past lti id' do
get :index, params: valid_params

expect(response.code).to eq '200'
expect(parsed_response_body['user_id']).to eq user.id
end
end

context 'when student was deleted and it was not merged (is not a past user)' do
let(:lti_id) { user.lti_id }
let(:valid_params) do
{ course_id: context.id, userId: lti_id, line_item_id: line_item.id }
end

before do
user.update!(workflow_state: 'deleted')
end

it 'fails to find user' do
get :index, params: valid_params
expect(response.code).to eq '422'
expect(JSON.parse(response.body)['errors']['message']).to eq('User not found in course or is not a student')
end
end

context 'when line item does not exist' do
before { user.enrollments.first.update!(workflow_state: 'active') }

Expand Down

0 comments on commit 12619c3

Please sign in to comment.