Redefine user roles with their role in a study group

app/controllers/application_controller.rb

@@ -16,12 +16,17 @@ 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]) ||
+    @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

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
 

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

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

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

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

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

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

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

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

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

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)
-      end
-    end
-
-    %i[teacher external_user].each do |user|
-      include_examples 'it does not grant access', FactoryBot.build(user) # rubocop:disable RSpec/FactoryBot/SyntaxMethods
+    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
   end
 end