Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions app/controllers/courses_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ class CoursesController < ApplicationController
before_action :determine_user_role

def index
teacher_courses = UserToCourse.includes(:course).where(user: @user, role: %w[teacher ta])
teacher_courses = UserToCourse.includes(:course).where(user: @user, role: UserToCourse.staff_roles)
@teacher_courses_by_semester = group_by_semester(teacher_courses)

# Only show courses to students if extensions are enabled at the course level
Expand Down Expand Up @@ -44,16 +44,14 @@ def new
@courses = Course.fetch_courses(token)
flash[:alert] = 'No courses found.' if @courses.empty?

# Collect unique semester names from Canvas term data for the filter dropdown
@semesters = @courses.filter_map { |c| c.dig('term', 'name') }.uniq.sort
@selected_semester = params[:semester]

teacher_enrollment_types = %w[teacher ta]
# TODO: Add spec for when a course is created, but the user is not enrolled in it.
# TODO: Why do some courses have empty enrollments?
existing_canvas_ids = @user.courses.pluck(:canvas_id)
@courses_teacher = filter_courses(@courses, teacher_enrollment_types, existing_canvas_ids)
@courses_student = filter_courses(@courses, [ 'student' ], existing_canvas_ids)
@courses_teacher = filter_courses(@courses, UserToCourse.staff_roles, existing_canvas_ids)
@courses_student = filter_courses(@courses, [ UserToCourse::STUDENT_ROLE ], existing_canvas_ids)

if @selected_semester.present?
@courses_teacher = filter_by_semester(@courses_teacher, @selected_semester)
Expand All @@ -68,7 +66,7 @@ def edit

def create
token = @user.lms_credentials.first.token
filter_courses(Course.fetch_courses(token), %w[teacher ta])
filter_courses(Course.fetch_courses(token), UserToCourse.staff_roles)
.select { |c| params[:courses]&.include?(c['id'].to_s) }
.each { |course_api| Course.create_or_update_from_canvas(course_api, token, @user) }
redirect_to courses_path, notice: 'Selected courses and their assignments have been imported successfully.'
Expand Down Expand Up @@ -146,7 +144,9 @@ def filter_courses(courses, roles, exclude_ids = [])
courses = courses - missing_enrollments - courses.select { |course| exclude_ids.include?(course['id'].to_s) }
return [] if courses.empty?

courses.select { |course| course['enrollments'].any? { |e| roles.include?(e['type']) } }
courses.select do |course|
course['enrollments'].any? { |enrollment| roles.include?(UserToCourse.role_from_canvas_enrollment(enrollment)) }
end
end

def course_data_for_sync
Expand Down
19 changes: 17 additions & 2 deletions app/facades/canvas_facade.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require 'date'
require 'cgi'
require 'faraday'
require 'json'
require 'ostruct'
Expand All @@ -8,6 +9,9 @@ class CanvasFacade < LmsFacade
class CanvasAPIError < LmsFacade::LmsAPIError; end

CANVAS_URL = ENV.fetch('CANVAS_URL', nil)
CANVAS_CUSTOM_COURSE_ROLES = {
UserToCourse::LEAD_TA_ROLE => 'Lead TA'
}.freeze

# Canvas instances can scope the flextensions developer key.
# There must be one scope for each endpoint we can use.
Expand Down Expand Up @@ -161,10 +165,10 @@ 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?
query_string += "&#{role_query_param(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
query_string += role.map { |r| "&#{role_query_param(r)}" }.join
end

depaginate_response(@canvas_conn.get("courses/#{course.canvas_id}/users?#{query_string}"))
Expand All @@ -189,6 +193,17 @@ def get_instructor_courses
teacher_courses + ta_courses
end

def role_query_param(role)
normalized_role = UserToCourse.normalize_role(role)
canvas_course_role = CANVAS_CUSTOM_COURSE_ROLES[normalized_role]

if canvas_course_role
"enrollment_role=#{CGI.escape(canvas_course_role)}"
else
"enrollment_type[]=#{CGI.escape(normalized_role)}"
end
end

