review points

app/models/user.rb

@@ -13,7 +13,7 @@ class User < ApplicationRecord
   has_many :user_proxy_exercise_exercises, as: :user
   has_many :user_exercise_interventions, as: :user
   has_many :interventions, through: :user_exercise_interventions
-  has_one :codeharbor_link
+  has_one :codeharbor_link, dependent: :destroy
   accepts_nested_attributes_for :user_proxy_exercise_exercises
 
 

app/policies/codeharbor_link_policy.rb

@@ -8,11 +8,11 @@ class CodeharborLinkPolicy < ApplicationPolicy
   end
 
   def new?
-    teacher?
+    teacher? || admin?
   end
 
   def create?
-    teacher?
+    teacher? || admin?
   end
 
   def edit?

app/services/proforma_service/import.rb

@@ -28,13 +28,9 @@ module ProformaService
 
     def base_exercise
       exercise = Exercise.find_by(uuid: @task.uuid)
-      if exercise
-        return exercise if ExercisePolicy.new(@user, exercise).update?
+      return exercise if exercise && ExercisePolicy.new(@user, exercise).update?
 
-        return Exercise.new(uuid: SecureRandom.uuid, unpublished: true)
-      end
-
-      Exercise.new(uuid: @task.uuid || SecureRandom.uuid, unpublished: true)
+      Exercise.new(uuid: @task.uuid, unpublished: true)
     end
 
     def import_multi

spec/policies/codeharbor_link_policy_spec.rb

@@ -18,13 +18,13 @@ describe CodeharborLinkPolicy do
   %i[new? create?].each do |action|
     permissions(action) do
       it 'grants access to teachers' do
-        expect(policy).to permit(FactoryBot.create(:teacher), codeharbor_link)
+        %i[teacher admin].each do |factory_name|
+          expect(policy).to permit(FactoryBot.create(factory_name), codeharbor_link)
+        end
       end
 
       it 'does not grant access to all other users' do
-        %i[external_user admin].each do |factory_name|
-          expect(policy).not_to permit(FactoryBot.create(factory_name), codeharbor_link)
-        end
+        expect(policy).not_to permit(FactoryBot.create(:external_user), codeharbor_link)
       end
     end
   end

spec/services/proforma_service/import_spec.rb

@@ -128,42 +128,14 @@ describe ProformaService::Import do
       it { is_expected.to be_an_equal_exercise_as exercise }
     end
 
-    # context 'when zip contains multiple tasks' do
-    #   let(:exporter) { ProformaService::ExportTasks.call(exercises: [exercise, exercise2]).string }
-
-    #   let(:exercise2) do
-    #     FactoryBot.create(:dummy,
-    #                       instruction: 'instruction2',
-    #                       execution_environment: execution_environment,
-    #                       exercise_files: [],
-    #                       tests: [],
-    #                       user: user)
-    #   end
-
-    #   it 'imports the exercises from zip containing multiple zips' do
-    #     expect(import_service).to all be_an(Exercise)
-    #   end
-
-    #   it 'imports the zip exactly how they were exported' do
-    #     expect(import_service).to all be_an_equal_exercise_as(exercise).or be_an_equal_exercise_as(exercise2)
-    #   end
-
-    #   context 'when a exercise has files and tests' do
-    #     let(:files) { [FactoryBot.build(:file), FactoryBot.build(:file, role: 'regular_file')] }
-    #     let(:tests) { FactoryBot.build_list(:codeharbor_test, 2) }
-
-    #     it 'imports the zip exactly how the were exported' do
-    #       expect(import_service).to all be_an_equal_exercise_as(exercise).or be_an_equal_exercise_as(exercise2)
-    #     end
-    #   end
-    # end
-
     context 'when task in zip has a different uuid' do
       let(:uuid) { SecureRandom.uuid }
       let(:new_uuid) { SecureRandom.uuid }
+      let(:imported_exercise) { import_service }
 
       before do
         exercise.update(uuid: new_uuid)
+        imported_exercise.save!
       end
 
       it 'creates a new Exercise' do
@@ -173,16 +145,18 @@ describe ProformaService::Import do
 
     context 'when task in zip has the same uuid and nothing has changed' do
       let(:uuid) { SecureRandom.uuid }
+      let(:imported_exercise) { import_service }
 
       it 'updates the old Exercise' do
-        expect(import_service.id).to be exercise.id
+        imported_exercise.save!
+        expect(imported_exercise.id).to be exercise.id
       end
 
       context 'when another user imports the exercise' do
         let(:import_user) { FactoryBot.create(:teacher) }
 
-        it 'creates a new Exercise' do
-          expect(import_service.id).not_to be exercise.id
+        it 'raises a validation error' do
+          expect { imported_exercise.save! } .to raise_error ActiveRecord::RecordInvalid
         end
       end
     end