-
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?
Conversation
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Naming/PredicateName: Rename is_saving_snapshots? to saving_snapshots?.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
[Correctable] Layout/TrailingWhitespace: Trailing whitespace detected.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
[Correctable] Layout/ClosingParenthesisIndentation: Align ) with (.
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 comment
The 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?
@@ -41,15 +42,62 @@ def edit | |||
|
|||
def update | |||
result = @programming_question.class.transaction do | |||
old_update_timestamp = @programming_question.snapshot_of_state_at |
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?
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
[Correctable] Style/RedundantSelf: Redundant self detected.
!skip_process_package? | ||
end | ||
|
||
def is_saving_snapshots? |
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.
Naming/PredicateName: Rename is_saving_snapshots? to saving_snapshots?.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Naming/PredicateName: Rename is_saving_snapshots? to saving_snapshots?.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Naming/PredicateName: Rename is_saving_snapshots? to saving_snapshots?.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Naming/PredicateName: Rename is_saving_snapshots? to saving_snapshots?.
end | ||
end | ||
def get_first_test_failures_by_type(test_cases_by_type, test_results_by_type) | ||
test_cases_by_type.entries.map do |test_case_type, test_cases| |
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.
[Correctable] Style/HashTransformValues: Prefer transform_values over map {...}.to_h.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
[Correctable] Layout/CommentIndentation: Incorrect indentation detected (column 4 instead of 2).
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
[Correctable] Layout/CommentIndentation: Incorrect indentation detected (column 4 instead of 2).
# our original question by comparing the updated_at timestamps. | ||
programming_auto_grading.question_snapshot_id = | ||
Course::Assessment::Question::Programming. | ||
where(current_id: question.id, snapshot_index: question.snapshot_index).first&.id || question.id |
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.
[Correctable] Layout/MultilineMethodCallIndentation: Align where with Course::Assessment::Question::Programming. on line 14.
|
||
# If the question was updated during the evaluation, we can still find | ||
# our original question by comparing the updated_at timestamps. | ||
programming_auto_grading.question_snapshot_id = |
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.
[Correctable] Layout/TrailingWhitespace: Trailing whitespace detected.
# For now, we have to manually query the latest_answer_ids first. | ||
latest_answer_ids = @question.answers. | ||
unscope(:order). | ||
select('DISTINCT ON (submission_id) id'). |
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.
[Correctable] Layout/MultilineMethodCallIndentation: Align select with @question.answers. on line 16.
# In Rails 8, it will be possible to override the batch ordering using :cursor. | ||
# For now, we have to manually query the latest_answer_ids first. | ||
latest_answer_ids = @question.answers. | ||
unscope(:order). |
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.
[Correctable] Layout/MultilineMethodCallIndentation: Align unscope with @question.answers. on line 16.
end | ||
end | ||
|
||
json.(auto_grading, :stdout, :stderr) if show_stdout_and_stderr |
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.
[Correctable] Style/LambdaCall: Prefer the use of lambda.call(...) over lambda.(...).
end | ||
elsif first_failures_by_type['private_test'] | ||
explanations << format_ckeditor_rich_text(get_hint( | ||
first_failures_by_type['private_test'], |
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.
[Correctable] Layout/FirstArgumentIndentation: Indent the first argument one step more than get_hint(.
end | ||
if first_failures_by_type['public_test'] | ||
explanations << format_ckeditor_rich_text(get_hint( | ||
first_failures_by_type['public_test'], |
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.
[Correctable] Layout/FirstArgumentIndentation: Indent the first argument one step more than get_hint(.
No description provided.