From e3603758ef57f267abb7cf7b17bc30f82cb492ed Mon Sep 17 00:00:00 2001
From: Sebastian Serth <Sebastian.Serth@hpi.de>
Date: Sat, 19 Aug 2023 11:28:57 +0200
Subject: [PATCH] Refactor LTI parameters and add study group

* This change also requires that submissions in our test need to have a valid study group.
---
 app/controllers/concerns/lti.rb               | 22 ++++------
 app/controllers/exercises_controller.rb       |  2 +-
 .../programming_groups_controller.rb          |  2 +-
 .../remote_evaluation_controller.rb           |  2 +-
 app/controllers/sessions_controller.rb        |  6 +--
 app/helpers/lti_helper.rb                     | 18 +++++---
 app/models/external_user.rb                   |  1 +
 app/models/lti_parameter.rb                   |  9 ++--
 app/models/study_group.rb                     |  1 +
 app/views/exercises/_editor.html.slim         |  4 +-
 app/views/exercises/_editor_output.html.slim  |  2 +-
 ...84916_add_study_group_to_lti_parameters.rb |  7 +++
 .../20230819084917_unify_lti_parameters.rb    | 44 +++++++++++++++++++
 db/schema.rb                                  | 17 ++++---
 db/scripts/migrate_exercise.sql               |  2 +-
 spec/concerns/lti_spec.rb                     | 14 +++---
 spec/controllers/sessions_controller_spec.rb  |  7 +--
 spec/factories/lti_parameter.rb               |  8 +++-
 18 files changed, 114 insertions(+), 54 deletions(-)
 create mode 100644 db/migrate/20230819084916_add_study_group_to_lti_parameters.rb
 create mode 100644 db/migrate/20230819084917_unify_lti_parameters.rb

diff --git a/app/controllers/concerns/lti.rb b/app/controllers/concerns/lti.rb
index 22111b68..e33af02b 100644
--- a/app/controllers/concerns/lti.rb
+++ b/app/controllers/concerns/lti.rb
@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-require 'oauth/request_proxy/rack_request'
+require 'oauth/request_proxy/action_controller_request'
 
 module Lti
   extend ActiveSupport::Concern
@@ -21,7 +21,7 @@ module Lti
 
   # exercise_id.nil? ==> the user has logged out. All session data is to be destroyed
   # exercise_id.exists? ==> the user has submitted the results of an exercise to the consumer.
-  # Only the lti_parameters are deleted.
+  # Only the assignment of the programming group is deleted.
   def clear_lti_session_data(exercise_id = nil)
     if exercise_id.nil?
       session.delete(:external_user_id)
@@ -148,10 +148,7 @@ module Lti
 
   def send_score_for(submission, user)
     if user.consumer
-      lti_parameter = LtiParameter.where(consumers_id: user.consumer.id,
-        external_users_id: user.id,
-        exercises_id: submission.exercise_id).last
-
+      lti_parameter = user.lti_parameters.find_by(exercise: submission.exercise, study_group: submission.study_group)
       provider = build_tool_provider(consumer: user.consumer, parameters: lti_parameter&.lti_parameters)
     end
 
@@ -237,14 +234,13 @@ module Lti
 
   private :set_embedding_options
 
-  def store_lti_session_data(options = {})
-    lti_parameters = LtiParameter.find_or_create_by(consumers_id: options[:consumer].id,
-      external_users_id: current_user.id,
-      exercises_id: @exercise.id)
+  def store_lti_session_data(parameters)
+    @lti_parameters = LtiParameter.find_or_initialize_by(external_user: current_user,
+      study_group_id: session[:study_group_id],
+      exercise: @exercise)
 
-    lti_parameters.lti_parameters = options[:parameters].slice(*SESSION_PARAMETERS).permit!.to_h
-    lti_parameters.save!
-    @lti_parameters = lti_parameters
+    @lti_parameters.lti_parameters = parameters.slice(*SESSION_PARAMETERS).permit!.to_h
+    @lti_parameters.save!
 
     session[:external_user_id] = current_user.id
   end
diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb
index f223d2ab..3339a775 100644
--- a/app/controllers/exercises_controller.rb
+++ b/app/controllers/exercises_controller.rb
@@ -527,7 +527,7 @@ class ExercisesController < ApplicationController
     @submission = Submission.create(submission_params)
     @submission.calculate_score
 
-    if @submission.users.map {|user| user.external_user? && lti_outcome_service?(@submission.exercise_id, user.id) }.any?
+    if @submission.users.map {|user| lti_outcome_service?(@submission.exercise, user, @submission.study_group_id) }.any?
       transmit_lti_score
     else
       redirect_after_submit