##
# Gets a specified course that the authorized user has access to.
#
Expand Down
2 changes: 1 addition & 1 deletion app/javascript/controllers/enrollments_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export default class extends Controller {
if (!DataTable.isDataTable('#enrollments-table')) {
// Define a custom sorting function for the Role column
DataTable.ext.type.order['role-pre'] = function (data) {
const rolePriority = { teacher: 4, ta: 2, student: 3 };
const rolePriority = { teacher: 4, leadta: 3, "lead ta": 3, ta: 2, student: 1 };
if (typeof data !== 'string') {
data = String(data).trim();
}
Expand Down
10 changes: 5 additions & 5 deletions app/models/course.rb
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ def enabled_assignments
# Or is user.staff_role?(course) or user.student_role?(course) better?
def user_role(user)
roles = UserToCourse.where(user_id: user.id, course_id: id).pluck(:role)
return 'instructor' if roles.include?('teacher') || roles.include?('ta')
return 'student' if roles.include?('student')
return 'instructor' if roles.intersect?(UserToCourse.staff_roles)
return 'student' if roles.include?(UserToCourse::STUDENT_ROLE)

nil
end
Expand Down Expand Up @@ -118,11 +118,11 @@ def assignments
end

def students
user_to_courses.where(role: 'student').map(&:user)
user_to_courses.where(role: UserToCourse::STUDENT_ROLE).map(&:user)
end

def instructors
user_to_courses.where(role: 'teacher').map(&:user)
user_to_courses.where(role: UserToCourse::TEACHER_ROLE).map(&:user)
end

def staff_users
Expand Down Expand Up @@ -257,7 +257,7 @@ def sync_users_from_canvas(user, roles = [ 'student' ])
end

def sync_all_enrollments_from_canvas(user)
sync_users_from_canvas(user, [ 'teacher', 'ta', 'student' ])
sync_users_from_canvas(user, UserToCourse.roles)
end

def regenerate_readonly_api_token_if_blank
Expand Down
48 changes: 44 additions & 4 deletions app/models/user_to_course.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,16 @@
#
# TODO: In the future we should name this CourseEnrollment
class UserToCourse < ApplicationRecord
STUDENT_ROLE = 'student'.freeze
TEACHER_ROLE = 'teacher'.freeze
TA_ROLE = 'ta'.freeze
LEAD_TA_ROLE = 'leadta'.freeze
STAFF_ROLES = [ TEACHER_ROLE, TA_ROLE, LEAD_TA_ROLE ].freeze
COURSE_ADMIN_ROLES = [ TEACHER_ROLE, LEAD_TA_ROLE ].freeze
ROLE_LABELS = {
LEAD_TA_ROLE => 'Lead TA'
}.freeze

# Associations
belongs_to :user
belongs_to :course
Expand All @@ -39,18 +49,48 @@ def staff?
end

def course_admin?
role == 'teacher' || role == 'leadta'
UserToCourse.course_admin_roles.include?(role)
end

def student?
role == 'student'
role == STUDENT_ROLE
end

def display_role
UserToCourse.display_role(role)
end

def self.roles
[ 'student' ] + UserToCourse.staff_roles
[ STUDENT_ROLE ] + UserToCourse.staff_roles
end

def self.staff_roles
%w[teacher ta leadta]
STAFF_ROLES
end

def self.course_admin_roles
COURSE_ADMIN_ROLES
end

def self.normalize_role(role)
role.to_s.downcase.gsub(/[^a-z]/, '')
end

def self.role_from_canvas_enrollment(enrollment)
return nil unless enrollment

normalized_role = normalize_role(enrollment['role'] || enrollment[:role])
return LEAD_TA_ROLE if normalized_role == LEAD_TA_ROLE

normalized_type = normalize_role(enrollment['type'] || enrollment[:type])
roles.include?(normalized_type) ? normalized_type : nil
end

def self.staff_enrollment?(enrollment)
staff_roles.include?(role_from_canvas_enrollment(enrollment))
end

def self.display_role(role)
ROLE_LABELS.fetch(role.to_s, role.to_s.capitalize)
end
end
4 changes: 2 additions & 2 deletions app/views/courses/enrollments.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
</td>
<td class="text-center"><%= enrollment.user.student_id %></td>
<td class="text-center"><%= enrollment.user.email %></td>
<td class="text-center"><%= enrollment.role.downcase.capitalize %></td>
<td class="text-center"><%= enrollment.display_role %></td>
<% if @is_course_admin %>
<td class="justify-content-center align-content-center"
data-order="<%= enrollment.allow_extended_requests ? 1 : 0 %>">
Expand Down Expand Up @@ -89,4 +89,4 @@
<% end %>
</div>
</div>
</div>
</div>
2 changes: 1 addition & 1 deletion app/views/courses/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
<tr>
<td><a href="/courses/<%= user_to_course.course.id %>"> <%= user_to_course.course.course_name %> </a></td>
<td><%= user_to_course.course.course_code %></td>
<td><%= user_to_course.role.capitalize %></td>
<td><%= user_to_course.display_role %></td>
</tr>
<% end %>
<% end %>
Expand Down
5 changes: 4 additions & 1 deletion app/views/courses/new.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@
<td><%= course.dig("term", "name") %></td>
<td><%= course["course_code"] %></td>
<td><%= course["name"] %></td>
<td><%= course['enrollments'].find { |enrollment| %w[teacher ta].include?(enrollment['type']) }['type'].capitalize %></td>
<td>
<% staff_enrollment = course['enrollments'].find { |enrollment| UserToCourse.staff_enrollment?(enrollment) } %>
<%= UserToCourse.display_role(UserToCourse.role_from_canvas_enrollment(staff_enrollment)) %>
</td>
</tr>
<% end %>
</tbody>
Expand Down
38 changes: 35 additions & 3 deletions spec/controllers/courses_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@
expect(response).to render_template(:index)
end

it 'includes Lead TA enrollments in staff courses' do
UserToCourse.create!(user: user, course: student_course, role: 'leadta')

get :index

expect(assigns(:teacher_courses).map(&:role)).to include('leadta')
end

context 'semester grouping' do
let(:spring_course) { Course.create!(course_name: 'Spring Course', canvas_id: 'sp1', course_code: 'SP101', semester: 'Spring 2026') }
let(:fall_course) { Course.create!(course_name: 'Fall Course', canvas_id: 'fa1', course_code: 'FA101', semester: 'Fall 2025') }
Expand Down Expand Up @@ -112,6 +120,21 @@
expect(response).to redirect_to(courses_path)
expect(flash[:notice]).to eq('Selected courses and their assignments have been imported successfully.')
end

it 'imports courses where the user is enrolled with the Canvas Lead TA role' do
lead_ta_course = {
'id' => '999',
'name' => 'Lead TA Canvas Course',
'course_code' => 'LTA101',
'enrollments' => [ { 'type' => 'ta', 'role' => 'Lead TA' } ]
}
allow(Course).to receive(:fetch_courses).and_return([ lead_ta_course ])
allow(Course).to receive(:create_or_update_from_canvas)

post :create, params: { courses: [ '999' ] }

expect(Course).to have_received(:create_or_update_from_canvas).with(lead_ta_course, 'fake_token', user)
end
end

describe 'POST #sync_assignments' do
Expand Down Expand Up @@ -139,6 +162,14 @@
headers: { 'Authorization' => 'Bearer fake_token' }
).to_return(status: 200, body: '[]', headers: {})
end
stub_request(:get, "#{ENV.fetch('CANVAS_URL', nil)}/api/v1/courses/456/users")
.with(
query: {
'enrollment_role' => 'Lead TA',
'per_page' => '100'
},
headers: { 'Authorization' => 'Bearer fake_token' }
).to_return(status: 200, body: '[]', headers: {})
end

context 'when user is a teacher (course admin)' do
Expand Down Expand Up @@ -215,7 +246,7 @@
'id' => '103',
'name' => 'Test Course 103',
'course_code' => 'TC103',
'enrollments' => [ { 'type' => 'teacher' } ],
'enrollments' => [ { 'type' => 'ta', 'role' => 'Lead TA' } ],
'term' => { 'name' => 'Fall 2025' }
},
{
Expand Down Expand Up @@ -246,8 +277,9 @@
expect(assigns(:courses_student)).not_to be_empty

# Teacher course should be categorized correctly
teacher_course = assigns(:courses_teacher).first
expect(teacher_course['enrollments'].first['type']).to eq('teacher')
teacher_course_roles = assigns(:courses_teacher).map { |canvas_course| canvas_course['enrollments'].first }
expect(teacher_course_roles).to include(hash_including('type' => 'teacher'))
expect(teacher_course_roles).to include(hash_including('role' => 'Lead TA'))

# Student course should be categorized correctly
student_course = assigns(:courses_student).first
Expand Down
18 changes: 18 additions & 0 deletions spec/facades/canvas_facade_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,24 @@
end
end

describe '#get_all_course_users' do
let(:course) { instance_double(Course, canvas_id: course_id) }

it 'uses enrollment_type for built-in Canvas roles' do
stubs.get("courses/#{course_id}/users?per_page=100&enrollment_type[]=ta") { [ 200, {}, '[]' ] }

expect(facade.get_all_course_users(course, 'ta')).to eq([])
stubs.verify_stubbed_calls
end

it 'uses enrollment_role for the custom Lead TA Canvas role' do
stubs.get("courses/#{course_id}/users?per_page=100&enrollment_role=Lead+TA") { [ 200, {}, '[]' ] }

expect(facade.get_all_course_users(course, 'leadta')).to eq([])
stubs.verify_stubbed_calls
end
end

describe 'get_course' do
before do
stubs.get("courses/#{course_id}") { [ 200, {}, '{}' ] }
Expand Down
20 changes: 20 additions & 0 deletions spec/models/course_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,16 @@
end
end

describe '#user_role' do
it 'treats leadta enrollments as instructors' do
course = described_class.create!(canvas_id: 'canvas_leadta', course_name: 'Test', course_code: 'TEST101')
user = User.create!(email: 'leadta@example.com', canvas_uid: 'leadta_123')
UserToCourse.create!(user: user, course: course, role: 'leadta')

expect(course.user_role(user)).to eq('instructor')
end
end

describe '.fetch_courses' do
it 'returns parsed JSON if response is successful' do
stub_request(:get, %r{api/v1/courses})
Expand Down Expand Up @@ -235,6 +245,16 @@
end
end

describe '#sync_all_enrollments_from_canvas' do
let!(:course) { described_class.create!(canvas_id: 'canvas_all_roles', course_name: 'User Sync', course_code: 'USYNC') }

it 'syncs every supported internal role, including leadta' do
expect(SyncUsersFromCanvasJob).to receive(:perform_now).with(course.id, 999, %w[student teacher ta leadta])

course.sync_all_enrollments_from_canvas(999)
end
end

describe '.create_or_update_from_canvas' do
let(:user) { create(:user, id: 999, canvas_uid: 'u1', name: 'User 1', email: 'user1@example.com') }

Expand Down
Loading
Loading