-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
Clear scores when another submission is selected to grade #3405
Conversation
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.
👍 (apart from Rubocop's complaint)
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.
I suppose this will also fix the issue reported in this comment?
db/seeds.rb
Outdated
summary: submission_summary(status), | ||
code: "print(input())\n", | ||
result: File.read(Rails.root.join('db', 'results', "#{exercise.judge.name}-result.json")) | ||
(1..rand(1..10)).each do |
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.
Do we really need this many submissions? I know this seems like a small number, but they quickly add up, especially in nested loops. After a while our tests and db seeds (and thus mestra deploys) become painfully slow.
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.
Created a separate course for visualizations. Because yes we need so many for decent graphs and testing features with multiple submission per student for example, but no need to have it in every course
db/seeds.rb
Outdated
status: status, | ||
accepted: status == :correct, | ||
summary: submission_summary(status), | ||
created_at: (Time.now - 1.day).beginning_of_day + 6.hour + rand(0..540).minutes + rand(0..540).minutes, |
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.
What is the goal if this construction? If there is a specific reason for this time range, maybe add a comment with the explanation.
Next to that, it can be cleaned up. The 6.hours and double rand can be replace by a single rand. The first part can probably be replaced by something like Date.yesterday or 1.day.ago.
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.
I improved the date and added a comment for the other part. Using two rands creates a bell curve, so it is not the same result as a single rand
This pull request clears the scores when an other submission is selected to grade.
Closes #3344 .