Merge pull request #86 from berkeley-cdss/staging

Staging

Author: cycomachead

.tool-versions

@@ -1 +1 @@
-ruby 3.3.9
+ruby 3.3.8

app/facades/canvas_facade.rb

@@ -84,7 +84,8 @@ def auth_header
   # Enable this to automatically parse JSON responses.
   # This will require some refactoring (and rebuilding VCRs/webmock)
   # do |faraday|
-  #     faraday.request :json
+  #     faraday.request :url_encoded # passed first, only affects request parameters.
+  #     faraday.request :json # 2nd, only affects request body
   #     faraday.response :json, content_type: /\bjson$/
   #     faraday.adapter Faraday.default_adapter
   # end
@@ -125,23 +126,41 @@ def depaginate_response(response)
     next_page = links.find { |page| page[:rel] == 'next' }
     return JSON.parse(response.body) if next_page.nil?
 
-    # raise CanvasAPIError, "Canvas API Error: #{response.status} - #{response.body}" if response.status != 200
-
     # NOTE: Do not log the full :url as it contains an auth token (from canvas)
     JSON.parse(response.body) + depaginate_response(@canvas_conn.get(next_page[:url], headers: auth_header))
   end
 
   ##
   # Gets all courses for the authorized user.
   #
-  # @return [Faraday::Response] list of the courses the user has access to.
+  # @return [Array<Hash>] list of the Course (hashes) the user has access to.
   def get_all_courses
     depaginate_response(@canvas_conn.get('courses', {
       per_page: 100,
       'include[]': 'term'
     }))
   end
 
+  ##
+  # Get all enrollments for a course.
+  #
+  # https://ucberkeleysandbox.instructure.com/doc/api/courses.html#method.courses.users
+  # @param  [Course] A Course object.
+  # @param  [String|Array<String>] role the role to filter users by (optional).
+  # @return [Array<Hash>] list of the Enrollment (hashes) in the course.
+  def get_all_course_users(course, role = nil)
+    # sigh, manually construct query string until we tweak Faraday middleware
+    # to include :url_encoded, then use `'enrollment_type[]' : list_or_string`
+    query_string = 'per_page=100'
+    query_string += "&enrollment_type[]=#{role}" if role.is_a?(String) && role.present?
+
+    if role.is_a?(Array) && role.present? # rubocop:disable Style/IfUnlessModifier
+      query_string += role.map { |r| "&enrollment_type[]=#{r}" }.join
+    end
+
+    depaginate_response(@canvas_conn.get("courses/#{course.canvas_id}/users?#{query_string}"))
+  end
+
   ##
   # Get all courses for which the user is an instructor.
   # Makes 2 API calls to Canvas, one for `teacher` and one for `ta`.

app/javascript/controllers/enrollments_controller.js

@@ -5,7 +5,7 @@ import "datatables.net-responsive-bs5";
 
 export default class extends Controller {
 	static values = { courseId: Number }
-	
+
 	connect() {
 		if (!DataTable.isDataTable('#enrollments-table')) {
 			// Define a custom sorting function for the Role column
@@ -48,7 +48,7 @@ export default class extends Controller {
 		})
 		  .then((response) => {
 			if (!response.ok) {
-			  throw new Error("Failed to sync enrollments.");
+			  throw new Error(`Failed to sync enrollments. ${response.status} - ${response.statusText}`);
 			}
 			return response.json();
 		  })
@@ -61,4 +61,4 @@ export default class extends Controller {
 			location.reload();
 		});
 	  }
-}
+}

app/models/course.rb

@@ -176,42 +176,25 @@ def self.extract_date_field(assignment_data, field_name)
     end
   end
 