diff --git a/app/controllers/programming_groups_controller.rb b/app/controllers/programming_groups_controller.rb
index c49bd67c..c2314bbb 100644
--- a/app/controllers/programming_groups_controller.rb
+++ b/app/controllers/programming_groups_controller.rb
@@ -13,7 +13,7 @@ class ProgrammingGroupsController < ApplicationController
     if existing_programming_group
       session[:pg_id] = existing_programming_group.id
       redirect_to(implement_exercise_path(@exercise),
-        notice: t("sessions.create_through_lti.session_#{lti_outcome_service?(@exercise.id, current_user.id) ? 'with' : 'without'}_outcome", consumer: @consumer))
+        notice: t("sessions.create_through_lti.session_#{lti_outcome_service?(@exercise, current_user) ? 'with' : 'without'}_outcome", consumer: @consumer))
     end
   end
 
diff --git a/app/controllers/remote_evaluation_controller.rb b/app/controllers/remote_evaluation_controller.rb
index a9eb78ad..0a5f5964 100644
--- a/app/controllers/remote_evaluation_controller.rb
+++ b/app/controllers/remote_evaluation_controller.rb
@@ -35,7 +35,7 @@ class RemoteEvaluationController < ApplicationController
 
   def try_lti
     # TODO: Need to consider and support programming groups
-    if !@submission.user.nil? && lti_outcome_service?(@submission.exercise_id, @submission.user.id)
+    if !@submission.user.nil? && lti_outcome_service?(@submission.exercise, @submission.user, @submission.study_group_id)
       lti_responses = send_scores(@submission)
       process_lti_response(lti_responses.first)
     else
diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
index 1f6745ca..4faf1663 100644
--- a/app/controllers/sessions_controller.rb
+++ b/app/controllers/sessions_controller.rb
@@ -17,7 +17,7 @@ class SessionsController < ApplicationController
   end
 
   def create_through_lti
-    store_lti_session_data(consumer: @consumer, parameters: params)
+    store_lti_session_data(params)
     store_nonce(params[:oauth_nonce])
     if params[:custom_redirect_target]
       redirect_to(URI.parse(params[:custom_redirect_target].to_s).path)
