diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 4874d845..1fb3c47a 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -16,13 +16,18 @@ class ApplicationController < ActionController::Base rescue_from ActionController::InvalidAuthenticityToken, with: :render_csrf_error def current_user - @current_user ||= ExternalUser.find_by(id: session[:external_user_id]) || - login_from_session || - login_from_other_sources || - login_from_authentication_token || - nil + @current_user ||= find_or_login_current_user&.store_current_study_group_id(session[:study_group_id]) end + def find_or_login_current_user + ExternalUser.find_by(id: session[:external_user_id]) || + login_from_session || + login_from_other_sources || + login_from_authentication_token || + nil + end + private :find_or_login_current_user + def require_user! raise Pundit::NotAuthorizedError unless current_user end diff --git a/app/controllers/concerns/lti.rb b/app/controllers/concerns/lti.rb index 6c931763..68dd8ceb 100644 --- a/app/controllers/concerns/lti.rb +++ b/app/controllers/concerns/lti.rb @@ -181,11 +181,7 @@ module Lti def set_current_user @current_user = ExternalUser.find_or_create_by(consumer_id: @consumer.id, external_id: @provider.user_id) - external_role = external_user_role(@provider) - internal_role = @current_user.role - desired_role = internal_role == 'admin' ? internal_role : external_role - # Update user with new information but change the role only if he is no admin user - @current_user.update(email: external_user_email(@provider), name: external_user_name(@provider), role: desired_role) + @current_user.update(email: external_user_email(@provider), name: external_user_name(@provider)) end private :set_current_user @@ -199,8 +195,9 @@ module Lti else StudyGroup.find_or_create_by(external_id: @provider.resource_link_id, consumer: @consumer) end - group.external_users << @current_user unless group.external_users.include? @current_user - group.save + + study_group_membership = StudyGroupMembership.find_or_create_by(study_group: group, user: @current_user) + study_group_membership.update(role: external_user_role(@provider)) session[:study_group_id] = group.id end diff --git a/app/controllers/internal_users_controller.rb b/app/controllers/internal_users_controller.rb index a5125fc6..0caa8091 100644 --- a/app/controllers/internal_users_controller.rb +++ b/app/controllers/internal_users_controller.rb @@ -32,7 +32,7 @@ class InternalUsersController < ApplicationController def create @user = InternalUser.new(internal_user_params) - @user.role = role_param if current_user.admin? + @user.platform_admin = platform_admin_param if current_user.admin? authorize! @user.send(:setup_activation) create_and_respond(object: @user) do @@ -77,10 +77,10 @@ class InternalUsersController < ApplicationController end private :internal_user_params - def role_param - params.require(:internal_user).permit(:role)[:role] + def platform_admin_param + params.require(:internal_user).permit(:platform_admin)[:platform_admin] end - private :role_param + private :platform_admin_param def new @user = InternalUser.new @@ -139,8 +139,7 @@ class InternalUsersController < ApplicationController # the form by another user. Otherwise, the update might fail if an # activation_token or password_reset_token is present @user.validate_password = current_user == @user - @user.role = role_param if current_user.admin? - + @user.platform_admin = platform_admin_param if current_user.admin? update_and_respond(object: @user, params: internal_user_params) end end diff --git a/app/models/internal_user.rb b/app/models/internal_user.rb index 32779e8a..9bf8d148 100644 --- a/app/models/internal_user.rb +++ b/app/models/internal_user.rb @@ -10,7 +10,6 @@ class InternalUser < User validates :email, presence: true, uniqueness: true validates :password, confirmation: true, if: -> { password_void? && validate_password? }, on: :update, presence: true validate :password_strength, if: -> { password_void? && validate_password? }, on: :update - validates :role, inclusion: {in: ROLES} def activated? activation_state == 'active' @@ -33,10 +32,6 @@ class InternalUser < User errors.add(:password, :weak) if result.score < 4 end - def teacher? - role == 'teacher' - end - def displayname name end diff --git a/app/models/user.rb b/app/models/user.rb index ce519fc2..8cb70288 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -3,7 +3,7 @@ class User < ApplicationRecord self.abstract_class = true - ROLES = %w[admin teacher learner].freeze + attr_reader :current_study_group_id belongs_to :consumer has_many :authentication_token, dependent: :destroy @@ -26,9 +26,7 @@ class User < ApplicationRecord joins(:study_group_memberships).where(study_group_memberships: {study_group_id: user.study_groups}) unless user.admin? } - ROLES.each do |role| - define_method("#{role}?") { try(:role) == role } - end + validates :platform_admin, boolean_presence: true def internal_user? is_a?(InternalUser) @@ -38,6 +36,30 @@ class User < ApplicationRecord is_a?(ExternalUser) end + def learner? + return true if current_study_group_id.nil? + + @learner ||= current_study_group_membership.exists?(role: :learner) && !platform_admin? + end + + def teacher? + @teacher ||= current_study_group_membership.exists?(role: :teacher) && !platform_admin? + end + + def admin? + @admin ||= platform_admin? + end + + def store_current_study_group_id(study_group_id) + @current_study_group_id = study_group_id + self + end + + def current_study_group_membership + # We use `where(...).limit(1)` instead of `find_by(...)` to allow query chaining + study_group_memberships.where(study_group: current_study_group_id).limit(1) + end + def to_s displayname end diff --git a/spec/factories/external_user.rb b/spec/factories/external_user.rb index d1439e1c..e6d5f0f2 100644 --- a/spec/factories/external_user.rb +++ b/spec/factories/external_user.rb @@ -7,5 +7,9 @@ FactoryBot.define do external_id { SecureRandom.uuid } generated_user_name singleton_external_user + member_of_study_group + transient do + teacher_in_study_group { false } + end end end diff --git a/spec/factories/internal_user.rb b/spec/factories/internal_user.rb index bdde51b0..b6166061 100644 --- a/spec/factories/internal_user.rb +++ b/spec/factories/internal_user.rb @@ -7,8 +7,12 @@ FactoryBot.define do email { 'admin@example.org' } generated_user_name password { 'admin' } - role { 'admin' } + platform_admin { true } singleton_internal_user + member_of_study_group + transient do + teacher_in_study_group { true } + end end factory :teacher, class: 'InternalUser' do @@ -17,8 +21,12 @@ FactoryBot.define do generated_email generated_user_name password { 'teacher' } - role { 'teacher' } + platform_admin { false } singleton_internal_user + member_of_study_group + transient do + teacher_in_study_group { true } + end end factory :learner, class: 'InternalUser' do @@ -27,8 +35,12 @@ FactoryBot.define do generated_email generated_user_name password { 'learner' } - role { 'learner' } + platform_admin { false } singleton_internal_user + member_of_study_group + transient do + teacher_in_study_group { false } + end end trait :activated_user do diff --git a/spec/factories/request_for_comment.rb b/spec/factories/request_for_comment.rb index 6b22cb58..f31b78e7 100644 --- a/spec/factories/request_for_comment.rb +++ b/spec/factories/request_for_comment.rb @@ -14,6 +14,7 @@ FactoryBot.define do after(:create) do |rfc| rfc.file = rfc.submission.files.first Comment.create(file: rfc.file, user: rfc.user, row: 1, text: "comment for rfc #{rfc.question}") + rfc.submission.study_group_id = rfc.user.current_study_group_id end end end diff --git a/spec/factories/shared_traits.rb b/spec/factories/shared_traits.rb index f9233cd6..1439d2ee 100644 --- a/spec/factories/shared_traits.rb +++ b/spec/factories/shared_traits.rb @@ -20,4 +20,17 @@ FactoryBot.define do initialize_with { klass.where(email: email).first_or_create } end end + + trait :member_of_study_group do + after(:create) do |user, evaluator| + # Do not create a study group if already passed + if user.study_groups.blank? + study_group = create(:study_group) + user.study_groups << study_group + end + + user.study_group_memberships.update(role: 'teacher') if evaluator.teacher_in_study_group + user.store_current_study_group_id(user.study_group_memberships.first) + end + end end diff --git a/spec/models/external_user_spec.rb b/spec/models/external_user_spec.rb index a99f00a7..0c5c81b1 100644 --- a/spec/models/external_user_spec.rb +++ b/spec/models/external_user_spec.rb @@ -36,4 +36,10 @@ describe ExternalUser do expect(build(:external_user).teacher?).to be false end end + + describe 'external_user has no current_study_group_id' do + it 'defaults to being a learner' do + expect(build(:external_user).learner?).to be true + end + end end diff --git a/spec/models/internal_user_spec.rb b/spec/models/internal_user_spec.rb index 82c525a2..6e784299 100644 --- a/spec/models/internal_user_spec.rb +++ b/spec/models/internal_user_spec.rb @@ -63,13 +63,9 @@ describe InternalUser do end end - it 'validates the domain of the role' do - user.update(role: 'Foo') - expect(user.errors[:role]).to be_present - end - - it 'validates the presence of a role' do - expect(user.errors[:role]).to be_present + it 'validates the presence of the platform_admin flag' do + user.update(platform_admin: nil) + expect(user.errors[:platform_admin]).to be_present end describe '#admin?' do diff --git a/spec/policies/execution_environment_policy_spec.rb b/spec/policies/execution_environment_policy_spec.rb index 5654f920..bfc5806a 100644 --- a/spec/policies/execution_environment_policy_spec.rb +++ b/spec/policies/execution_environment_policy_spec.rb @@ -62,14 +62,10 @@ describe ExecutionEnvironmentPolicy do expect(policy).to permit(build(:admin)) end - shared_examples 'it does not grant access' do |user| - it "does not grant access to a user with role #{user.role}" do - expect(policy).not_to permit(user) + it 'does not grant access to all other users' do + %i[external_user teacher].each do |factory_name| + expect(policy).not_to permit(build(factory_name)) end end - - %i[teacher external_user].each do |user| - include_examples 'it does not grant access', FactoryBot.build(user) # rubocop:disable RSpec/FactoryBot/SyntaxMethods - end end end