From be4f2b790d13fc90ec734a339f2ebc36b0de902d Mon Sep 17 00:00:00 2001
From: Sebastian Serth <Sebastian.Serth@hpi.de>
Date: Sun, 20 Aug 2023 17:59:24 +0200
Subject: [PATCH] Add user to testrun

* We want to identify a user that has triggered a testrun. Previously (in regular operation), only submission author who were regular users were able to start a testrun. Now, we want to prepare a future where submission authors are programming groups. Still, a testrun is triggered by an individual user and not a group.
* Further, this commit fixes some missing foreign key constrains.
---
 app/controllers/exercises_controller.rb       |  2 +-
 .../remote_evaluation_controller.rb           |  2 +-
 .../request_for_comments_controller.rb        |  2 +-
 app/controllers/submissions_controller.rb     |  5 +++--
 app/models/programming_group.rb               |  1 +
 app/models/submission.rb                      | 11 +++++-----
 app/models/user.rb                            |  1 +
 ...30037_require_contributor_on_submission.rb |  8 +++++++
 ...710130038_require_submission_on_testrun.rb |  8 +++++++
 .../20230710130039_add_user_to_testrun.rb     | 22 +++++++++++++++++++
 db/schema.rb                                  | 10 ++++++---
 spec/models/submission_spec.rb                |  4 ++--
 12 files changed, 61 insertions(+), 15 deletions(-)
 create mode 100644 db/migrate/20230710130037_require_contributor_on_submission.rb
 create mode 100644 db/migrate/20230710130038_require_submission_on_testrun.rb
 create mode 100644 db/migrate/20230710130039_add_user_to_testrun.rb

diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb
index 3339a775..da423418 100644
--- a/app/controllers/exercises_controller.rb
+++ b/app/controllers/exercises_controller.rb
@@ -525,7 +525,7 @@ class ExercisesController < ApplicationController
 
   def submit
     @submission = Submission.create(submission_params)
-    @submission.calculate_score
+    @submission.calculate_score(current_user)
 
     if @submission.users.map {|user| lti_outcome_service?(@submission.exercise, user, @submission.study_group_id) }.any?
       transmit_lti_score
diff --git a/app/controllers/remote_evaluation_controller.rb b/app/controllers/remote_evaluation_controller.rb
index 0a5f5964..1c4b4b6b 100644
--- a/app/controllers/remote_evaluation_controller.rb
+++ b/app/controllers/remote_evaluation_controller.rb
@@ -66,7 +66,7 @@ class RemoteEvaluationController < ApplicationController
     validation_token = remote_evaluation_params[:validation_token]
     if (remote_evaluation_mapping = RemoteEvaluationMapping.find_by(validation_token:))
       @submission = Submission.create(build_submission_params(cause, remote_evaluation_mapping))
-      @submission.calculate_score
+      @submission.calculate_score(remote_evaluation_mapping.user)
     else
       # TODO: better output
       # TODO: check token expired?
diff --git a/app/controllers/request_for_comments_controller.rb b/app/controllers/request_for_comments_controller.rb
index b1392964..a9cb4414 100644
--- a/app/controllers/request_for_comments_controller.rb
+++ b/app/controllers/request_for_comments_controller.rb
@@ -157,7 +157,7 @@ class RequestForCommentsController < ApplicationController
         # execute the tests here and wait until they finished.
         # As the same runner is used for the score and test run, no parallelization is possible
         # A run is triggered from the frontend and does not need to be handled here.
-        @request_for_comment.submission.calculate_score
+        @request_for_comment.submission.calculate_score(current_user)
         format.json { render :show, status: :created, location: @request_for_comment }
       else
         format.html { render :new }
diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb
index f118a8ef..c9b658d4 100644
--- a/app/controllers/submissions_controller.rb
+++ b/app/controllers/submissions_controller.rb
@@ -249,7 +249,7 @@ class SubmissionsController < ApplicationController
     return true if disable_scoring
 
     # The score is stored separately, we can forward it to the client immediately
-    client_socket&.send_data(JSON.dump(@submission.calculate_score))
+    client_socket&.send_data(JSON.dump(@submission.calculate_score(current_user)))
     # To enable hints when scoring a submission, uncomment the next line:
     # send_hints(client_socket, StructuredError.where(submission: @submission))
   rescue Runner::Error => e
@@ -284,7 +284,7 @@ class SubmissionsController < ApplicationController
     return true if @embed_options[:disable_run]
 
     # The score is stored separately, we can forward it to the client immediately
