From be3ec82bd4d6e658d2e9594921b69f9110590355 Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Thu, 15 Oct 2020 00:43:57 +0200 Subject: [PATCH] Add new file role teacher_defined_linter --- app/assets/javascripts/editor/editor.js.erb | 2 +- app/assets/javascripts/exercises.js.erb | 2 +- .../concerns/submission_scoring.rb | 2 +- app/models/code_ocean/file.rb | 21 ++++++++++++------- app/models/exercise.rb | 6 +++--- .../convert_exercise_to_task.rb | 4 ++-- app/views/exercises/_file_form.html.slim | 2 +- app/views/exercises/index.html.slim | 2 +- app/views/shared/_file.html.slim | 2 +- config/locales/de.yml | 1 + config/locales/en.yml | 1 + lib/docker_client.rb | 7 +++++-- lib/py_unit_and_py_lint_adapter.rb | 9 ++++---- spec/concerns/submission_scoring_spec.rb | 2 +- .../submissions_controller_spec.rb | 2 +- spec/lib/docker_client_spec.rb | 2 +- .../convert_task_to_exercise_spec.rb | 2 ++ 17 files changed, 41 insertions(+), 28 deletions(-) diff --git a/app/assets/javascripts/editor/editor.js.erb b/app/assets/javascripts/editor/editor.js.erb index a58afae9..d6ef4c14 100644 --- a/app/assets/javascripts/editor/editor.js.erb +++ b/app/assets/javascripts/editor/editor.js.erb @@ -413,7 +413,7 @@ var CodeOceanEditor = { }, isActiveFileTestable: function () { - return this.isActiveFileExecutable() && ['teacher_defined_test', 'user_defined_test'].includes(this.active_frame.data('role')); + return this.isActiveFileExecutable() && ['teacher_defined_test', 'user_defined_test', 'teacher_defined_linter'].includes(this.active_frame.data('role')); }, isBrowserSupported: function () { diff --git a/app/assets/javascripts/exercises.js.erb b/app/assets/javascripts/exercises.js.erb index 000637e6..16a0dc78 100644 --- a/app/assets/javascripts/exercises.js.erb +++ b/app/assets/javascripts/exercises.js.erb @@ -195,7 +195,7 @@ $(document).on('turbolinks:load', function () { var observeFileRoleChanges = function () { $(document).on('change', 'select[name$="[role]"]', function () { - var is_test_file = $(this).val() === 'teacher_defined_test'; + var is_test_file = $(this).val() === 'teacher_defined_test' || $(this).val() === 'teacher_defined_linter'; var parent = $(this).parents('.card'); var fields = parent.find('.test-related-fields'); if (is_test_file) { diff --git a/app/controllers/concerns/submission_scoring.rb b/app/controllers/concerns/submission_scoring.rb index 131680cb..f8acd267 100644 --- a/app/controllers/concerns/submission_scoring.rb +++ b/app/controllers/concerns/submission_scoring.rb @@ -3,7 +3,7 @@ require 'concurrent/future' module SubmissionScoring def collect_test_results(submission) # Mnemosyne.trace 'custom.codeocean.collect_test_results', meta: { submission: submission.id } do - submission.collect_files.select(&:teacher_defined_test?).map do |file| + submission.collect_files.select(&:teacher_defined_assessment?).map do |file| future = Concurrent::Future.execute do # Mnemosyne.trace 'custom.codeocean.collect_test_results_block', meta: { file: file.id, submission: submission.id } do assessor = Assessor.new(execution_environment: submission.execution_environment) diff --git a/app/models/code_ocean/file.rb b/app/models/code_ocean/file.rb index 3ee30afa..35e42070 100644 --- a/app/models/code_ocean/file.rb +++ b/app/models/code_ocean/file.rb @@ -19,11 +19,11 @@ module CodeOcean include DefaultValues DEFAULT_WEIGHT = 1.0 - ROLES = %w(main_file reference_implementation regular_file executable_file teacher_defined_test user_defined_file user_defined_test) - TEACHER_DEFINED_ROLES = ROLES - %w(user_defined_file) + ROLES = %w[main_file reference_implementation regular_file executable_file teacher_defined_test user_defined_file user_defined_test teacher_defined_linter].freeze + TEACHER_DEFINED_ROLES = ROLES - %w[user_defined_file] after_initialize :set_default_values - before_validation :clear_weight, unless: :teacher_defined_test? + before_validation :clear_weight, unless: :teacher_defined_assessment? before_validation :hash_content, if: :content_present? before_validation :set_ancestor_values, if: :incomplete_descendent? @@ -45,19 +45,20 @@ module CodeOcean ROLES.each do |role| scope :"#{role}s", -> { where(role: role) } end + scope :teacher_defined_assessments, -> { where(role: %w[teacher_defined_test teacher_defined_linter]) } default_scope { order(name: :asc) } - validates :feedback_message, if: :teacher_defined_test?, presence: true - validates :feedback_message, absence: true, unless: :teacher_defined_test? + validates :feedback_message, if: :teacher_defined_assessment?, presence: true + validates :feedback_message, absence: true, unless: :teacher_defined_assessment? validates :file_type_id, presence: true validates :hashed_content, if: :content_present?, presence: true validates :hidden, boolean_presence: true validates :name, presence: true validates :read_only, boolean_presence: true validates :role, inclusion: {in: ROLES} - validates :weight, if: :teacher_defined_test?, numericality: true, presence: true - validates :weight, absence: true, unless: :teacher_defined_test? + validates :weight, if: :teacher_defined_assessment?, numericality: true, presence: true + validates :weight, absence: true, unless: :teacher_defined_assessment? validates :file, presence: true if :context.is_a?(Submission) validates_with FileNameValidator, fields: [:name, :path, :file_type_id] @@ -75,6 +76,10 @@ module CodeOcean end private :clear_weight + def teacher_defined_assessment? + teacher_defined_test? || teacher_defined_linter? + end + def content_present? content? || native_file? end @@ -111,7 +116,7 @@ module CodeOcean def set_default_values set_default_values_if_present(content: '', hidden: false, read_only: false) - set_default_values_if_present(weight: DEFAULT_WEIGHT) if teacher_defined_test? + set_default_values_if_present(weight: DEFAULT_WEIGHT) if teacher_defined_assessment? end private :set_default_values diff --git a/app/models/exercise.rb b/app/models/exercise.rb index 74e8a477..fd043d59 100644 --- a/app/models/exercise.rb +++ b/app/models/exercise.rb @@ -263,7 +263,7 @@ class Exercise < ApplicationRecord FROM files WHERE context_type = 'Exercise' AND context_id = #{id} - AND role = 'teacher_defined_test' + AND role IN ('teacher_defined_test', 'teacher_defined_linter') GROUP BY context_id), -- filter for rows containing max points time_max_score AS @@ -392,7 +392,7 @@ class Exercise < ApplicationRecord WHERE exercise_id = #{id} AND user_id = #{user.id} AND user_type = '#{user_type}' GROUP BY user_id, id, exercise_id), MAX_POINTS AS - (SELECT context_id AS ex_id, sum(weight) AS max_points FROM files WHERE context_type = 'Exercise' AND context_id = #{id} AND role = 'teacher_defined_test' GROUP BY context_id), + (SELECT context_id AS ex_id, sum(weight) AS max_points FROM files WHERE context_type = 'Exercise' AND context_id = #{id} AND role IN ('teacher_defined_test', 'teacher_defined_linter') GROUP BY context_id), -- filter for rows containing max points TIME_MAX_SCORE AS @@ -506,7 +506,7 @@ class Exercise < ApplicationRecord 0 end else - files.teacher_defined_tests.sum(:weight) + files.teacher_defined_assessments.sum(:weight) end end diff --git a/app/services/proforma_service/convert_exercise_to_task.rb b/app/services/proforma_service/convert_exercise_to_task.rb index 80b4b6a9..4937acd3 100644 --- a/app/services/proforma_service/convert_exercise_to_task.rb +++ b/app/services/proforma_service/convert_exercise_to_task.rb @@ -55,7 +55,7 @@ module ProformaService end def tests - @exercise.files.filter { |file| file.role == 'teacher_defined_test' }.map do |file| + @exercise.files.filter { |file| file.role == 'teacher_defined_test' || file.role == 'teacher_defined_linter' }.map do |file| Proforma::Test.new( id: file.id, title: file.name, @@ -78,7 +78,7 @@ module ProformaService def task_files @exercise.files - .filter { |file| !file.role.in? %w[reference_implementation teacher_defined_test] }.map do |file| + .filter { |file| !file.role.in? %w[reference_implementation teacher_defined_test teacher_defined_linter] }.map do |file| task_file(file) end end diff --git a/app/views/exercises/_file_form.html.slim b/app/views/exercises/_file_form.html.slim index ffb0def6..e5c0a5cd 100644 --- a/app/views/exercises/_file_form.html.slim +++ b/app/views/exercises/_file_form.html.slim @@ -37,7 +37,7 @@ li.card.mt-2 label.form-check-label = f.check_box(:read_only, class: 'form-check-input') = t('activerecord.attributes.file.read_only') - .test-related-fields style="display: #{f.object.teacher_defined_test? ? 'initial' : 'none'};" + .test-related-fields style="display: #{f.object.teacher_defined_assessment? ? 'initial' : 'none'};" .form-group = f.label(:name, t('activerecord.attributes.file.feedback_message')) = f.text_area(:feedback_message, class: 'form-control', maxlength: 255) diff --git a/app/views/exercises/index.html.slim b/app/views/exercises/index.html.slim index 337eebab..762b5b85 100644 --- a/app/views/exercises/index.html.slim +++ b/app/views/exercises/index.html.slim @@ -30,7 +30,7 @@ h1 = Exercise.model_name.human(count: 2) tr data-id=exercise.id td.p-1.pt-2 = link_to_if(policy(exercise).show?, exercise.title, exercise, 'data-turbolinks' => "false") td.p-1.pt-2 = link_to_if(exercise.execution_environment && policy(exercise.execution_environment).show?, exercise.execution_environment, exercise.execution_environment) - td.p-1.pt-2 = exercise.files.teacher_defined_tests.count + td.p-1.pt-2 = exercise.files.teacher_defined_assessments.count td.p-1.pt-2 = exercise.maximum_score td.p-1.pt-2 = exercise.exercise_tags.count td.p-1.pt-2 = exercise.expected_difficulty diff --git a/app/views/shared/_file.html.slim b/app/views/shared/_file.html.slim index a98c92da..b8122113 100644 --- a/app/views/shared/_file.html.slim +++ b/app/views/shared/_file.html.slim @@ -4,7 +4,7 @@ = row(label: 'file.role', value: file.role? ? t("files.roles.#{file.role}") : '') = row(label: 'file.hidden', value: file.hidden) = row(label: 'file.read_only', value: file.read_only) -- if file.teacher_defined_test? +- if file.teacher_defined_assessment? = row(label: 'file.feedback_message', value: render_markdown(file.feedback_message), class: 'm-0') = row(label: 'file.weight', value: file.weight) = row(label: 'file.content', value: file.native_file? ? link_to_if(policy(file).show?, file.native_file.file.filename, file.native_file.url) : code_tag(file.content)) diff --git a/config/locales/de.yml b/config/locales/de.yml index 8d2ae058..d9acfc1e 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -481,6 +481,7 @@ de: teacher_defined_test: Test als Bewertungsgrundlage user_defined_file: Benutzerdefinierte Datei user_defined_test: Benutzerdefinierter Test + teacher_defined_linter: Linter als Bewertungsgrundlage error: filename: "Die Datei konnte nicht gespeichert werden, da eine Datei mit dem Namen '%{name}' bereits existiert." hints: diff --git a/config/locales/en.yml b/config/locales/en.yml index 5d7bd43d..46b8ce00 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -481,6 +481,7 @@ en: teacher_defined_test: Test for Assessment user_defined_file: User-defined File user_defined_test: User-defined Test + teacher_defined_linter: Linter for Assessment error: filename: "The file could not be saved, because another file with the name '%{name}' already exists." hints: diff --git a/lib/docker_client.rb b/lib/docker_client.rb index c1912f81..f8ae4e61 100644 --- a/lib/docker_client.rb +++ b/lib/docker_client.rb @@ -376,10 +376,13 @@ class DockerClient """ Stick to existing Docker API with exec command. """ - filepath = submission.collect_files.find { |f| f.name_with_extension == filename }.filepath + file = submission.collect_files.find { |f| f.name_with_extension == filename } + filepath = file.filepath command = submission.execution_environment.test_command % command_substitutions(filepath) create_workspace_files = proc { create_workspace_files(container, submission) } - execute_command(command, create_workspace_files, block) + test_result = execute_command(command, create_workspace_files, block) + test_result.merge!(file_role: file.role) + test_result end def self.find_image_by_tag(tag) diff --git a/lib/py_unit_and_py_lint_adapter.rb b/lib/py_unit_and_py_lint_adapter.rb index ef49f141..4b6dea5f 100644 --- a/lib/py_unit_and_py_lint_adapter.rb +++ b/lib/py_unit_and_py_lint_adapter.rb @@ -5,9 +5,10 @@ class PyUnitAndPyLintAdapter < TestingFrameworkAdapter end def parse_output(output) - PyLintAdapter.new.parse_output(output) - rescue NoMethodError - # The regex for PyLint failed and did not return any matches - PyUnitAdapter.new.parse_output(output) + if output[:file_role] == 'teacher_defined_linter' + PyLintAdapter.new.parse_output(output) + else + PyUnitAdapter.new.parse_output(output) + end end end diff --git a/spec/concerns/submission_scoring_spec.rb b/spec/concerns/submission_scoring_spec.rb index c32fe9c3..62b29817 100644 --- a/spec/concerns/submission_scoring_spec.rb +++ b/spec/concerns/submission_scoring_spec.rb @@ -13,7 +13,7 @@ describe SubmissionScoring do after(:each) { controller.send(:collect_test_results, @submission) } it 'executes every teacher-defined test file' do - @submission.collect_files.select(&:teacher_defined_test?).each do |file| + @submission.collect_files.select(&:teacher_defined_assessment?).each do |file| expect(controller).to receive(:execute_test_file).with(file, @submission).and_return({}) end end diff --git a/spec/controllers/submissions_controller_spec.rb b/spec/controllers/submissions_controller_spec.rb index 97dd2eb3..3cfecc64 100644 --- a/spec/controllers/submissions_controller_spec.rb +++ b/spec/controllers/submissions_controller_spec.rb @@ -241,7 +241,7 @@ describe SubmissionsController do end describe 'GET #test' do - let(:filename) { submission.collect_files.detect(&:teacher_defined_test?).name_with_extension } + let(:filename) { submission.collect_files.detect(&:teacher_defined_assessment?).name_with_extension } let(:output) { {} } before(:each) do diff --git a/spec/lib/docker_client_spec.rb b/spec/lib/docker_client_spec.rb index 4884a802..06b1faa2 100644 --- a/spec/lib/docker_client_spec.rb +++ b/spec/lib/docker_client_spec.rb @@ -263,7 +263,7 @@ describe DockerClient, docker: true do end describe '#execute_test_command' do - let(:filename) { submission.exercise.files.detect { |file| file.role == 'teacher_defined_test' }.name_with_extension } + let(:filename) { submission.exercise.files.detect { |file| file.role == 'teacher_defined_test' || file.role == 'teacher_defined_linter' }.name_with_extension } after(:each) { docker_client.send(:execute_test_command, submission, filename) } it 'takes a container from the pool' do diff --git a/spec/services/proforma_service/convert_task_to_exercise_spec.rb b/spec/services/proforma_service/convert_task_to_exercise_spec.rb index ad41de3e..7cd93af8 100644 --- a/spec/services/proforma_service/convert_task_to_exercise_spec.rb +++ b/spec/services/proforma_service/convert_task_to_exercise_spec.rb @@ -3,6 +3,8 @@ require 'rails_helper' describe ProformaService::ConvertTaskToExercise do + # ToDo: Add teacher_defined_linter for tests + describe '.new' do subject(:convert_to_exercise_service) { described_class.new(task: task, user: user, exercise: exercise) }