-  # Fetch users for a course from Canvas API
-  # TODO: Replace with call to Canvas Facade
-  def fetch_users_from_canvas(token, enrollment_type = nil)
-    url = "#{ENV.fetch('CANVAS_URL')}/api/v1/courses/#{canvas_id}/users"
-    response = Faraday.get(url) do |req|
-      req.headers['Authorization'] = "Bearer #{token}"
-      req.headers['Content-Type'] = 'application/json'
-      req.params['enrollment_type'] = enrollment_type if enrollment_type.present?
-    end
-
-    if response.success?
-      JSON.parse(response.body)
-    else
-      Rails.logger.error "Failed to fetch users: #{response.status} - #{response.body}"
-      []
-    end
-  end
-
   # Fetch users for a course and create/find their User and UserToCourse records
+  # TODO: This may need to become a background job
   def sync_users_from_canvas(token, role)
     # Fetch all users for the course from Canvas
-    users_data = fetch_users_from_canvas(token, role)
+    canvas_users = CanvasFacade.new(token).get_all_course_users(self, role)
 
     # Extract the Canvas user IDs from the fetched data
-    current_canvas_user_ids = users_data.pluck('id')
+    current_canvas_user_ids = canvas_users.pluck('id')
 
+    # Delete UserToCourse records for users no longer in the course
     # Find all UserToCourse records for this course and role
     existing_user_to_courses = UserToCourse.where(course_id: id, role: role)
-
-    # Delete UserToCourse records for users no longer in the course
-    existing_user_to_courses.each do |user_to_course|
-      user_to_course.destroy unless current_canvas_user_ids.include?(user_to_course.user.canvas_uid)
-    end
+    user_ids_to_remove = existing_user_to_courses.reject do |utc|
+      current_canvas_user_ids.include?(utc.user.canvas_uid)
+    end.map(&:id)
+    UserToCourse.where(id: user_ids_to_remove).destroy_all if user_ids_to_remove.any?
 
     # Create or update User and UserToCourse records for current users
-    users_data.each do |user_data|
+    canvas_users.each do |user_data|
       # this line skips importing a user if the api doesn't return their email
       # one case this is happening if the user was invited to the course but hasn't accepted
       next if user_data['email'].blank?
@@ -230,9 +213,9 @@ def sync_users_from_canvas(token, role)
   end
 
   def sync_enrollments_from_canvas(token)
-    sync_users_from_canvas(token, 'student')
     sync_users_from_canvas(token, 'teacher')
     sync_users_from_canvas(token, 'ta')
+    sync_users_from_canvas(token, 'student')
   end
 
   def regenerate_readonly_api_token_if_blank

spec/controllers/courses_controller_spec.rb

@@ -91,17 +91,14 @@
     before do
       roles = %w[teacher ta student]
       roles.each do |role|
-        stub_request(:get, "#{ENV.fetch('CANVAS_URL', nil)}/api/v1/courses/456/users?enrollment_type=#{role}")
+        stub_request(:get, "#{ENV.fetch('CANVAS_URL', nil)}/api/v1/courses/456/users")
           .with(
-            headers: {
-              'Accept' => '*/*',
-              'Accept-Encoding' => 'gzip;q=1.0,deflate;q=0.6,identity;q=0.3',
-              'Authorization' => 'Bearer fake_token',
-              'Content-Type' => 'application/json',
-              'User-Agent' => 'Faraday v2.12.2'
-            }
-          )
-          .to_return(status: 200, body: '[]', headers: {})
+            query: {
+              'enrollment_type[]' => role,
+              'per_page' => '100'
+            },
+            headers: { 'Authorization' => 'Bearer fake_token' }
+          ).to_return(status: 200, body: '[]', headers: {})
       end
     end
 

spec/spec_helper.rb

@@ -43,6 +43,8 @@
     WebMock.disable_net_connect!(allow_localhost: true)
   end
 
+  config.include WebMock::API
+
   config.expect_with :rspec do |expectations|
     # This option will default to `true` in RSpec 4. It makes the `description`
     # and `failure_message` of custom matchers include text for helper methods