@@ -25,7 +25,7 @@ class SessionsController < ApplicationController
       redirect_to(new_exercise_programming_group_path(@exercise))
     else
       redirect_to(implement_exercise_path(@exercise),
-        notice: t("sessions.create_through_lti.session_#{lti_outcome_service?(@exercise.id, current_user.id) ? 'with' : 'without'}_outcome",
+        notice: t("sessions.create_through_lti.session_#{lti_outcome_service?(@exercise, current_user) ? 'with' : 'without'}_outcome",
           consumer: @consumer))
     end
   end
@@ -44,7 +44,7 @@ class SessionsController < ApplicationController
   def destroy_through_lti
     @submission = Submission.find(params[:submission_id])
     authorize(@submission, :show?)
-    lti_parameter = LtiParameter.where(external_users_id: current_user.id, exercises_id: @submission.exercise_id).last
+    lti_parameter = current_user.lti_parameters.find_by(exercise: @submission.exercise, study_group_id: current_user.current_study_group_id)
     @url = consumer_return_url(build_tool_provider(consumer: current_user.consumer, parameters: lti_parameter&.lti_parameters))
 
     clear_lti_session_data(@submission.exercise_id)
diff --git a/app/helpers/lti_helper.rb b/app/helpers/lti_helper.rb
index e092ca93..444f0ddd 100644
--- a/app/helpers/lti_helper.rb
+++ b/app/helpers/lti_helper.rb
@@ -1,13 +1,17 @@
 # frozen_string_literal: true
 
-require 'oauth/request_proxy/action_controller_request' # Rails 5 changed `Rack::Request` to `ActionDispatch::Request`
-
 module LtiHelper
-  def lti_outcome_service?(exercise_id, external_user_id)
-    return false if external_user_id == ''
+  # Checks for support of the LTI outcome service for the given exercise and user.
+  # If the user passed is not the `current_user`, a study group id **must** be passed as well.
+  def lti_outcome_service?(exercise, user, study_group_id = user.current_study_group_id)
+    return false unless user.external_user?
 
-    lti_parameters = LtiParameter.where(external_users_id: external_user_id,
-      exercises_id: exercise_id).lis_outcome_service_url?.last
-    !lti_parameters.nil? && lti_parameters.present?
+    lis_outcome_service_parameters(exercise, user, study_group_id).present?
+  end
+
+  private
+
+  def lis_outcome_service_parameters(exercise, external_user, study_group_id)
+    external_user.lti_parameters.lis_outcome_service_url?.find_by(exercise:, study_group_id:)
   end
 end
diff --git a/app/models/external_user.rb b/app/models/external_user.rb
index 25df7be7..aabc7369 100644
--- a/app/models/external_user.rb
+++ b/app/models/external_user.rb
@@ -2,6 +2,7 @@
 
 class ExternalUser < User
   validates :external_id, presence: true
+  has_many :lti_parameters, dependent: :destroy
 
   def displayname
     name.presence || "User #{id}"
diff --git a/app/models/lti_parameter.rb b/app/models/lti_parameter.rb
index fe39dc75..767f7b87 100644
--- a/app/models/lti_parameter.rb
+++ b/app/models/lti_parameter.rb
@@ -1,9 +1,12 @@
 # frozen_string_literal: true
 
 class LtiParameter < ApplicationRecord
-  belongs_to :consumer, foreign_key: 'consumers_id'
-  belongs_to :exercise, foreign_key: 'exercises_id'
-  belongs_to :external_user, foreign_key: 'external_users_id'
+  belongs_to :exercise
+  belongs_to :external_user
+  belongs_to :study_group, optional: true
+  delegate :consumer, to: :external_user
+
+  validates :external_user_id, uniqueness: {scope: %i[study_group_id exercise_id]}
 
   scope :lis_outcome_service_url?, lambda {
     where("lti_parameters.lti_parameters ? 'lis_outcome_service_url'")
diff --git a/app/models/study_group.rb b/app/models/study_group.rb
index 552b3006..dac8c1f5 100644
--- a/app/models/study_group.rb
+++ b/app/models/study_group.rb
@@ -8,6 +8,7 @@ class StudyGroup < ApplicationRecord
   has_many :remote_evaluation_mappings, dependent: :nullify
   has_many :subscriptions, dependent: :nullify
   has_many :authentication_tokens, dependent: :nullify
+  has_many :lti_parameters, dependent: :delete_all
   belongs_to :consumer
 
   def users
diff --git a/app/views/exercises/_editor.html.slim b/app/views/exercises/_editor.html.slim
index 8143e873..a2e3909b 100644
--- a/app/views/exercises/_editor.html.slim
+++ b/app/views/exercises/_editor.html.slim
@@ -1,6 +1,4 @@
 - external_user_external_id = current_user.respond_to?(:external_id) ? current_user.external_id : ''
-- external_user_id = current_user.respond_to?(:external_id) ? current_user.id : ''
-- consumer_id = current_user.respond_to?(:external_id) ? current_user.consumer_id : ''
 - show_break_interventions = @show_break_interventions || "false"
 - show_rfc_interventions = @show_rfc_interventions || "false"
 - show_tips_interventions = @show_tips_interventions || "false"
@@ -53,7 +51,7 @@
           = t('exercises.editor.start_over_active_file')
 
   - unless @embed_options[:disable_run] && @embed_options[:disable_score]
-    div id='output_sidebar' class='output-col-collapsed' = render('exercises/editor_output', external_user_id: external_user_id, consumer_id: consumer_id )
+    div id='output_sidebar' class='output-col-collapsed' = render('exercises/editor_output')
 
 
 = render('shared/modal', id: 'comment-modal', title: t('exercises.implement.comment.request'), template: 'exercises/_request_comment_dialogcontent') unless @embed_options[:disable_rfc]
diff --git a/app/views/exercises/_editor_output.html.slim b/app/views/exercises/_editor_output.html.slim
index e37122d0..b1e0e75c 100644
--- a/app/views/exercises/_editor_output.html.slim
+++ b/app/views/exercises/_editor_output.html.slim
@@ -60,7 +60,7 @@ div.d-grid id='output_sidebar_uncollapsed' class='d-none col-sm-12 enforce-botto
           .progress-bar role='progressbar'
 
       br
-      - if lti_outcome_service?(@exercise.id, external_user_id)
+      - if lti_outcome_service?(@exercise, current_user)
         p.text-center = render('editor_button', classes: 'btn-lg btn-success d-none', data: {:'data-url' => submit_exercise_path(@exercise)}, icon: 'fa-solid fa-paper-plane', id: 'submit', label: t('exercises.editor.submit'))
         - if @exercise.submission_deadline.present? || @exercise.late_submission_deadline.present?
           #deadline data-submission-deadline=@exercise.submission_deadline&.rfc2822 data-late-submission-deadline=@exercise.late_submission_deadline&.rfc2822
diff --git a/db/migrate/20230819084916_add_study_group_to_lti_parameters.rb b/db/migrate/20230819084916_add_study_group_to_lti_parameters.rb
new file mode 100644
index 00000000..470e00cb
--- /dev/null
+++ b/db/migrate/20230819084916_add_study_group_to_lti_parameters.rb
@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddStudyGroupToLtiParameters < ActiveRecord::Migration[7.0]
+  def change
+    add_reference :lti_parameters, :study_group, index: true, null: true, foreign_key: true
+  end
+end
diff --git a/db/migrate/20230819084917_unify_lti_parameters.rb b/db/migrate/20230819084917_unify_lti_parameters.rb
new file mode 100644
index 00000000..61f82cc2
--- /dev/null
+++ b/db/migrate/20230819084917_unify_lti_parameters.rb
@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+class UnifyLtiParameters < ActiveRecord::Migration[7.0]
+  def change
+    reversible do |dir|
+      dir.up do
+        # We cannot add a foreign key to a table that has rows that violate the constraint.
+        LtiParameter.where(external_users_id: nil)
+          .or(LtiParameter.where.not(external_users_id: ExternalUser.all.select(:id)))
+          .or(LtiParameter.where(exercises_id: nil))
+          .or(LtiParameter.where.not(exercises_id: Exercise.all.select(:id)))
+          .delete_all
+
+        # For each user/exercise pair, keep the most recent LtiParameter.
+        LtiParameter.all.group(:external_users_id, :exercises_id).having('count(*) > 1').count.each do |ids, count|
+          LtiParameter.where(external_users_id: ids.first, exercises_id: ids.second).order(updated_at: :asc).limit(count - 1).delete_all
+        end
+        change_column :lti_parameters, :id, :bigint
+      end
+
+      dir.down do
+        change_column :lti_parameters, :id, :integer
+      end
+    end
+
+    remove_column :lti_parameters, :consumers_id, :bigint
+
+    rename_column :lti_parameters, :external_users_id, :external_user_id
+    change_column_null :lti_parameters, :external_user_id, false
+    add_foreign_key :lti_parameters, :external_users
+
+    rename_column :lti_parameters, :exercises_id, :exercise_id
+    change_column_null :lti_parameters, :exercise_id, false
+    add_foreign_key :lti_parameters, :exercises
+
+    add_index :lti_parameters, %i[external_user_id study_group_id exercise_id], unique: true, name: 'index_lti_params_on_external_user_and_study_group_and_exercise'
+  end
+
+  class LtiParameter < ActiveRecord::Base; end
+
+  class ExternalUser < ActiveRecord::Base; end
+
+  class Exercise < ActiveRecord::Base; end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 2d3779cc..775f1d70 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2023_07_27_080619) do
+ActiveRecord::Schema[7.0].define(version: 2023_08_19_084917) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
   enable_extension "pgcrypto"
@@ -341,14 +341,16 @@ ActiveRecord::Schema[7.0].define(version: 2023_07_27_080619) do
     t.string "severity"
   end
 
-  create_table "lti_parameters", id: :serial, force: :cascade do |t|
-    t.integer "external_users_id"
-    t.integer "consumers_id"
-    t.integer "exercises_id"
+  create_table "lti_parameters", force: :cascade do |t|
+    t.integer "external_user_id", null: false
+    t.integer "exercise_id", null: false
     t.jsonb "lti_parameters", default: {}, null: false
     t.datetime "created_at"
     t.datetime "updated_at"
-    t.index ["external_users_id"], name: "index_lti_parameters_on_external_users_id"
+    t.bigint "study_group_id"
+    t.index ["external_user_id", "study_group_id", "exercise_id"], name: "index_lti_params_on_external_user_and_study_group_and_exercise", unique: true
+    t.index ["external_user_id"], name: "index_lti_parameters_on_external_user_id"
+    t.index ["study_group_id"], name: "index_lti_parameters_on_study_group_id"
   end
 
   create_table "programming_group_memberships", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
@@ -599,6 +601,9 @@ ActiveRecord::Schema[7.0].define(version: 2023_07_27_080619) do
   add_foreign_key "exercise_tips", "exercise_tips", column: "parent_exercise_tip_id"
   add_foreign_key "exercise_tips", "exercises"
   add_foreign_key "exercise_tips", "tips"
+  add_foreign_key "lti_parameters", "exercises"
+  add_foreign_key "lti_parameters", "external_users"
+  add_foreign_key "lti_parameters", "study_groups"
   add_foreign_key "programming_group_memberships", "programming_groups"
   add_foreign_key "programming_groups", "exercises"
   add_foreign_key "remote_evaluation_mappings", "study_groups"
diff --git a/db/scripts/migrate_exercise.sql b/db/scripts/migrate_exercise.sql
index 8b685e88..eb913606 100644
--- a/db/scripts/migrate_exercise.sql
+++ b/db/scripts/migrate_exercise.sql
@@ -40,7 +40,7 @@ BEGIN
 
     DELETE FROM anomaly_notifications WHERE exercise_id = duplicated_exercise;
     DELETE FROM exercise_tags WHERE exercise_id = duplicated_exercise;
-    DELETE FROM lti_parameters WHERE exercises_id = duplicated_exercise;
+    DELETE FROM lti_parameters WHERE exercise_id = duplicated_exercise;
     DELETE FROM remote_evaluation_mappings WHERE exercise_id = duplicated_exercise;
 
     -- Preventing duplicated entries in exercise_collection_items
diff --git a/spec/concerns/lti_spec.rb b/spec/concerns/lti_spec.rb
index 85655e8a..ba1f73c0 100644
--- a/spec/concerns/lti_spec.rb
+++ b/spec/concerns/lti_spec.rb
@@ -108,7 +108,7 @@ describe Lti do
     let(:submission) { create(:submission) }
 
     before do
-      create(:lti_parameter, consumers_id: consumer.id, external_users_id: submission.contributor_id, exercises_id: submission.exercise_id)
+      create(:lti_parameter, external_user: submission.contributor, exercise: submission.exercise)
     end
 
     context 'with an invalid score' do
@@ -173,15 +173,15 @@ describe Lti do
       controller.instance_variable_set(:@current_user, create(:external_user))
       controller.instance_variable_set(:@exercise, create(:fibonacci))
       expect(controller.session).to receive(:[]=).with(:external_user_id, anything)
-      controller.send(:store_lti_session_data, consumer: build(:consumer), parameters:)
+      controller.send(:store_lti_session_data, parameters)
     end
 
     it 'creates an LtiParameter Object' do
-      before_count = LtiParameter.count
-      controller.instance_variable_set(:@current_user, create(:external_user))
-      controller.instance_variable_set(:@exercise, create(:fibonacci))
-      controller.send(:store_lti_session_data, consumer: build(:consumer), parameters:)
-      expect(LtiParameter.count).to eq(before_count + 1)
+      expect do
+        controller.instance_variable_set(:@current_user, create(:external_user))
+        controller.instance_variable_set(:@exercise, create(:fibonacci))
+        controller.send(:store_lti_session_data, parameters)
+      end.to change(LtiParameter, :count).by(1)
     end
   end
 
diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb
index 9a15f02e..943221a2 100644
--- a/spec/controllers/sessions_controller_spec.rb
+++ b/spec/controllers/sessions_controller_spec.rb
@@ -101,7 +101,6 @@ describe SessionsController do
       end
 
       it 'stores LTI parameters in the session' do
-        # Todo replace session with lti_parameter /should be done already
         expect(controller).to receive(:store_lti_session_data)
         perform_request
       end
@@ -191,7 +190,6 @@ describe SessionsController do
       end
 
       it 'clears the session' do
-        # Todo replace session with lti_parameter /should be done already
         expect(controller).to receive(:clear_lti_session_data)
         delete :destroy
       end
@@ -207,15 +205,12 @@ describe SessionsController do
     let(:submission) { create(:submission, exercise: create(:dummy)) }
 
     before do
-      # Todo replace session with lti_parameter
-      # Todo create LtiParameter Object
-      # session[:lti_parameters] = {}
+      create(:lti_parameter, external_user: submission.contributor)
       allow(controller).to receive(:current_user).and_return(submission.contributor)
       perform_request.call
     end
 
     it 'clears the session' do
-      # Todo replace session with lti_parameter /should be done already
       expect(controller).to receive(:clear_lti_session_data)
       perform_request.call
     end
diff --git a/spec/factories/lti_parameter.rb b/spec/factories/lti_parameter.rb
index 0eda270f..a8bde6fc 100644
--- a/spec/factories/lti_parameter.rb
+++ b/spec/factories/lti_parameter.rb
@@ -8,12 +8,18 @@ FactoryBot.define do
   }.freeze
 
   factory :lti_parameter do
-    consumer
     exercise factory: :math
     external_user
 
     lti_parameters { lti_params }
 
+    after(:create) do |lti_parameter|
+      # Do not change anything if a study group was provided explicitly or user has no study groups
+      next if lti_parameter.study_group.present? || lti_parameter.external_user.study_groups.blank?
+
+      lti_parameter.update!(study_group: lti_parameter.external_user.study_groups.first)
+    end
+
     trait :without_outcome_service_url do
       lti_parameters { lti_params.except(:lis_outcome_service_url) }
     end