-    client_socket&.send_data(JSON.dump(@submission.test(@file)))
+    client_socket&.send_data(JSON.dump(@submission.test(@file, current_user)))
   rescue Runner::Error => e
     extract_durations(e)
     send_and_store client_socket, {cmd: :status, status: :container_depleted}
@@ -396,6 +396,7 @@ class SubmissionsController < ApplicationController
       passed: @testrun[:passed],
       cause:,
       submission: @submission,
+      user: current_user,
       exit_code: @testrun[:exit_code], # might be nil, e.g., when the run did not finish
       status: @testrun[:status] || :failed,
       output: @testrun[:output].presence, # TODO: Remove duplicated saving of the output after creating TestrunMessages
diff --git a/app/models/programming_group.rb b/app/models/programming_group.rb
index deb340c9..9f5e7745 100644
--- a/app/models/programming_group.rb
+++ b/app/models/programming_group.rb
@@ -6,6 +6,7 @@ class ProgrammingGroup < ApplicationRecord
   has_many :programming_group_memberships, dependent: :destroy
   has_many :external_users, through: :programming_group_memberships, source_type: 'ExternalUser', source: :user
   has_many :internal_users, through: :programming_group_memberships, source_type: 'InternalUser', source: :user
+  has_many :testruns, through: :submissions
   belongs_to :exercise
 
   validate :group_size
diff --git a/app/models/submission.rb b/app/models/submission.rb
index 1d63afed..fd192bf6 100644
--- a/app/models/submission.rb
+++ b/app/models/submission.rb
@@ -141,7 +141,7 @@ class Submission < ApplicationRecord
 
   # @raise [Runner::Error] if the score could not be calculated due to a failure with the runner.
   #                        See the specific type and message for more details.
-  def calculate_score
+  def calculate_score(requesting_user)
     file_scores = nil
     # If prepared_runner raises an error, no Testrun will be created.
     prepared_runner do |runner, waiting_duration|
@@ -153,7 +153,7 @@ class Submission < ApplicationRecord
         output = run_test_file file, runner, waiting_duration
         # If the previous execution failed and there is at least one more test, we request a new runner.
         runner, waiting_duration = swap_runner(runner) if output[:status] == :timeout && index < assessment_number
-        score_file(output, file)
+        score_file(output, file, requesting_user)
       end
     end
     # We sort the files again, so that the linter tests are displayed last.
@@ -178,10 +178,10 @@ class Submission < ApplicationRecord
 
   # @raise [Runner::Error] if the file could not be tested due to a failure with the runner.
   #                        See the specific type and message for more details.
-  def test(file)
+  def test(file, requesting_user)
     prepared_runner do |runner, waiting_duration|
       output = run_test_file file, runner, waiting_duration
-      score_file output, file
+      score_file output, file, requesting_user
     rescue Runner::Error => e
       e.waiting_duration = waiting_duration
       raise
@@ -251,7 +251,7 @@ class Submission < ApplicationRecord
     }
   end
 
-  def score_file(output, file)
+  def score_file(output, file, requesting_user)
     assessor = Assessor.new(execution_environment:)
     assessment = assessor.assess(output)
     passed = ((assessment[:passed] == assessment[:count]) and (assessment[:score]).positive?)
