-
Notifications
You must be signed in to change notification settings - Fork 80
Create / Save snapshots of programming questions on edit #8022
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 3 commits
21ed15d
61c7749
71603a0
c5a25a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ def new | |
|
|
||
| def create | ||
| @programming_question.package_type = programming_question_params.key?(:file) ? :zip_upload : :online_editor | ||
| @programming_question.current = @programming_question | ||
| process_package | ||
|
|
||
| if @programming_question.save | ||
|
|
@@ -41,15 +42,62 @@ def edit | |
|
|
||
| def update | ||
| result = @programming_question.class.transaction do | ||
| old_update_timestamp = @programming_question.snapshot_of_state_at | ||
|
|
||
| # Duplicate the original question as a snapshot | ||
| snapshot = @programming_question.dup | ||
| snapshot.current = @programming_question | ||
|
|
||
| snapshot.template_files = @programming_question.template_files.map do |template_file| | ||
| duplicated_template_file = template_file.dup | ||
| duplicated_template_file.question = snapshot | ||
| duplicated_template_file | ||
| end | ||
|
|
||
| snapshot.test_cases = @programming_question.test_cases.map do |test_case| | ||
| duplicated_test_case = test_case.dup | ||
| duplicated_test_case.question = snapshot | ||
|
|
||
| # Test case results aren't duplicated by default, so we do that now | ||
| duplicated_test_case.test_results = test_case.test_results.map(&:dup) if test_case.test_results.any? | ||
| duplicated_test_case | ||
| end | ||
|
|
||
| @question_assessment.skill_ids = programming_question_params[:question_assessment]. | ||
| try(:[], :skill_ids) | ||
| @programming_question.assign_attributes(programming_question_params. | ||
| except(:question_assessment)) | ||
| @programming_question.is_synced_with_codaveri = false | ||
| process_package | ||
|
|
||
| update_timestamp = Time.current | ||
| @programming_question.updated_at = update_timestamp | ||
| @programming_question.snapshot_of_state_at = update_timestamp | ||
| @programming_question.snapshot_index = @programming_question.snapshot_index + 1 | ||
|
|
||
| raise ActiveRecord::Rollback unless @programming_question.save | ||
|
|
||
| if @programming_question.should_create_snapshot? | ||
| @programming_question.update_column(:import_job_id, nil) # maintains uniqueness constraint | ||
| snapshot.skip_process_package = true | ||
| snapshot.save! | ||
|
|
||
| update_result = ActiveRecord::Base.connection.execute(<<-SQL.squish | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lint/UselessAssignment: Useless assignment to variable - update_result. Did you mean update_timestamp? |
||
| UPDATE course_assessment_answer_programming_auto_gradings | ||
| SET question_snapshot_id = #{snapshot.id} | ||
| FROM course_assessment_answer_auto_gradings, course_assessment_answers, course_assessment_questions | ||
| WHERE course_assessment_answer_programming_auto_gradings.id = course_assessment_answer_auto_gradings.actable_id | ||
| AND course_assessment_answer_auto_gradings.answer_id = course_assessment_answers.id | ||
| AND course_assessment_questions.id = course_assessment_answers.question_id | ||
| AND course_assessment_questions.actable_id = #{@programming_question.id} | ||
| AND course_assessment_questions.actable_type = 'Course::Assessment::Question::Programming' | ||
| AND (course_assessment_answer_programming_auto_gradings.question_snapshot_id IS NULL | ||
| OR course_assessment_answer_programming_auto_gradings.question_snapshot_id = #{@programming_question.id}) | ||
| SQL | ||
| ) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Correctable] Layout/ClosingParenthesisIndentation: Align ) with (. |
||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Correctable] Layout/TrailingWhitespace: Trailing whitespace detected. |
||
| end | ||
|
|
||
| true | ||
| end | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,8 +74,8 @@ def last_attempt_answer_submitted_job(answer) | |
|
|
||
| attempts = submission.answers.from_question(answer.question_id) | ||
| last_non_current_answer = attempts.reject(&:current_answer?).last | ||
| job = last_non_current_answer&.auto_grading&.job | ||
| job&.status == 'submitted' ? job : nil | ||
| jobs = last_non_current_answer&.auto_gradings&.map(&:job)&.compact&.select { |j| j.status == 'submitted' } | ||
| jobs&.first | ||
| end | ||
|
|
||
| def reattempt_and_grade_answer(answer) | ||
|
|
@@ -87,7 +87,8 @@ def reattempt_and_grade_answer(answer) | |
| # so destroy the failed job answer and re-grade the current entry. | ||
| answer.class.transaction do | ||
| last_answer = answer.submission.answers.select { |ans| ans.question_id == answer.question_id }.last | ||
| last_answer.destroy! if last_answer&.auto_grading&.job&.errored? | ||
| p({ laag: last_answer&.auto_gradings, should_destroy: last_answer&.auto_gradings&.any? { |ag| ag.job&.errored? } }) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Correctable] Layout/LineLength: Line is too long. [121/120] |
||
| last_answer.destroy! if last_answer&.auto_gradings&.any? { |ag| ag.job&.errored? } | ||
| new_answer = reattempt_answer(answer, finalise: true) | ||
| new_answer.auto_grade!(redirect_to_path: nil, reduce_priority: false) | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,8 +55,9 @@ class Course::Assessment::Answer < ApplicationRecord | |
| belongs_to :submission, inverse_of: :answers | ||
| belongs_to :question, class_name: 'Course::Assessment::Question', inverse_of: nil | ||
| belongs_to :grader, class_name: 'User', inverse_of: nil, optional: true | ||
| has_one :auto_grading, class_name: 'Course::Assessment::Answer::AutoGrading', | ||
| dependent: :destroy, inverse_of: :answer, autosave: true | ||
| has_many :auto_gradings, -> { order(:created_at) }, | ||
| class_name: 'Course::Assessment::Answer::AutoGrading', | ||
| dependent: :destroy, inverse_of: :answer, autosave: true | ||
|
|
||
| accepts_nested_attributes_for :actable | ||
|
|
||
|
|
@@ -80,13 +81,18 @@ class Course::Assessment::Answer < ApplicationRecord | |
| def auto_grade!(redirect_to_path: nil, reduce_priority: false) | ||
| raise IllegalStateError if attempting? | ||
|
|
||
| ensure_auto_grading! | ||
| p({ ssq: self.question, ssq_is_saving_snapshots: self.question.is_saving_snapshots? }) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Correctable] Style/RedundantSelf: Redundant self detected. |
||
| auto_grading = if self.question.is_saving_snapshots? | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Correctable] Style/RedundantSelf: Redundant self detected. |
||
| Course::Assessment::Answer::AutoGrading.create!(answer: self) | ||
| else | ||
| ensure_auto_grading! | ||
| end | ||
| if grade_inline? | ||
| Course::Assessment::Answer::AutoGradingService.grade(self) | ||
| Course::Assessment::Answer::AutoGradingService.grade(self, auto_grading) | ||
| nil | ||
| else | ||
| auto_grading_job_class(reduce_priority). | ||
| perform_later(self, redirect_to_path).tap do |job| | ||
| perform_later(self, auto_grading, redirect_to_path).tap do |job| | ||
| auto_grading.update_column(:job_id, job.job_id) | ||
| end | ||
| end | ||
|
|
@@ -189,31 +195,17 @@ def validate_grade | |
|
|
||
| # Ensures that an auto grading record exists for this answer. | ||
| # | ||
| # Use this to guarantee that an auto grading record exists, and retrieves it. This is because | ||
| # there can be a concurrent creation of such a record across two processes, and this can only | ||
| # be detected at the database level. | ||
| # | ||
| # The additional transaction is in place because a RecordNotUnique will cause the active | ||
| # transaction to be considered as errored, and needing a rollback. | ||
| # | ||
| # @return [Course::Assessment::Answer::AutoGrading] | ||
| def ensure_auto_grading! | ||
| ActiveRecord::Base.transaction(requires_new: true) do | ||
| auto_grading || create_auto_grading! | ||
| end | ||
| rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotUnique => e | ||
| raise e if e.is_a?(ActiveRecord::RecordInvalid) && e.record.errors[:answer_id].empty? | ||
|
|
||
| association(:auto_grading).reload | ||
| auto_grading | ||
| auto_gradings&.last || Course::Assessment::Answer::AutoGrading.create!(answer: self) | ||
| end | ||
|
|
||
| def unsubmit | ||
| self.grade = nil | ||
| self.grader = nil | ||
| self.graded_at = nil | ||
| self.submitted_at = nil | ||
| auto_grading&.mark_for_destruction | ||
| auto_gradings.map(&:mark_for_destruction) | ||
| end | ||
|
|
||
| def auto_grading_job_class(reduce_priority) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,11 @@ class Course::Assessment::Answer::ProgrammingAutoGrading < ApplicationRecord | |
| class_name: 'Course::Assessment::Answer::ProgrammingAutoGradingTestResult', | ||
| foreign_key: :auto_grading_id, inverse_of: :auto_grading, | ||
| dependent: :destroy | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Correctable] Layout/TrailingWhitespace: Trailing whitespace detected. |
||
| belongs_to :question_snapshot, | ||
| class_name: 'Course::Assessment::Question::Programming', | ||
| foreign_key: :question_snapshot_id, | ||
| optional: true | ||
|
|
||
| private | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,10 @@ class Course::Assessment::Question::ForumPostResponse < ApplicationRecord | |
| validates :max_posts, presence: true, numericality: { only_integer: true } | ||
| validate :allowable_max_post_count | ||
|
|
||
| def is_saving_snapshots? | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming/PredicateName: Rename is_saving_snapshots? to saving_snapshots?. |
||
| false | ||
| end | ||
|
|
||
| def question_type | ||
| 'ForumPostResponse' | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,10 @@ class Course::Assessment::Question::MultipleResponse < ApplicationRecord | |
| # "any correct" allows it to have more than one correct answer. | ||
| alias_method :multiple_choice?, :any_correct? | ||
|
|
||
| def is_saving_snapshots? | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming/PredicateName: Rename is_saving_snapshots? to saving_snapshots?. |
||
| false | ||
| end | ||
|
|
||
| def auto_gradable? | ||
| true | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,8 @@ class Course::Assessment::Question::Programming < ApplicationRecord # rubocop:di | |
|
|
||
| acts_as :question, class_name: 'Course::Assessment::Question' | ||
|
|
||
| before_create :set_snapshot_attributes | ||
| after_destroy_commit :destroy_snapshots | ||
| after_initialize :set_defaults | ||
| before_save :process_package, unless: :skip_process_package? | ||
| before_validation :assign_template_attributes | ||
|
|
@@ -45,6 +47,20 @@ class Course::Assessment::Question::Programming < ApplicationRecord # rubocop:di | |
| has_many :test_cases, class_name: 'Course::Assessment::Question::ProgrammingTestCase', | ||
| dependent: :destroy, foreign_key: :question_id, inverse_of: :question | ||
|
|
||
| has_many :snapshots, class_name: 'Course::Assessment::Question::Programming', | ||
| foreign_key: :current_id, inverse_of: :current | ||
|
|
||
| belongs_to :current, class_name: 'Course::Assessment::Question::Programming', | ||
| optional: true, inverse_of: :snapshots | ||
|
|
||
| def should_create_snapshot? | ||
| !skip_process_package? | ||
| end | ||
|
|
||
| def is_saving_snapshots? | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming/PredicateName: Rename is_saving_snapshots? to saving_snapshots?. |
||
| true | ||
| end | ||
|
|
||
| def auto_gradable? | ||
| !test_cases.empty? | ||
| end | ||
|
|
@@ -167,6 +183,12 @@ def create_or_update_codaveri_problem | |
|
|
||
| private | ||
|
|
||
| def set_snapshot_attributes | ||
| self.current ||= self | ||
| self.snapshot_of_state_at ||= Time.current | ||
| self.snapshot_index ||= 0 | ||
| end | ||
|
|
||
| def set_defaults | ||
| self.max_time_limit = DEFAULT_CPU_TIMEOUT | ||
| self.skip_process_package = false | ||
|
|
@@ -262,6 +284,10 @@ def validate_codaveri_question | |
| 'Activate it in the course setting or switch this question into a non-codaveri type.') | ||
| end | ||
| end | ||
|
|
||
| def destroy_snapshots | ||
| self.snapshots.where.not(id: self).destroy_all | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Correctable] Style/RedundantSelf: Redundant self detected. |
||
| end | ||
| end | ||
|
|
||
| def validate_language_enabled | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,10 @@ def initialize_duplicate(duplicator, other) | |
| self.categories = duplicator.duplicate(other.categories) | ||
| end | ||
|
|
||
| def is_saving_snapshots? | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming/PredicateName: Rename is_saving_snapshots? to saving_snapshots?. |
||
| false | ||
| end | ||
|
|
||
| def auto_gradable? | ||
| !categories.empty? && ai_grading_enabled? | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,10 @@ class Course::Assessment::Question::Scribing < ApplicationRecord | |
| acts_as :question, class_name: 'Course::Assessment::Question' | ||
| has_one_attachment | ||
|
|
||
| def is_saving_snapshots? | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming/PredicateName: Rename is_saving_snapshots? to saving_snapshots?. |
||
| false | ||
| end | ||
|
|
||
| def to_partial_path | ||
| 'course/assessment/question/scribing/scribing' | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,10 @@ class Course::Assessment::Question::TextResponse < ApplicationRecord | |
|
|
||
| accepts_nested_attributes_for :groups, allow_destroy: true | ||
|
|
||
| def is_saving_snapshots? | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming/PredicateName: Rename is_saving_snapshots? to saving_snapshots?. |
||
| false | ||
| end | ||
|
|
||
| def auto_gradable? | ||
| if comprehension_question? | ||
| groups.any?(&:auto_gradable_group?) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,10 @@ | |
| class Course::Assessment::Question::VoiceResponse < ApplicationRecord | ||
| acts_as :question, class_name: 'Course::Assessment::Question' | ||
|
|
||
| def is_saving_snapshots? | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming/PredicateName: Rename is_saving_snapshots? to saving_snapshots?. |
||
| false | ||
| end | ||
|
|
||
| def attempt(submission, last_attempt = nil) | ||
| answer = | ||
| Course::Assessment::Answer::VoiceResponse.new(submission: submission, question: question) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,9 +5,10 @@ class << self | |
| # +Course::Assessment::Answer::AutoGrading+ object. | ||
| # | ||
| # @param [Course::Assessment::Answer] answer The answer to be graded. | ||
| def grade(answer) | ||
| # @param [Course::Assessment::Answer::AutoGrading] auto_grading The auto grading object that will store the results. | ||
| def grade(answer, auto_grading) | ||
| answer = if answer.question.auto_gradable? | ||
| pick_grader(answer.question).grade(answer) | ||
| pick_grader(answer.question).grade(answer, auto_grading) | ||
| else | ||
| assign_maximum_grade(answer) | ||
| end | ||
|
|
@@ -19,7 +20,7 @@ def grade(answer) | |
| # Picks the grader to use for the given question. | ||
| # | ||
| # @param [Course::Assessment::Question] question The question that the needs to be graded. | ||
| # @return [Course::Assessment::Answer::AnswerAutoGraderService] The service object that can | ||
| # @return [Course::Assessment::Answer::AnswerAutoGradingService] The service object that can | ||
| # grade this question. | ||
| def pick_grader(question) | ||
| question.auto_grader | ||
|
|
@@ -48,10 +49,11 @@ def assign_maximum_grade(answer) | |
| # and makes sure answer is in the correct state. | ||
| # | ||
| # @param [Course::Assessment::Answer] answer The answer to be graded. | ||
| # @param [Course::Assessment::Answer::AutoGrading] auto_grading The auto grading object that will store the results. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Correctable] Layout/CommentIndentation: Incorrect indentation detected (column 4 instead of 2). |
||
| # @return [Course::Assessment::Answer] The graded answer. Note that this answer is not persisted | ||
| # yet. | ||
| def grade(answer) | ||
| grade = evaluate(answer) | ||
| def grade(answer, auto_grading) | ||
| grade = evaluate(answer, auto_grading) | ||
| answer.evaluate! | ||
|
|
||
| if answer.submission.assessment.autograded? | ||
|
|
@@ -66,8 +68,9 @@ def grade(answer) | |
| # subclasses. | ||
| # | ||
| # @param [Course::Assessment::Answer] answer The answer to be evaluated. | ||
| # @param [Course::Assessment::Answer::AutoGrading] auto_grading The auto grading object that will store the results. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Correctable] Layout/CommentIndentation: Incorrect indentation detected (column 4 instead of 2). |
||
| # @return [Integer] grade The grade of the answer. | ||
| def evaluate(_answer) | ||
| def evaluate(_answer, _auto_grading) | ||
| raise 'Not Implemented' | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint/UselessAssignment: Useless assignment to variable - old_update_timestamp. Did you mean update_timestamp?