@@ -264,6 +264,7 @@ class Submission < ApplicationRecord
     end
     testrun = Testrun.create(
       submission: self,
+      user: requesting_user,
       cause: 'assess', # Required to differ run and assess for RfC show
       file:, # Test file that was executed
       passed:,
diff --git a/app/models/user.rb b/app/models/user.rb
index 62744170..3b5b62a7 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -17,6 +17,7 @@ class User < ApplicationRecord
   has_many :participations, through: :submissions, source: :exercise, as: :user
   has_many :user_proxy_exercise_exercises, as: :user
   has_many :user_exercise_interventions, as: :user
+  has_many :testruns, as: :user
   has_many :interventions, through: :user_exercise_interventions
   has_many :remote_evaluation_mappings, as: :user
   has_one :codeharbor_link, dependent: :destroy
diff --git a/db/migrate/20230710130037_require_contributor_on_submission.rb b/db/migrate/20230710130037_require_contributor_on_submission.rb
new file mode 100644
index 00000000..da655f3a
--- /dev/null
+++ b/db/migrate/20230710130037_require_contributor_on_submission.rb
@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+class RequireContributorOnSubmission < ActiveRecord::Migration[7.0]
+  def change
+    change_column_null :submissions, :contributor_id, false
+    change_column_null :submissions, :contributor_type, false
+  end
+end
diff --git a/db/migrate/20230710130038_require_submission_on_testrun.rb b/db/migrate/20230710130038_require_submission_on_testrun.rb
new file mode 100644
index 00000000..b9f419be
--- /dev/null
+++ b/db/migrate/20230710130038_require_submission_on_testrun.rb
@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+class RequireSubmissionOnTestrun < ActiveRecord::Migration[7.0]
+  def change
+    change_column_null :testruns, :submission_id, false
+    add_foreign_key :testruns, :submissions
+  end
+end
diff --git a/db/migrate/20230710130039_add_user_to_testrun.rb b/db/migrate/20230710130039_add_user_to_testrun.rb
new file mode 100644
index 00000000..43820115
--- /dev/null
+++ b/db/migrate/20230710130039_add_user_to_testrun.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+class AddUserToTestrun < ActiveRecord::Migration[7.0]
+  def change
+    add_reference :testruns, :user, polymorphic: true, index: true
+
+    up_only do
+      # Since we do not have programming groups, we can assume that the user who triggered a testrun is the same as the author of the corresponding submission.
+      # For programming groups, this assumption is not valid (the author of a submission would the group, whereas an individual user would trigger the testrun).
+      execute <<~SQL.squish
+        UPDATE testruns
+        SET user_id   = submissions.contributor_id,
+            user_type = submissions.contributor_type
+        FROM submissions
+        WHERE submissions.id = testruns.submission_id;
+      SQL
+    end
+
+    change_column_null :testruns, :user_id, false
+    change_column_null :testruns, :user_type, false
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 775f1d70..22cc3410 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -473,11 +473,11 @@ ActiveRecord::Schema[7.0].define(version: 2023_08_19_084917) do
   create_table "submissions", id: :serial, force: :cascade do |t|
     t.integer "exercise_id"
     t.float "score"
-    t.integer "contributor_id"
+    t.integer "contributor_id", null: false
     t.datetime "created_at"
     t.datetime "updated_at"
     t.string "cause"
-    t.string "contributor_type"
+    t.string "contributor_type", null: false
     t.bigint "study_group_id"
     t.index ["contributor_id"], name: "index_submissions_on_contributor_id"
     t.index ["exercise_id"], name: "index_submissions_on_exercise_id"
@@ -528,7 +528,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_08_19_084917) do
     t.boolean "passed"
     t.text "output"
     t.integer "file_id"
-    t.integer "submission_id"
+    t.integer "submission_id", null: false
     t.datetime "created_at"
     t.datetime "updated_at"
     t.string "cause"
@@ -536,7 +536,10 @@ ActiveRecord::Schema[7.0].define(version: 2023_08_19_084917) do
     t.interval "waiting_for_container_time"
     t.integer "exit_code", limit: 2, comment: "No exit code is available in case of a timeout"
     t.integer "status", limit: 2, default: 0, null: false, comment: "Used as enum in Rails"
+    t.string "user_type", null: false
+    t.bigint "user_id", null: false
     t.index ["submission_id"], name: "index_testruns_on_submission_id"
+    t.index ["user_type", "user_id"], name: "index_testruns_on_user"
     t.check_constraint "exit_code >= 0 AND exit_code <= 255", name: "exit_code_constraint"
   end
 
@@ -616,6 +619,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_08_19_084917) do
   add_foreign_key "testrun_execution_environments", "execution_environments"
   add_foreign_key "testrun_execution_environments", "testruns"
   add_foreign_key "testrun_messages", "testruns"
+  add_foreign_key "testruns", "submissions"
   add_foreign_key "tips", "file_types"
   add_foreign_key "user_exercise_feedbacks", "submissions"
 end
diff --git a/spec/models/submission_spec.rb b/spec/models/submission_spec.rb
index 42685572..d55710aa 100644
--- a/spec/models/submission_spec.rb
+++ b/spec/models/submission_spec.rb
@@ -134,12 +134,12 @@ describe Submission do
       allow(runner).to receive(:attach_to_execution).and_return(1.0)
     end
 
-    after { submission.calculate_score }
+    after { submission.calculate_score(submission.contributor) }
 
     it 'executes every teacher-defined test file' do
       allow(submission).to receive(:combine_file_scores)
       submission.collect_files.select(&:teacher_defined_assessment?).each do |file|
-        expect(submission).to receive(:score_file).with(any_args, file)
+        expect(submission).to receive(:score_file).with(any_args, file, submission.contributor)
       end
     end