From 266f38502a12b047445d9a3672e54d8e651dac64 Mon Sep 17 00:00:00 2001 From: Martijn Stegeman Date: Thu, 9 Jan 2020 12:30:12 +0100 Subject: [PATCH 01/41] initial refactoring of submitting --- app/controllers/page_controller.rb | 391 +++++++++++----------- app/controllers/submissions_controller.rb | 80 +++++ app/models/grade.rb | 2 + app/models/pset.rb | 1 + app/models/submit.rb | 54 +-- app/models/user.rb | 2 +- app/utils/attachments.rb | 77 +++++ app/utils/auto_check_uploader.rb | 36 ++ app/utils/dropbox.rb | 4 +- app/utils/dropbox_uploader.rb | 15 + app/views/grades/_summary.html.erb | 2 +- app/views/page/index.html.erb | 4 +- config/routes.rb | 1 + 13 files changed, 441 insertions(+), 228 deletions(-) create mode 100644 app/controllers/submissions_controller.rb create mode 100644 app/utils/attachments.rb create mode 100644 app/utils/auto_check_uploader.rb create mode 100644 app/utils/dropbox_uploader.rb diff --git a/app/controllers/page_controller.rb b/app/controllers/page_controller.rb index a6e4337c..a85be6c2 100644 --- a/app/controllers/page_controller.rb +++ b/app/controllers/page_controller.rb @@ -1,15 +1,17 @@ -require 'dropbox' - -class LUploadIO < StringIO - def initialize(name) - @path = name - super() # the () is essential, calls no-arg initializer - end - - def path - @path - end -end +# require 'dropbox' +# +# class LUploadIO < StringIO +# # string serves as in-memory container for a zipfile +# +# def initialize(name) +# @path = name +# super() # the () is essential, calls no-arg initializer +# end +# +# def path +# @path +# end +# end class PageController < ApplicationController @@ -43,189 +45,186 @@ def section raise ActionController::RoutingError.new('Not Found') if !@section || @section.content_page.blank? end - def submit - # we may get here after expiry of the session if someone doesn't reload now and then - if not logged_in? - redirect_back(fallback_location: '/', alert: 'Please login again before submitting.') and return - end - - # is the total upload size acceptable? (10MiB) - if request.content_length > 9999999 - redirect_back(fallback_location: '/', alert: "Your files are too big somehow! Please check what you're uploading or ask your teacher.") and return - end - - page = Page.find(params[:page_id]) - pset = page.pset - - folder_name = pset.name + "__" + Time.now.to_i.to_s - - if (pset.form || pset.files.any?) && (!Dropbox.connected? || Settings.dropbox_folder_name.blank?) - redirect_back(fallback_location: '/', alert: 'There is a problem with submitting! Warn your teacher immediately and mention Dropbox.'.html_safe) and return - end - - form_text = render_form_text(params[:a]) - - # - # upload everything to dropbox - # - if pset.form || pset.files - begin - upload_to_dropbox(current_user.login_id, current_user.name, - Settings.dropbox_folder_name, folder_name, params[:notes], form_text, params[:f]) - rescue - redirect_back(fallback_location: '/', alert: "There was a problem uploading your submission! Please try again. If the problem persists, contact your teacher.".html_safe ) and return - end - end - - # - # create submit record - # - submit = Submit.where(:user_id => current_user.id, :pset_id => pset.id).first_or_initialize - submit.submitted_at = Time.now - submit.used_login = current_user.login_id - submit.url = params[:url] - submit.folder_name = folder_name - submit.check_feedback = nil - submit.style_feedback = nil - submit.auto_graded = false - submit.submitted_files = params.permit(f: {})[:f].to_h.map { |file,info| info.original_filename } if params[:f] - if files = params[:f] - file_contents = {} - files.each do |filename, file| - name = file.original_filename - if text_file?(name) - if file.size < 60000 - file.rewind and file_contents[name] = file.read - else - file_contents[name] = "Uploaded file was too large!" - end - end - end - end - submit.file_contents = file_contents - submit.save - - # - # create or touch submit for associated module, if possible/needed - # - if pset.parent_mod.present? && pset.parent_mod.pset.present? - mod_submit = Submit.where(:user_id => current_user.id, :pset_id => pset.parent_mod.pset.id).first_or_initialize - if mod_submit.persisted? - mod_submit.touch(:submitted_at) - else - mod_submit.submitted_at = Time.now - mod_submit.save - end - end - - # - # get files to check server - # - if pset.config['check'] && files = params[:f] - submitted_zips = files.keys.select { |x| x.end_with?(".zip") } - if submitted_zips.any? - zipfile = files[submitted_zips[0]] - zipfile.rewind - else - zipfile = Zip::OutputStream.write_buffer(::LUploadIO.new('file.zip')) do |zio| - files.each do |filename, file| - zio.put_next_entry(filename) - file.rewind - zio.write file.read - end - end - zipfile.rewind - end - - server = RestClient::Resource.new( - "https://agile008.science.uva.nl/#{pset.config['check']['tool']}", - :verify_ssl => OpenSSL::SSL::VERIFY_NONE - ) - - begin - args = { - file: zipfile, - password: "martijndoeteenphd", - webhook: "https://#{request.host}/api/check_result/do", - multipart: true - # and add slug/repo/args from the config file - }.merge(pset.config['check'].slice('slug', 'repo', 'args')) - response = server.post(args) - logger.debug JSON.parse(response.body)['id'] - logger.debug submit.inspect - submit.check_token = JSON.parse(response.body)['id'] - submit.save - rescue RestClient::ExceptionWithResponse => e - logger.debug e.response - end - - end - - # - # this is a re-submit, so re-open for grading - # - if submit.grade - submit.grade.grade = nil - submit.grade.unfinished! - end - - # - # success, get back to previous page - # - begin - redirect_back fallback_location: '/' - rescue ActionController::RedirectBackError - redirect_to :root - end - end - - private - - def text_file?(name) - return [".py", ".c", ".txt", ".html", ".css", ".h", ".java"].include?(File.extname(name)) || name == "Makefile" - end - - # writes hash with form contents to a plain text string - def render_form_text(form) - form_text = nil - if form - form_text = "" - form.each do |key, value| - form_text += "#{key}\n\n" - form_text += "#{value}\n\n" - end - end - return form_text - end - - def upload_to_dropbox(user, name, course, item, notes, form, files) - - dropbox_client = Dropbox.client - dropbox_root = "Submit" - - # cache timestamp for folder name - item_folder = item - - # compose info.txt file contents - info = "student_login_id = " + user - info += ("\nname = " + name) if name - info += "\n\n" - info += notes if notes - - # upload the notes - dropbox_client.upload(File.join("/", dropbox_root, course, user, item_folder, 'info.txt'), info) if notes - - # upload the form - dropbox_client.upload(File.join("/", dropbox_root, course, user, item_folder, 'form.md'), form) if form - - # upload all posted files - if files - files.each do |filename, file| - dropbox_client.upload(File.join("/", dropbox_root, course, user, item_folder, file.original_filename), file.read, autorename: true) - end - end - - end + # def submit + # + # + # # # we may get here after expiry of the session if someone doesn't reload now and then + # # if not logged_in? + # # redirect_back(fallback_location: '/', alert: 'Please login again before submitting.') and return + # # end + # + # # is the total upload size acceptable? (10MiB) + # if request.content_length > 9999999 + # redirect_back(fallback_location: '/', alert: "Your files are too big somehow! Please check what you're uploading or ask your teacher.") and return + # end + # + # page = Page.find(params[:page_id]) + # pset = page.pset + # + # folder_name = pset.name + "__" + Time.now.to_i.to_s + # + # if (pset.form || pset.files.any?) && (!Dropbox.connected? || Settings.dropbox_folder_name.blank?) + # redirect_back(fallback_location: '/', alert: 'There is a problem with submitting! Warn your teacher immediately and mention Dropbox.'.html_safe) and return + # end + # + # form_text = render_form_text(params[:a]) + # + # # + # # upload everything to dropbox + # # + # if pset.form || pset.files + # begin + # upload_to_dropbox(current_user.login_id, current_user.name, + # Settings.dropbox_folder_name, folder_name, params[:notes], form_text, params[:f]) + # rescue + # redirect_back(fallback_location: '/', alert: "There was a problem uploading your submission! Please try again. If the problem persists, contact your teacher.".html_safe ) and return + # end + # end + # + # # + # # create submit record + # # + # submit = Submit.where(:user_id => current_user.id, :pset_id => pset.id).first_or_initialize + # submit.submitted_at = Time.now + # submit.used_login = current_user.login_id + # submit.url = params[:url] + # submit.folder_name = folder_name + # submit.check_feedback = nil + # submit.style_feedback = nil + # submit.auto_graded = false + # submit.submitted_files = params.permit(f: {})[:f].to_h.map { |file,info| info.original_filename } if params[:f] + # if files = params[:f] + # file_contents = {} + # files.each do |filename, file| + # name = file.original_filename + # if text_file?(name) + # if file.size < 60000 + # file.rewind and file_contents[name] = file.read + # else + # file_contents[name] = "Uploaded file was too large!" + # end + # end + # end + # end + # submit.file_contents = file_contents + # submit.save + # + # # + # # create or touch submit for associated module, if possible/needed + # # + # if pset.parent_mod.present? && pset.parent_mod.pset.present? + # mod_submit = Submit.where(:user_id => current_user.id, :pset_id => pset.parent_mod.pset.id).first_or_initialize + # if mod_submit.persisted? + # mod_submit.touch(:submitted_at) + # else + # mod_submit.submitted_at = Time.now + # mod_submit.save + # end + # end + # + # # + # # get files to check server + # # + # if pset.config['check'] && files = params[:f] + # submitted_zips = files.keys.select { |x| x.end_with?(".zip") } + # if submitted_zips.any? + # zipfile = files[submitted_zips[0]] + # zipfile.rewind + # else + # zipfile = Zip::OutputStream.write_buffer(::LUploadIO.new('file.zip')) do |zio| + # files.each do |filename, file| + # zio.put_next_entry(filename) + # file.rewind + # zio.write file.read + # end + # end + # zipfile.rewind + # end + # + # server = RestClient::Resource.new( + # "https://agile008.science.uva.nl/#{pset.config['check']['tool']}", + # :verify_ssl => OpenSSL::SSL::VERIFY_NONE + # ) + # + # begin + # args = { + # file: zipfile, + # password: "martijndoeteenphd", + # webhook: "https://#{request.host}/api/check_result/do", + # multipart: true + # # and add slug/repo/args from the config file + # }.merge(pset.config['check'].slice('slug', 'repo', 'args')) + # response = server.post(args) + # logger.debug JSON.parse(response.body)['id'] + # logger.debug submit.inspect + # submit.check_token = JSON.parse(response.body)['id'] + # submit.save + # rescue RestClient::ExceptionWithResponse => e + # logger.debug e.response + # end + # + # end + # + # # + # # this is a re-submit, so re-open for grading + # # + # if submit.grade + # submit.grade.grade = nil + # submit.grade.unfinished! + # end + # + # # + # # success, get back to previous page + # # + # redirect_back fallback_location: '/' + # end + # + # private + # + # def text_file?(name) + # return [".py", ".c", ".txt", ".html", ".css", ".h", ".java"].include?(File.extname(name)) || name == "Makefile" + # end + # + # # writes hash with form contents to a plain text string + # def render_form_text(form) + # form_text = nil + # if form + # form_text = "" + # form.each do |key, value| + # form_text += "#{key}\n\n" + # form_text += "#{value}\n\n" + # end + # end + # return form_text + # end + # + # def upload_to_dropbox(user, name, course, item, notes, form, files) + # + # dropbox_client = Dropbox.client + # dropbox_root = "Submit" + # + # # cache timestamp for folder name + # item_folder = item + # + # # compose info.txt file contents + # info = "student_login_id = " + user + # info += ("\nname = " + name) if name + # info += "\n\n" + # info += notes if notes + # + # # upload the notes + # dropbox_client.upload(File.join("/", dropbox_root, course, user, item_folder, 'info.txt'), info) if notes + # + # # upload the form + # dropbox_client.upload(File.join("/", dropbox_root, course, user, item_folder, 'form.md'), form) if form + # + # # upload all posted files + # if files + # files.each do |filename, file| + # dropbox_client.upload(File.join("/", dropbox_root, course, user, item_folder, file.original_filename), file.read, autorename: true) + # end + # end + # + # end - end diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb new file mode 100644 index 00000000..4d2d7c0b --- /dev/null +++ b/app/controllers/submissions_controller.rb @@ -0,0 +1,80 @@ +class SubmissionsController < ApplicationController + + before_action :load_pset, :validate_attachment_size + + def create + collect_attachments + upload_attachments_to_dropbox if Dropbox.available? + upload_files_to_check_server if can_do_auto_check? + record_submission + redirect_back fallback_location: '/' + end + + private + + def load_pset + @pset = Pset.find(params[:pset_id]) + end + + def validate_attachment_size + request.content_length < 10000000 + end + + def collect_attachments + @attachments = Attachments.new(params.permit(f: {})[:f].to_h) + + # we don't actually have forms in our sites at this point + # if params[:a] + # form = params[:a].each do |key, value| + # "#{key}\n\n" + "#{value}\n\n" + # end + # @attachments.add("form.txt", form) + # end + + # params[:notes] is historically also interesting + # # compose info.txt file contents + # info = "student_login_id = " + user + # info += ("\nname = " + name) if name + # info += "\n\n" + # info += notes if notes + + puts @attachments.inspect + end + + def upload_attachments_to_dropbox + @submit_folder_name = @pset.name + "__" + Time.now.to_i.to_s + + dropbox_path = File.join( + "/", + 'Submit', # /Submit + Settings.dropbox_folder_name, # /course name + current_user.login_id, # /student ID + @submit_folder_name) # /mario__21981289 + + begin + uploader = DropboxUploader.new(path) + uploader.upload(@attachments) + rescue + redirect_back(fallback_location: '/', alert: "There was a problem uploading your submission! Please try again. If the problem persists, contact your teacher.".html_safe ) and return + end + end + + def can_do_auto_check? + AutoCheckUploader.enabled? && @pset.config['check'].present? + end + + def upload_files_to_check_server + @token = AutoCheckUploader.new(@attachments, @pset.config['check'], request.host).start + end + + def record_submission + submit = Submit.where(user: current_user, pset: @pset).first_or_initialize + submit.record( + used_login: current_user.login_id, + dropbox_folder_name: @submit_folder_name, + url: params[:url], + attachments: @attachments, + check_token: @token) + end + +end diff --git a/app/models/grade.rb b/app/models/grade.rb index dc0c1a38..84dae067 100644 --- a/app/models/grade.rb +++ b/app/models/grade.rb @@ -10,6 +10,8 @@ class Grade < ApplicationRecord delegate :name, to: :grader, prefix: true, allow_nil: true delegate :initials, to: :grader, prefix: true, allow_nil: true + scope :showable, -> { where(status: [Grade.statuses[:published], Grade.statuses[:exported]]) } + before_save :set_calculated_grade, :unpublicize_if_undone # :update_grades_cache, serialize :auto_grades diff --git a/app/models/pset.rb b/app/models/pset.rb index 10164b02..51a0ae3f 100644 --- a/app/models/pset.rb +++ b/app/models/pset.rb @@ -3,6 +3,7 @@ class Pset < ApplicationRecord belongs_to :page, optional: true has_one :mod belongs_to :parent_mod, class_name: "Mod", foreign_key: "mod_id", optional: true + delegate :pset, to: :parent_mod, prefix: 'parent', allow_nil: true # belongs_to :mod # has_one :parent_mod, class_name: "Mod" diff --git a/app/models/submit.rb b/app/models/submit.rb index da041c3b..11bf86d7 100644 --- a/app/models/submit.rb +++ b/app/models/submit.rb @@ -61,6 +61,34 @@ def last_submitted submitted_at && submitted_at.to_formatted_s(:short) || "never" end + def record(used_login: nil, dropbox_folder_name: nil, url: nil, attachments: nil, check_token: nil) + # basic info + self.submitted_at = Time.now + self.used_login = used_login + self.folder_name = dropbox_folder_name + + # attachments + self.url = url + self.submitted_files = attachments.file_names + self.file_contents = attachments.presentable_file_contents + + # reset auto checks + self.check_token = check_token + self.check_feedback = nil + self.style_feedback = nil + self.auto_graded = false + + self.save + + # update the submission for the parent module, if there is one + if pset.parent_pset + Submit.where(user: self.user, pset: pset.parent_pset).first_or_initialize.update(submitted_at:Time.now) + end + + # reset and unpublish grade + self.grade.update_columns(grade: nil, status: Grade.statuses[:unfinished]) if self.grade + end + def automatic puts "HIER #{self.inspect}" f = pset.config @@ -159,32 +187,6 @@ def check_feedback_problems? self.check_feedback.index { |x| x["status"].blank? }.present? end - def retrieve_check_feedback - path = File.join(Dropbox.root_folder, Settings.dropbox_folder_name, user.login_id, self.folder_name, 'check_results.json') - begin - json = Dropbox.download(path) - contents = json.present? ? JSON.parse(json) : nil - if contents.is_a?(Array) - self.update(check_results: { "checkpy" => contents }.to_json ) - else - self.update(check_results: contents.to_json) - end - rescue - # go on, assuming its not there - end - end - - # def retrieve_style_feedback - # path = File.join(Dropbox.root_folder, Settings.dropbox_folder_name, user.login_id, self.folder_name, 'style_results.json') - # begin - # json = Dropbox.download(path) - # contents = json.present? ? JSON.parse(json) : nil - # self.update(style_feedback: contents) - # rescue - # # go on, assuming its not there - # end - # end - def has_feedback? return false if not self.check_results check_results = JSON(self.check_results) diff --git a/app/models/user.rb b/app/models/user.rb index 12613f23..dc515253 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -78,7 +78,7 @@ def self.find_by_login(login) def items(with_private=false) items = [] items += submits.includes({:pset => [:parent_mod, :mod]}).where("submitted_at is not null").where("psets.mod_id is not null or mods_psets.pset_id is null").references(:psets, :mods).to_a - items += grades.includes(:pset, :submit, :grader).to_a + items += grades.includes(:pset, :submit, :grader).showable.to_a items += hands.to_a if with_private items += notes.to_a if with_private items = items.sort { |a,b| b.updated_at <=> a.updated_at } diff --git a/app/utils/attachments.rb b/app/utils/attachments.rb new file mode 100644 index 00000000..d8b5897a --- /dev/null +++ b/app/utils/attachments.rb @@ -0,0 +1,77 @@ +class LUploadIO < StringIO + # string serves as in-memory container for a zipfile + + def initialize(name) + @path = name + super() # the () is essential, calls no-arg initializer + end + + def path + @path + end +end + +class Attachments + + def initialize(files) + @files = files || {} + @other_files = {} + end + + def add(filename, contents) + @other_files[filename] = contents + end + + def filenames + @files.map do |file, info| + info.original_filename + end + end + + def presentable_file_contents + # start with form contents + presentable_files = @other_files.to_h + + # add uploaded files if presentable + @files.each do |filename, file| + name = file.original_filename + if text_file?(name) + if file.size < 60000 + file.rewind and presentable_files[name] = file.read + else + presentable_files[name] = "Uploaded file was too large!" + end + end + end + presentable_files + end + + def file_names + @files.map { |file,info| info.original_filename } + end + + def zipped + # if a zipfile is among submitted files, post that and ignore the rest + submitted_zips = @files.keys.select { |x| x.end_with?(".zip") } + if submitted_zips.any? + zipfile = @files[submitted_zips[0]] + else + zipfile = Zip::OutputStream.write_buffer(::LUploadIO.new('file.zip')) do |zio| + @files.each do |filename, file| + zio.put_next_entry(filename) + file.rewind + zio.write file.read + end + end + end + zipfile.rewind + zipfile + end + + private + + def text_file?(name) + return [".py", ".c", ".txt", ".html", ".css", ".h", ".java"].include?(File.extname(name)) || name == "Makefile" + end + +end diff --git a/app/utils/auto_check_uploader.rb b/app/utils/auto_check_uploader.rb new file mode 100644 index 00000000..8164d5bb --- /dev/null +++ b/app/utils/auto_check_uploader.rb @@ -0,0 +1,36 @@ +class AutoCheckUploader + + def self.enabled? + true + end + + def initialize(attachments, config, host) + @attachments = attachments + @config = config + @host = host + end + + def start + server = RestClient::Resource.new( + "https://agile008.science.uva.nl/#{@config['tool']}", + verify_ssl: OpenSSL::SSL::VERIFY_NONE) + + begin + args = { + file: @attachments.zipped, + password: "martijndoeteenphd", + webhook: "https://#{@host}/api/check_result/do", + multipart: true + # and add slug/repo/args from the config file + }.merge(@config.slice('slug', 'repo', 'args')) + + response = server.post(args) + Rails.logger.debug JSON.parse(response.body)['id'] + return JSON.parse(response.body)['id'] + rescue RestClient::ExceptionWithResponse => e + Rails.logger.debug e.response + end + + end + +end diff --git a/app/utils/dropbox.rb b/app/utils/dropbox.rb index ead5293b..7d66bb00 100644 --- a/app/utils/dropbox.rb +++ b/app/utils/dropbox.rb @@ -1,4 +1,4 @@ -module Dropbox +class Dropbox @@dropbox_key = ENV['DROPBOX_KEY'] @@dropbox_secret = ENV['DROPBOX_SECRET'] @@ -11,7 +11,7 @@ module Dropbox # check basic requirements for this API to function def self.available? - return @@dropbox_key.present? && @@dropbox_secret.present? && @@dropbox_access_type.present? + return Rails.env.production? && @@dropbox_key.present? && @@dropbox_secret.present? && @@dropbox_access_type.present? end # see if we're already connected (dropbox connections do not expire) diff --git a/app/utils/dropbox_uploader.rb b/app/utils/dropbox_uploader.rb new file mode 100644 index 00000000..cee3636a --- /dev/null +++ b/app/utils/dropbox_uploader.rb @@ -0,0 +1,15 @@ +class DropboxUploader + + def initialize(base_path) + @base_path = base_path + end + + def upload(files) + dropbox = Dropbox.client + files.each do |filename, file| + filename = File.join(@base_path, file.original_filename) + dropbox.upload(filename, file.read, autorename: true) + end + end + +end diff --git a/app/views/grades/_summary.html.erb b/app/views/grades/_summary.html.erb index 1faf98cd..6656ec87 100644 --- a/app/views/grades/_summary.html.erb +++ b/app/views/grades/_summary.html.erb @@ -2,5 +2,5 @@ <%= @grade.status %> Grading <%= t(:by) %> <%= @grade.grader_name %> / First graded <%= @submit.first_graded %> / - Last graded <%= @submit.last_graded %> + Last changed <%= @submit.last_graded %>

\ No newline at end of file diff --git a/app/views/page/index.html.erb b/app/views/page/index.html.erb index 0bf59d3b..0958e95b 100644 --- a/app/views/page/index.html.erb +++ b/app/views/page/index.html.erb @@ -1,4 +1,4 @@ -<%= form_tag_if(logged_in? && @page.pset, page_submit_path, { :multipart => true, :id => "page_form", data: { persist: 'garlic', destroy: false } }) do %> +<%= form_tag_if(logged_in? && @page.pset, submissions_path, { :multipart => true, :id => "page_form", data: { persist: 'garlic', destroy: false } }) do %> <% @subpages.each_with_index do |subpage, index| %>
<%= markdown(subpage.content, @page) %> @@ -14,5 +14,5 @@

You can't submit on mobile, sorry. If you're on desktop, increase your browser width to submit!

<% end %> -<%= hidden_field_tag :page_id, @page.id %> +<%= hidden_field_tag :pset_id, @page.pset.id if @page.pset %> <% end %> diff --git a/config/routes.rb b/config/routes.rb index f065d752..776fd01b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -190,6 +190,7 @@ get "search/subpage" # pages + resource :submissions, only: [ :create ] post "page/submit" get ":section/:page" => "page#index" # default route, for content pages (must be 2nd last!) get ":section" => "page#section" # default route, for section pages (must be last!) From 72c10c7d3f96e5eef035dba95d5ba25042f9b087 Mon Sep 17 00:00:00 2001 From: Martijn Stegeman Date: Thu, 9 Jan 2020 12:50:35 +0100 Subject: [PATCH 02/41] ch --- app/controllers/submissions_controller.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index 4d2d7c0b..7f4c6263 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -4,7 +4,11 @@ class SubmissionsController < ApplicationController def create collect_attachments - upload_attachments_to_dropbox if Dropbox.available? + begin + upload_attachments_to_dropbox if Dropbox.available? + rescue + redirect_back(fallback_location: '/', alert: "There was a problem uploading your submission! Please try again. If the problem persists, contact your teacher.".html_safe ) and return + end upload_files_to_check_server if can_do_auto_check? record_submission redirect_back fallback_location: '/' @@ -44,19 +48,15 @@ def collect_attachments def upload_attachments_to_dropbox @submit_folder_name = @pset.name + "__" + Time.now.to_i.to_s - dropbox_path = File.join( - "/", + submission_path = File.join( + '/', 'Submit', # /Submit Settings.dropbox_folder_name, # /course name current_user.login_id, # /student ID @submit_folder_name) # /mario__21981289 - begin - uploader = DropboxUploader.new(path) - uploader.upload(@attachments) - rescue - redirect_back(fallback_location: '/', alert: "There was a problem uploading your submission! Please try again. If the problem persists, contact your teacher.".html_safe ) and return - end + uploader = DropboxUploader.new(submission_path) + uploader.upload(@attachments) end def can_do_auto_check? From 6db770ce286c00c396cb6f1aef22cf5525ebafa8 Mon Sep 17 00:00:00 2001 From: Martijn Stegeman Date: Thu, 9 Jan 2020 12:56:04 +0100 Subject: [PATCH 03/41] fix --- app/controllers/submissions_controller.rb | 12 ++++++------ app/utils/attachments.rb | 4 ++++ app/utils/auto_check_uploader.rb | 4 ++-- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index 7f4c6263..e8821d3f 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -4,11 +4,11 @@ class SubmissionsController < ApplicationController def create collect_attachments - begin + # begin upload_attachments_to_dropbox if Dropbox.available? - rescue - redirect_back(fallback_location: '/', alert: "There was a problem uploading your submission! Please try again. If the problem persists, contact your teacher.".html_safe ) and return - end + # rescue + # redirect_back(fallback_location: '/', alert: "There was a problem uploading your submission! Please try again. If the problem persists, contact your teacher.".html_safe ) and return + # end upload_files_to_check_server if can_do_auto_check? record_submission redirect_back fallback_location: '/' @@ -56,7 +56,7 @@ def upload_attachments_to_dropbox @submit_folder_name) # /mario__21981289 uploader = DropboxUploader.new(submission_path) - uploader.upload(@attachments) + uploader.upload(@attachments.all) end def can_do_auto_check? @@ -64,7 +64,7 @@ def can_do_auto_check? end def upload_files_to_check_server - @token = AutoCheckUploader.new(@attachments, @pset.config['check'], request.host).start + @token = AutoCheckUploader.new(@attachments.zipped, @pset.config['check'], request.host).start end def record_submission diff --git a/app/utils/attachments.rb b/app/utils/attachments.rb index d8b5897a..d62a1359 100644 --- a/app/utils/attachments.rb +++ b/app/utils/attachments.rb @@ -28,6 +28,10 @@ def filenames end end + def all + @files + end + def presentable_file_contents # start with form contents presentable_files = @other_files.to_h diff --git a/app/utils/auto_check_uploader.rb b/app/utils/auto_check_uploader.rb index 8164d5bb..16338607 100644 --- a/app/utils/auto_check_uploader.rb +++ b/app/utils/auto_check_uploader.rb @@ -5,7 +5,7 @@ def self.enabled? end def initialize(attachments, config, host) - @attachments = attachments + @zipped_attachments = attachments @config = config @host = host end @@ -17,7 +17,7 @@ def start begin args = { - file: @attachments.zipped, + file: @zipped_attachments, password: "martijndoeteenphd", webhook: "https://#{@host}/api/check_result/do", multipart: true From 857baa1c9362c5132672eda5f39506682c2bcb32 Mon Sep 17 00:00:00 2001 From: Martijn Stegeman Date: Thu, 9 Jan 2020 13:12:45 +0100 Subject: [PATCH 04/41] fix --- app/utils/auto_check_uploader.rb | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/app/utils/auto_check_uploader.rb b/app/utils/auto_check_uploader.rb index 16338607..642f960e 100644 --- a/app/utils/auto_check_uploader.rb +++ b/app/utils/auto_check_uploader.rb @@ -1,34 +1,37 @@ class AutoCheckUploader def self.enabled? - true + ENV['CHECK_SERVER_URL'].present? && ENV['CHECK_SERVER_SECRET'].present? end def initialize(attachments, config, host) + @server_url = ENV['CHECK_SERVER_URL'] + @server_secret = ENV['CHECK_SERVER_SECRET'] + @zipped_attachments = attachments @config = config @host = host end def start - server = RestClient::Resource.new( - "https://agile008.science.uva.nl/#{@config['tool']}", + endpoint = RestClient::Resource.new( + URI.join(@server_url, @config['tool']).to_s, verify_ssl: OpenSSL::SSL::VERIFY_NONE) begin - args = { + opts = { file: @zipped_attachments, - password: "martijndoeteenphd", + password: @server_secret, webhook: "https://#{@host}/api/check_result/do", multipart: true - # and add slug/repo/args from the config file - }.merge(@config.slice('slug', 'repo', 'args')) + } + # and add slug/repo/args from the config file + config_opts = @config.slice('slug', 'repo', 'args') - response = server.post(args) - Rails.logger.debug JSON.parse(response.body)['id'] + response = endpoint.post(opts.merge(config_opts)) return JSON.parse(response.body)['id'] rescue RestClient::ExceptionWithResponse => e - Rails.logger.debug e.response + return false end end From 3fd13dbab59de5191f370eedd7576fff6e378786 Mon Sep 17 00:00:00 2001 From: Martijn Stegeman Date: Thu, 9 Jan 2020 18:26:37 +0100 Subject: [PATCH 05/41] move components into features folder --- app/controllers/admin/course_controller.rb | 2 +- app/controllers/admin/dropbox_controller.rb | 8 +- app/controllers/admin/site_controller.rb | 4 +- app/controllers/admin/updates_controller.rb | 2 +- app/controllers/api/api_controller.rb | 2 +- app/controllers/page_controller.rb | 197 ------------------ .../schedules/grades_controller.rb | 4 +- app/controllers/submissions_controller.rb | 44 ++-- app/controllers/users_controller.rb | 4 +- .../check_server/uploader.rb} | 2 +- .../course_git.rb => features/course/git.rb} | 10 +- .../course/loader.rb} | 9 +- .../course/tools.rb} | 4 +- .../dropbox.rb => features/dropbox/client.rb} | 12 +- .../dropbox/uploader.rb} | 4 +- .../grading}/final_grade_assigner.rb | 4 +- .../grading}/final_grade_calculator.rb | 2 +- app/{utils => models}/attachments.rb | 0 app/models/submit.rb | 4 +- app/utils/api_provider.rb | 13 -- app/utils/features.rb | 7 - app/views/admin/site/_dropbox.html.erb | 2 +- app/views/admin/site/_index.html.erb | 2 + app/views/schedules/schedules/_tabs.html.erb | 2 +- app/views/users/_title.html.erb | 2 +- config/initializers/default_settings.rb | 2 +- config/initializers/override_git.rb | 7 + 27 files changed, 71 insertions(+), 284 deletions(-) rename app/{utils/auto_check_uploader.rb => features/check_server/uploader.rb} (96%) rename app/{utils/course_git.rb => features/course/git.rb} (88%) rename app/{utils/course_loader.rb => features/course/loader.rb} (99%) rename app/{utils/course_tools.rb => features/course/tools.rb} (97%) rename app/{utils/dropbox.rb => features/dropbox/client.rb} (85%) rename app/{utils/dropbox_uploader.rb => features/dropbox/uploader.rb} (80%) rename app/{utils => features/grading}/final_grade_assigner.rb (91%) rename app/{utils => features/grading}/final_grade_calculator.rb (98%) rename app/{utils => models}/attachments.rb (100%) delete mode 100644 app/utils/api_provider.rb delete mode 100644 app/utils/features.rb create mode 100644 config/initializers/override_git.rb diff --git a/app/controllers/admin/course_controller.rb b/app/controllers/admin/course_controller.rb index 66a36aed..75583405 100644 --- a/app/controllers/admin/course_controller.rb +++ b/app/controllers/admin/course_controller.rb @@ -20,7 +20,7 @@ def index # update the courseware from the linked git repository # def import - errors = CourseLoader.new.run + errors = Course::Loader.new.run logger.debug errors.join('
').inspect if errors.size > 0 redirect_back fallback_location: '/', alert: errors.join('
') diff --git a/app/controllers/admin/dropbox_controller.rb b/app/controllers/admin/dropbox_controller.rb index 2e9ae9f0..cd372089 100644 --- a/app/controllers/admin/dropbox_controller.rb +++ b/app/controllers/admin/dropbox_controller.rb @@ -5,16 +5,12 @@ class Admin::DropboxController < ApplicationController # redirects to dropbox to allow oauth confirmation def connect - if Dropbox.available? - redirect_to Dropbox.get_auth_url(url_for action: 'oauth', protocol: 'https') - else - redirect_back fallback_location: '/', alert: "Dropbox has not been configured server-side." - end + redirect_to Dropbox::Client.get_auth_url(url_for action: 'oauth', protocol: 'https') end # endpoint after dropbox confirmation, checks connection and saves def oauth - Dropbox.process_authorization(params[:code]) + Dropbox::Client.process_authorization(params[:code]) redirect_to :root end diff --git a/app/controllers/admin/site_controller.rb b/app/controllers/admin/site_controller.rb index 41083d73..338fddaf 100644 --- a/app/controllers/admin/site_controller.rb +++ b/app/controllers/admin/site_controller.rb @@ -4,7 +4,7 @@ class Admin::SiteController < ApplicationController before_action :require_admin def index - @dropbox_linked = Dropbox.connected? + @dropbox_linked = Dropbox::Client.connected? @secret = Settings.webhook_secret render_to_modal header: 'Site configuration' @@ -33,7 +33,7 @@ def set_git_repo else Settings.git_repo = params[:repo_url] Settings.git_branch = params[:repo_branch] - CourseLoader.new.run + Course::Loader.new.run redirect_back fallback_location: '/', notice: 'The course content was successfully cloned.' end end diff --git a/app/controllers/admin/updates_controller.rb b/app/controllers/admin/updates_controller.rb index 3d22108c..e676e405 100644 --- a/app/controllers/admin/updates_controller.rb +++ b/app/controllers/admin/updates_controller.rb @@ -7,7 +7,7 @@ class Admin::UpdatesController < ApplicationController # update the courseware from the linked git repository # def create - errors = CourseLoader.new.run + errors = Course::Loader.new.run logger.debug errors.join('
').inspect if errors.size > 0 redirect_back fallback_location: '/', alert: errors.join('
') diff --git a/app/controllers/api/api_controller.rb b/app/controllers/api/api_controller.rb index 6648f94e..55bdf042 100644 --- a/app/controllers/api/api_controller.rb +++ b/app/controllers/api/api_controller.rb @@ -6,7 +6,7 @@ class Api::ApiController < ApplicationController before_action :restrict_access, only: :reload def reload - CourseLoader.new.run + Course::Loader.new.run head :ok end diff --git a/app/controllers/page_controller.rb b/app/controllers/page_controller.rb index a85be6c2..8773a801 100644 --- a/app/controllers/page_controller.rb +++ b/app/controllers/page_controller.rb @@ -1,18 +1,3 @@ -# require 'dropbox' -# -# class LUploadIO < StringIO -# # string serves as in-memory container for a zipfile -# -# def initialize(name) -# @path = name -# super() # the () is essential, calls no-arg initializer -# end -# -# def path -# @path -# end -# end - class PageController < ApplicationController before_action :authorize, if: :request_from_local_network? @@ -45,186 +30,4 @@ def section raise ActionController::RoutingError.new('Not Found') if !@section || @section.content_page.blank? end - # def submit - # - # - # # # we may get here after expiry of the session if someone doesn't reload now and then - # # if not logged_in? - # # redirect_back(fallback_location: '/', alert: 'Please login again before submitting.') and return - # # end - # - # # is the total upload size acceptable? (10MiB) - # if request.content_length > 9999999 - # redirect_back(fallback_location: '/', alert: "Your files are too big somehow! Please check what you're uploading or ask your teacher.") and return - # end - # - # page = Page.find(params[:page_id]) - # pset = page.pset - # - # folder_name = pset.name + "__" + Time.now.to_i.to_s - # - # if (pset.form || pset.files.any?) && (!Dropbox.connected? || Settings.dropbox_folder_name.blank?) - # redirect_back(fallback_location: '/', alert: 'There is a problem with submitting! Warn your teacher immediately and mention Dropbox.'.html_safe) and return - # end - # - # form_text = render_form_text(params[:a]) - # - # # - # # upload everything to dropbox - # # - # if pset.form || pset.files - # begin - # upload_to_dropbox(current_user.login_id, current_user.name, - # Settings.dropbox_folder_name, folder_name, params[:notes], form_text, params[:f]) - # rescue - # redirect_back(fallback_location: '/', alert: "There was a problem uploading your submission! Please try again. If the problem persists, contact your teacher.".html_safe ) and return - # end - # end - # - # # - # # create submit record - # # - # submit = Submit.where(:user_id => current_user.id, :pset_id => pset.id).first_or_initialize - # submit.submitted_at = Time.now - # submit.used_login = current_user.login_id - # submit.url = params[:url] - # submit.folder_name = folder_name - # submit.check_feedback = nil - # submit.style_feedback = nil - # submit.auto_graded = false - # submit.submitted_files = params.permit(f: {})[:f].to_h.map { |file,info| info.original_filename } if params[:f] - # if files = params[:f] - # file_contents = {} - # files.each do |filename, file| - # name = file.original_filename - # if text_file?(name) - # if file.size < 60000 - # file.rewind and file_contents[name] = file.read - # else - # file_contents[name] = "Uploaded file was too large!" - # end - # end - # end - # end - # submit.file_contents = file_contents - # submit.save - # - # # - # # create or touch submit for associated module, if possible/needed - # # - # if pset.parent_mod.present? && pset.parent_mod.pset.present? - # mod_submit = Submit.where(:user_id => current_user.id, :pset_id => pset.parent_mod.pset.id).first_or_initialize - # if mod_submit.persisted? - # mod_submit.touch(:submitted_at) - # else - # mod_submit.submitted_at = Time.now - # mod_submit.save - # end - # end - # - # # - # # get files to check server - # # - # if pset.config['check'] && files = params[:f] - # submitted_zips = files.keys.select { |x| x.end_with?(".zip") } - # if submitted_zips.any? - # zipfile = files[submitted_zips[0]] - # zipfile.rewind - # else - # zipfile = Zip::OutputStream.write_buffer(::LUploadIO.new('file.zip')) do |zio| - # files.each do |filename, file| - # zio.put_next_entry(filename) - # file.rewind - # zio.write file.read - # end - # end - # zipfile.rewind - # end - # - # server = RestClient::Resource.new( - # "https://agile008.science.uva.nl/#{pset.config['check']['tool']}", - # :verify_ssl => OpenSSL::SSL::VERIFY_NONE - # ) - # - # begin - # args = { - # file: zipfile, - # password: "martijndoeteenphd", - # webhook: "https://#{request.host}/api/check_result/do", - # multipart: true - # # and add slug/repo/args from the config file - # }.merge(pset.config['check'].slice('slug', 'repo', 'args')) - # response = server.post(args) - # logger.debug JSON.parse(response.body)['id'] - # logger.debug submit.inspect - # submit.check_token = JSON.parse(response.body)['id'] - # submit.save - # rescue RestClient::ExceptionWithResponse => e - # logger.debug e.response - # end - # - # end - # - # # - # # this is a re-submit, so re-open for grading - # # - # if submit.grade - # submit.grade.grade = nil - # submit.grade.unfinished! - # end - # - # # - # # success, get back to previous page - # # - # redirect_back fallback_location: '/' - # end - # - # private - # - # def text_file?(name) - # return [".py", ".c", ".txt", ".html", ".css", ".h", ".java"].include?(File.extname(name)) || name == "Makefile" - # end - # - # # writes hash with form contents to a plain text string - # def render_form_text(form) - # form_text = nil - # if form - # form_text = "" - # form.each do |key, value| - # form_text += "#{key}\n\n" - # form_text += "#{value}\n\n" - # end - # end - # return form_text - # end - # - # def upload_to_dropbox(user, name, course, item, notes, form, files) - # - # dropbox_client = Dropbox.client - # dropbox_root = "Submit" - # - # # cache timestamp for folder name - # item_folder = item - # - # # compose info.txt file contents - # info = "student_login_id = " + user - # info += ("\nname = " + name) if name - # info += "\n\n" - # info += notes if notes - # - # # upload the notes - # dropbox_client.upload(File.join("/", dropbox_root, course, user, item_folder, 'info.txt'), info) if notes - # - # # upload the form - # dropbox_client.upload(File.join("/", dropbox_root, course, user, item_folder, 'form.md'), form) if form - # - # # upload all posted files - # if files - # files.each do |filename, file| - # dropbox_client.upload(File.join("/", dropbox_root, course, user, item_folder, file.original_filename), file.read, autorename: true) - # end - # end - # - # end - end diff --git a/app/controllers/schedules/grades_controller.rb b/app/controllers/schedules/grades_controller.rb index 2229cbb2..97e5b4b6 100644 --- a/app/controllers/schedules/grades_controller.rb +++ b/app/controllers/schedules/grades_controller.rb @@ -66,9 +66,9 @@ def publish_all def assign_all_final # feature has to be enabled by supplying a grading.yml - raise ActionController::RoutingError.new('Not Found') if not FinalGradeAssigner.available? + raise ActionController::RoutingError.new('Not Found') if not Grading::FinalGradeAssigner.available? @schedule.users.each do |student| - FinalGradeAssigner.assign_final_grade(student, @current_user) + Grading::FinalGradeAssigner.assign_final_grade(student, @current_user) end redirect_back fallback_location: '/' end diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index e8821d3f..2fbec0f5 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -3,15 +3,18 @@ class SubmissionsController < ApplicationController before_action :load_pset, :validate_attachment_size def create - collect_attachments - # begin - upload_attachments_to_dropbox if Dropbox.available? - # rescue - # redirect_back(fallback_location: '/', alert: "There was a problem uploading your submission! Please try again. If the problem persists, contact your teacher.".html_safe ) and return - # end - upload_files_to_check_server if can_do_auto_check? - record_submission - redirect_back fallback_location: '/' + begin + collect_attachments + upload_attachments_to_dropbox if should_upload_to_dropbox? + upload_files_to_check_server if should_perform_auto_check? + record_submission + redirect_back fallback_location: '/' + rescue + redirect_back( + fallback_location: '/', + alert: "There was a problem uploading your submission! Please try again. " \ + "If the problem persists, contact your teacher.".html_safe) + end end private @@ -50,28 +53,33 @@ def upload_attachments_to_dropbox submission_path = File.join( '/', - 'Submit', # /Submit - Settings.dropbox_folder_name, # /course name - current_user.login_id, # /student ID - @submit_folder_name) # /mario__21981289 + Settings.dropbox_base_folder, # /Submit + Settings.dropbox_course_folder, # /course name + current_user.login_id, # /student ID + @submit_folder_name) # /mario__21981289 - uploader = DropboxUploader.new(submission_path) + uploader = Dropbox::Uploader.new(submission_path) uploader.upload(@attachments.all) end - def can_do_auto_check? - AutoCheckUploader.enabled? && @pset.config['check'].present? + def should_upload_to_dropbox? + # dropbox is generally skipped in development mode! + Rails.env.production? && Dropbox::Client.available? + end + + def should_perform_auto_check? + CheckServer::Uploader.enabled? && @pset.config['check'].present? end def upload_files_to_check_server - @token = AutoCheckUploader.new(@attachments.zipped, @pset.config['check'], request.host).start + @token = CheckServer::Uploader.new(@attachments.zipped, @pset.config['check'], request.host).start end def record_submission submit = Submit.where(user: current_user, pset: @pset).first_or_initialize submit.record( used_login: current_user.login_id, - dropbox_folder_name: @submit_folder_name, + archive_folder_name: @submit_folder_name, url: params[:url], attachments: @attachments, check_token: @token) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 7fcf751c..fd1a6a22 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -58,9 +58,9 @@ def assign_schedule def calculate_final_grade # feature has to be enabled by supplying a grading.yml - raise ActionController::RoutingError.new('Not Found') if not FinalGradeAssigner.available? + raise ActionController::RoutingError.new('Not Found') if not Grading::FinalGradeAssigner.available? student = User.find(params[:user_id]) - result = FinalGradeAssigner.assign_final_grade(student, current_user) + result = Grading::FinalGradeAssigner.assign_final_grade(student, current_user) redirect_back fallback_location: '/' end diff --git a/app/utils/auto_check_uploader.rb b/app/features/check_server/uploader.rb similarity index 96% rename from app/utils/auto_check_uploader.rb rename to app/features/check_server/uploader.rb index 642f960e..46df3c27 100644 --- a/app/utils/auto_check_uploader.rb +++ b/app/features/check_server/uploader.rb @@ -1,4 +1,4 @@ -class AutoCheckUploader +class CheckServer::Uploader def self.enabled? ENV['CHECK_SERVER_URL'].present? && ENV['CHECK_SERVER_SECRET'].present? diff --git a/app/utils/course_git.rb b/app/features/course/git.rb similarity index 88% rename from app/utils/course_git.rb rename to app/features/course/git.rb index bc8d3088..c5f2eddf 100644 --- a/app/utils/course_git.rb +++ b/app/features/course/git.rb @@ -2,15 +2,7 @@ # Get remote git data, either by pulling existing, or cloning anew. # -class Git::Lib - - def update_submodules - command 'submodule update --remote' - end - -end - -module CourseGit +module Course::Git def self.pull if git = self.existing_local_repo diff --git a/app/utils/course_loader.rb b/app/features/course/loader.rb similarity index 99% rename from app/utils/course_loader.rb rename to app/features/course/loader.rb index 039e7b17..456ce0b5 100644 --- a/app/utils/course_loader.rb +++ b/app/features/course/loader.rb @@ -1,7 +1,4 @@ -# require 'course_git' -# require 'course_tools' - -class CourseLoader +class Course::Loader # This class is responsible for importing course information from # the source into the database. @@ -38,7 +35,7 @@ def run recreate_all_slugs # put psets in order - CourseTools.clean_psets + Course::Tools.clean_psets rescue SQLite3::BusyException @errors << "A timeout occurred while loading the new course content. Just try again!" end @@ -88,7 +85,7 @@ def recreate_all_slugs # overridden in an initializer in order to function well! # def update_repo(dir) - if !CourseGit.pull + if !Course::Git.pull @errors << "Repo could not be updated from remote. You can simply try again." end end diff --git a/app/utils/course_tools.rb b/app/features/course/tools.rb similarity index 97% rename from app/utils/course_tools.rb rename to app/features/course/tools.rb index 3aa62574..04e5245f 100644 --- a/app/utils/course_tools.rb +++ b/app/features/course/tools.rb @@ -1,10 +1,10 @@ -class CourseTools +class Course::Tools # # # Walks all psets named in course.yml and ranks them in the database - def CourseTools.clean_psets + def self.clean_psets # the structure of the 'psets' info in course.yml can differ if Settings['psets'].class == Array diff --git a/app/utils/dropbox.rb b/app/features/dropbox/client.rb similarity index 85% rename from app/utils/dropbox.rb rename to app/features/dropbox/client.rb index 7d66bb00..c6e1b581 100644 --- a/app/utils/dropbox.rb +++ b/app/features/dropbox/client.rb @@ -1,23 +1,25 @@ -class Dropbox +class Dropbox::Client @@dropbox_key = ENV['DROPBOX_KEY'] @@dropbox_secret = ENV['DROPBOX_SECRET'] @@dropbox_access_type = ENV['DROPBOX_ACCESS_TYPE'] - @@root_folder = "/Submit" - @@client = nil @@return_url = nil # check basic requirements for this API to function def self.available? - return Rails.env.production? && @@dropbox_key.present? && @@dropbox_secret.present? && @@dropbox_access_type.present? + return @@dropbox_key.present? && @@dropbox_secret.present? && @@dropbox_access_type.present? end # see if we're already connected (dropbox connections do not expire) def self.connected? return !!Settings['dropbox.session'] end + + def self.configured? + Settings.dropbox_base_folder.present? && Settings.dropbox_course_folder + end # get a reference to a client object def self.client @@ -25,7 +27,7 @@ def self.client end def self.root_folder - @@root_folder + Settings['dropbox_base_folder'] end # is able to download a (small?) file by path; only returns contents, no metadata diff --git a/app/utils/dropbox_uploader.rb b/app/features/dropbox/uploader.rb similarity index 80% rename from app/utils/dropbox_uploader.rb rename to app/features/dropbox/uploader.rb index cee3636a..f2386a86 100644 --- a/app/utils/dropbox_uploader.rb +++ b/app/features/dropbox/uploader.rb @@ -1,11 +1,11 @@ -class DropboxUploader +class Dropbox::Uploader def initialize(base_path) @base_path = base_path end def upload(files) - dropbox = Dropbox.client + dropbox = Dropbox::Client.client files.each do |filename, file| filename = File.join(@base_path, file.original_filename) dropbox.upload(filename, file.read, autorename: true) diff --git a/app/utils/final_grade_assigner.rb b/app/features/grading/final_grade_assigner.rb similarity index 91% rename from app/utils/final_grade_assigner.rb rename to app/features/grading/final_grade_assigner.rb index 93b1b519..9ac201b2 100644 --- a/app/utils/final_grade_assigner.rb +++ b/app/features/grading/final_grade_assigner.rb @@ -1,11 +1,11 @@ -module FinalGradeAssigner +module Grading::FinalGradeAssigner def self.available? !!Settings['grading'] && !!Settings['grading']['calculation'] end def self.assign_final_grade(student, grader) - grades = FinalGradeCalculator.run_for(student.all_submits) + grades = Grading::FinalGradeCalculator.run_for(student.all_submits) grades.each do |name, grade| grade = number_grade(grade) if grade.present? # there really is an assignable grade diff --git a/app/utils/final_grade_calculator.rb b/app/features/grading/final_grade_calculator.rb similarity index 98% rename from app/utils/final_grade_calculator.rb rename to app/features/grading/final_grade_calculator.rb index 360cda81..6c142c25 100644 --- a/app/utils/final_grade_calculator.rb +++ b/app/features/grading/final_grade_calculator.rb @@ -1,4 +1,4 @@ -module FinalGradeCalculator +module Grading::FinalGradeCalculator # FinalGradeCalculator.run_for(User.first) diff --git a/app/utils/attachments.rb b/app/models/attachments.rb similarity index 100% rename from app/utils/attachments.rb rename to app/models/attachments.rb diff --git a/app/models/submit.rb b/app/models/submit.rb index 11bf86d7..a475a892 100644 --- a/app/models/submit.rb +++ b/app/models/submit.rb @@ -61,11 +61,11 @@ def last_submitted submitted_at && submitted_at.to_formatted_s(:short) || "never" end - def record(used_login: nil, dropbox_folder_name: nil, url: nil, attachments: nil, check_token: nil) + def record(used_login: nil, archive_folder_name: nil, url: nil, attachments: nil, check_token: nil) # basic info self.submitted_at = Time.now self.used_login = used_login - self.folder_name = dropbox_folder_name + self.folder_name = archive_folder_name # attachments self.url = url diff --git a/app/utils/api_provider.rb b/app/utils/api_provider.rb deleted file mode 100644 index a003ab2e..00000000 --- a/app/utils/api_provider.rb +++ /dev/null @@ -1,13 +0,0 @@ -module ApiProvider - - @@api_token = ENV['COURSESITE_API_TOKEN'] - - def self.available? - return @@api_token.present? - end - - def self.check_token(token) - return @@api_token == token - end - -end diff --git a/app/utils/features.rb b/app/utils/features.rb deleted file mode 100644 index e5bcdc9a..00000000 --- a/app/utils/features.rb +++ /dev/null @@ -1,7 +0,0 @@ -class Features - - # def self.slack_integration? - # !ENV['SLACK_WEBHOOK'].blank? - # end - -end diff --git a/app/views/admin/site/_dropbox.html.erb b/app/views/admin/site/_dropbox.html.erb index 8eaefdda..7e6a5202 100644 --- a/app/views/admin/site/_dropbox.html.erb +++ b/app/views/admin/site/_dropbox.html.erb @@ -8,4 +8,4 @@ <% end %>

-<%= change_setting_form('dropbox_folder_name', 'Dropbox folder name to save submissions to (/Dropbox/Submits/[name])') %> +<%= change_setting_form('dropbox_course_folder', 'Dropbox folder name to save submissions to (/Dropbox/Submits/[name])') %> diff --git a/app/views/admin/site/_index.html.erb b/app/views/admin/site/_index.html.erb index fe0ca507..c2ffe2f1 100644 --- a/app/views/admin/site/_index.html.erb +++ b/app/views/admin/site/_index.html.erb @@ -4,7 +4,9 @@ <%= render partial: 'settings', layout: 'modal_card', locals: { title: 'Settings', subtitle: nil }%> +<% if Dropbox::Client.available? %> <%= render partial: 'dropbox', layout: 'modal_card', locals: { title: 'Dropbox', subtitle: nil }%> +<% end %> <%= render partial: 'webhook', layout: 'modal_card', locals: { title: 'Webhook', subtitle: nil }%> diff --git a/app/views/schedules/schedules/_tabs.html.erb b/app/views/schedules/schedules/_tabs.html.erb index 4bf91293..c2a34deb 100644 --- a/app/views/schedules/schedules/_tabs.html.erb +++ b/app/views/schedules/schedules/_tabs.html.erb @@ -42,7 +42,7 @@ <%= link_to 'Notify students who did not submit yet'.html_safe, form_for_missing_schedule_submits_path(schedule_id: @current_module_id), class:"dropdown-item", remote: true %> <% end %> <% end %> - <% if current_user.admin? && FinalGradeAssigner.available? %> + <% if current_user.admin? && Grading::FinalGradeAssigner.available? %>
<%= link_to 'Calculate final grades'.html_safe, assign_all_final_schedule_grades_path(schedule_id: @current_module_id), method: :put, data: { confirm:'Are you sure?' }, class:"dropdown-item" %> <% end %> diff --git a/app/views/users/_title.html.erb b/app/views/users/_title.html.erb index 914d7f2f..d2129c6d 100644 --- a/app/views/users/_title.html.erb +++ b/app/views/users/_title.html.erb @@ -61,7 +61,7 @@ <%= link_to 'Make admin', admin_user_set_role_path(@student, user: { role: :admin }), method: :patch, remote: true, class: 'dropdown-item' %> <%= link_to 'Undo admin', admin_user_set_role_path(@student, user: { role: :student, schedule_id: nil }), method: :patch, remote: true, class: 'dropdown-item' %>
- <%= link_to_if FinalGradeAssigner.available?, 'Calculate final grade', user_calculate_final_grade_path(@student), method: :post, class: 'dropdown-item' %> + <%= link_to_if Grading::FinalGradeAssigner.available?, 'Calculate final grade', user_calculate_final_grade_path(@student), method: :post, class: 'dropdown-item' %> <% end %> diff --git a/config/initializers/default_settings.rb b/config/initializers/default_settings.rb index dc94a4f0..6d7c38cb 100644 --- a/config/initializers/default_settings.rb +++ b/config/initializers/default_settings.rb @@ -1 +1 @@ -Settings.defaults['dropbox.root_folder'] = 'Submit' +Settings.defaults['dropbox_base_folder'] = 'Submit' diff --git a/config/initializers/override_git.rb b/config/initializers/override_git.rb new file mode 100644 index 00000000..944fb046 --- /dev/null +++ b/config/initializers/override_git.rb @@ -0,0 +1,7 @@ +class Git::Lib + + def update_submodules + command 'submodule update --remote' + end + +end From cd10cfe2519f05e591338c50720cd292fb3d5f7c Mon Sep 17 00:00:00 2001 From: Martijn Stegeman Date: Tue, 14 Jan 2020 20:35:06 +0100 Subject: [PATCH 06/41] clean up submit model --- app/concerns/auto_check_feedback_formatter.rb | 72 ++++++ app/concerns/auto_check_receiver.rb | 27 +++ app/concerns/auto_check_score_interpreter.rb | 75 ++++++ .../auto_check_sender.rb} | 2 +- .../api/check_result_controller.rb | 6 +- app/controllers/profile_controller.rb | 2 +- app/controllers/submissions_controller.rb | 4 +- app/helpers/grades_helper.rb | 4 + app/models/grade.rb | 2 +- app/models/submit.rb | 216 +----------------- app/models/user.rb | 2 +- app/views/grades/_form.html.erb | 2 +- app/views/grades/_show.html.erb | 2 +- app/views/grading/show.html.erb | 8 +- app/views/items/_submit.html.erb | 2 +- app/views/submits/_grading_info.erb | 2 +- app/views/submits/_info.html.erb | 2 +- 17 files changed, 200 insertions(+), 230 deletions(-) create mode 100644 app/concerns/auto_check_feedback_formatter.rb create mode 100644 app/concerns/auto_check_receiver.rb create mode 100644 app/concerns/auto_check_score_interpreter.rb rename app/{features/check_server/uploader.rb => concerns/auto_check_sender.rb} (96%) diff --git a/app/concerns/auto_check_feedback_formatter.rb b/app/concerns/auto_check_feedback_formatter.rb new file mode 100644 index 00000000..1a128ee5 --- /dev/null +++ b/app/concerns/auto_check_feedback_formatter.rb @@ -0,0 +1,72 @@ +module AutoCheckFeedbackFormatter + + extend ActiveSupport::Concern + + def has_auto_feedback? + return false if not self.check_results + (self.check_results.keys & ["check50v2", "check50", "checkpy", "check50v3"]).any? + end + + def formatted_auto_feedback + check_results = self.check_results + + result = "" + items = nil + v3=nil + + # each tool has different output formats that we detect here + + check_results.keys.each do |tool| + # puts tool + # puts check_results[tool].is_a?(Array) + case tool + when "check50v2" + v3=false + items = check_results[tool] + when "check50" + v3=true + items = check_results[tool]["results"] + return check_results[tool]["error"]["value"] if items.nil? + when "check50v3" + v3=true + items = check_results[tool]["results"] + return check_results[tool]["error"]["value"] if items.nil? + when "checkpy" + if check_results[tool].is_a?(Array) + # puts "ARR" + v3=true + items = check_results[tool].collect {|f| f["results"]}.compact.flatten + elsif check_results[tool].is_a?(Hash) + v3=true + items = [check_results[tool]].collect {|f| f["results"]}.flatten + end + end + + # puts items + end + + return "" if items.nil? + + # now generate basic feedback + + items.each do |item| + # puts item + case v3 && item["passed"] || item["status"] + when true + result << ":)" + when false + # puts "FALSE" + result << ":(" + when nil + result << ":|" + end + result << " " + item["description"] + "\n" + if item["cause"].present? + result << " " + item["cause"]["rationale"] + "\n" + end + end + + return result + end + +end diff --git a/app/concerns/auto_check_receiver.rb b/app/concerns/auto_check_receiver.rb new file mode 100644 index 00000000..ba77f173 --- /dev/null +++ b/app/concerns/auto_check_receiver.rb @@ -0,0 +1,27 @@ +module AutoCheckReceiver + + extend ActiveSupport::Concern + + def register_auto_check_results(json) + # put results into db + self.check_token = nil + self.check_results = json + + # create a create if needed + grade = self.grade || self.build_grade + + # check via the grade if this submit is OK + grade.reset_automatic_grades(self.automatic_scores) + grade.set_calculated_grade + grade.status = Grade.statuses[:published] + grade.save + + self.save + + # if not OK, send an e-mail + if grade.calculated_grade == 0 + GradeMailer.bad_submit(self).deliver + end + end + +end diff --git a/app/concerns/auto_check_score_interpreter.rb b/app/concerns/auto_check_score_interpreter.rb new file mode 100644 index 00000000..9a842d60 --- /dev/null +++ b/app/concerns/auto_check_score_interpreter.rb @@ -0,0 +1,75 @@ +module AutoCheckScoreInterpreter + + extend ActiveSupport::Concern + + def automatic_scores + # puts "HIER #{self.inspect}" + f = pset.config + return {} if f.nil? || f['automatic'].nil? + + # take all automatic rules and use it to create hash of grades + results = f['automatic'].transform_values do |rule| + # logger.debug rule + begin + self.instance_eval(rule) + rescue + # puts "FAIL" + nil + end + end + + return results + end + + def correctness_score + check_results = self.check_results + return nil if !self.check_results.present? + + check_results.keys.each do |tool| + puts tool + puts check_results[tool].inspect + case tool + when "check50v2" + return check_results[tool].count { |x| x["status"].present? } / check_results[tool].size.to_f + when "check50", "check50v3" + return nil if check_results[tool]["error"].present? + return check_results[tool]["results"].count { |x| x["passed"].present? } / check_results[tool]["results"].size.to_f + when "checkpy" + if check_results[tool].is_a?(Array) + puts "arr" + return check_results[tool].collect { |f| f["nPassed"] }.sum + elsif check_results[tool].is_a?(Hash) + puts "hash" + return [check_results[tool]].collect { |f| f["nPassed"] }.sum + end + end + end + + # didn't document what kind of tool generates the below + # elsif self.check_feedback.is_a?(Array) && self.check_feedback[0].is_a?(Array) + # fb = self.check_feedback.flatten(1) + # fb.count { |x| x["status"].present? } / fb.size.to_f + end + + def style_score + check_results = self.check_results + check_results.keys.each do |tool| + case tool + when "style50" + case check_results[tool] + when 0.0..0.2 + return 1 + when 0.2..0.5 + return 2 + when 0.5..0.8 + return 3 + when 0.8..0.9999 + return 4 + when 1.0 + return 5 + end + end + end + end + +end diff --git a/app/features/check_server/uploader.rb b/app/concerns/auto_check_sender.rb similarity index 96% rename from app/features/check_server/uploader.rb rename to app/concerns/auto_check_sender.rb index 46df3c27..d4a4475b 100644 --- a/app/features/check_server/uploader.rb +++ b/app/concerns/auto_check_sender.rb @@ -1,4 +1,4 @@ -class CheckServer::Uploader +class AutoCheckSender def self.enabled? ENV['CHECK_SERVER_URL'].present? && ENV['CHECK_SERVER_SECRET'].present? diff --git a/app/controllers/api/check_result_controller.rb b/app/controllers/api/check_result_controller.rb index f4adc4cc..0bc2a455 100644 --- a/app/controllers/api/check_result_controller.rb +++ b/app/controllers/api/check_result_controller.rb @@ -6,10 +6,8 @@ def do submit = Submit.find_by_check_token(params["id"]) if submit results = params["result"] - submit.register_check_results(results.to_json) - render json: "Hi!" - else - render json: "Bye!" + submit.register_auto_check_results(results) + head :ok end end diff --git a/app/controllers/profile_controller.rb b/app/controllers/profile_controller.rb index 269bc260..a2f344e9 100644 --- a/app/controllers/profile_controller.rb +++ b/app/controllers/profile_controller.rb @@ -8,7 +8,7 @@ def show def feedback submit = Submit.find(params[:submit_id]) - @formatted_feedback = submit.check_feedback_formatted + @formatted_feedback = submit.formatted_auto_feedback respond_to do |format| format.js do render 'feedback' diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index 2fbec0f5..0dc5b2d3 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -68,11 +68,11 @@ def should_upload_to_dropbox? end def should_perform_auto_check? - CheckServer::Uploader.enabled? && @pset.config['check'].present? + AutoCheckSender.enabled? && @pset.config['check'].present? end def upload_files_to_check_server - @token = CheckServer::Uploader.new(@attachments.zipped, @pset.config['check'], request.host).start + @token = AutoCheckSender.new(@attachments.zipped, @pset.config['check'], request.host).start end def record_submission diff --git a/app/helpers/grades_helper.rb b/app/helpers/grades_helper.rb index 5dcffc5d..4a23da7e 100644 --- a/app/helpers/grades_helper.rb +++ b/app/helpers/grades_helper.rb @@ -1,5 +1,9 @@ module GradesHelper + def some_time_or_never(time) + time && time.to_formatted_s(:short) || "never" + end + def color_for_filename(filename, potential) potential.include?(filename) && 'text-success' || 'text-danger' end diff --git a/app/models/grade.rb b/app/models/grade.rb index 84dae067..fc155e21 100644 --- a/app/models/grade.rb +++ b/app/models/grade.rb @@ -29,7 +29,7 @@ class Grade < ApplicationRecord after_initialize do if !self.persisted? # add any newly found autogrades to the subgrades as default - self.submit.automatic().to_h.each do |k,v| + self.submit.automatic_scores.to_h.each do |k,v| self.subgrades[k] = v if not self.subgrades[k].present? end end diff --git a/app/models/submit.rb b/app/models/submit.rb index a475a892..d40b8431 100644 --- a/app/models/submit.rb +++ b/app/models/submit.rb @@ -1,4 +1,8 @@ class Submit < ApplicationRecord + + include AutoCheckReceiver + include AutoCheckScoreInterpreter + include AutoCheckFeedbackFormatter belongs_to :user delegate :name, to: :user, prefix: true, allow_nil: true @@ -13,8 +17,6 @@ class Submit < ApplicationRecord delegate :last_graded, to: :grade, allow_nil: true serialize :submitted_files - serialize :check_feedback - serialize :style_feedback serialize :file_contents serialize :check_results @@ -35,32 +37,6 @@ class Submit < ApplicationRecord order('submits.created_at asc') end - before_save do |s| - if s.check_feedback_changed? || s.style_feedback_changed? - # TODO this is hardcoded to having keys "correctness" and "style" in the autograder - # which shouldn't be neccessary - s.auto_graded = s.pset.config["automatic"].collect do |k,v| - case k - when "correctness", "points" - s.check_feedback.present? - when "style" - s.style_feedback.present? - else - true - end - end.all? - end - true - end - - def graded? - return (self.grade && (!self.grade.grade.blank? || !self.grade.calculated_grade.blank?)) - end - - def last_submitted - submitted_at && submitted_at.to_formatted_s(:short) || "never" - end - def record(used_login: nil, archive_folder_name: nil, url: nil, attachments: nil, check_token: nil) # basic info self.submitted_at = Time.now @@ -74,8 +50,7 @@ def record(used_login: nil, archive_folder_name: nil, url: nil, attachments: nil # reset auto checks self.check_token = check_token - self.check_feedback = nil - self.style_feedback = nil + self.check_results = nil self.auto_graded = false self.save @@ -89,185 +64,4 @@ def record(used_login: nil, archive_folder_name: nil, url: nil, attachments: nil self.grade.update_columns(grade: nil, status: Grade.statuses[:unfinished]) if self.grade end - def automatic - puts "HIER #{self.inspect}" - f = pset.config - return {} if f.nil? || f['automatic'].nil? - - # take all automatic rules and use it to create hash of grades - results = f['automatic'].transform_values do |rule| - logger.debug rule - begin - self.instance_eval(rule) - rescue - puts "FAIL" - nil - end - end - - return results - end - - def check_score - check_results = JSON(self.check_results) - return nil if !self.check_results.present? - - check_results.keys.each do |tool| - puts tool - puts check_results[tool].inspect - case tool - when "check50v2" - return check_results[tool].count { |x| x["status"].present? } / check_results[tool].size.to_f - when "check50", "check50v3" - return nil if check_results[tool]["error"].present? - return check_results[tool]["results"].count { |x| x["passed"].present? } / check_results[tool]["results"].size.to_f - when "checkpy" - if check_results[tool].is_a?(Array) - puts "arr" - return check_results[tool].collect { |f| f["nPassed"] }.sum - elsif check_results[tool].is_a?(Hash) - puts "hash" - return [check_results[tool]].collect { |f| f["nPassed"] }.sum - end - end - end - - # didn't document what kind of tool generates the below - # elsif self.check_feedback.is_a?(Array) && self.check_feedback[0].is_a?(Array) - # fb = self.check_feedback.flatten(1) - # fb.count { |x| x["status"].present? } / fb.size.to_f - end - - def style_score - check_results = JSON(self.check_results) - check_results.keys.each do |tool| - case tool - when "style50" - case check_results[tool] - when 0.0..0.2 - return 1 - when 0.2..0.5 - return 2 - when 0.5..0.8 - return 3 - when 0.8..0.9999 - return 4 - when 1.0 - return 5 - end - end - end - end - - def register_check_results(json) - # put results into db - self.check_token = nil - self.check_results = json - - # create a create if needed - grade = self.grade || self.build_grade - - # check via the grade if this submit is OK - grade.reset_automatic_grades(self.automatic) - grade.set_calculated_grade - grade.status = Grade.statuses[:published] - grade.save - - self.save - - # if not OK, send an e-mail - if grade.calculated_grade == 0 - GradeMailer.bad_submit(self).deliver - end - end - - def check_feedback_problems? - return false if self.check_feedback.blank? - - self.check_feedback.index { |x| x["status"].blank? }.present? - end - - def has_feedback? - return false if not self.check_results - check_results = JSON(self.check_results) - (check_results.keys & ["check50v2", "check50", "checkpy", "check50v3"]).any? - end - - def check_feedback_formatted - puts "HAHA" - check_results = JSON(self.check_results) - - result = "" - items = nil - v3=nil - - check_results.keys.each do |tool| - puts tool - puts check_results[tool].is_a?(Array) - case tool - when "check50v2" - v3=false - items = check_results[tool] - when "check50" - v3=true - items = check_results[tool]["results"] - return check_results[tool]["error"]["value"] if items.nil? - when "check50v3" - v3=true - items = check_results[tool]["results"] - return check_results[tool]["error"]["value"] if items.nil? - when "checkpy" - if check_results[tool].is_a?(Array) - puts "ARR" - v3=true - items = check_results[tool].collect {|f| f["results"]}.compact.flatten - elsif check_results[tool].is_a?(Hash) - v3=true - items = [check_results[tool]].collect {|f| f["results"]}.flatten - end - end - - puts items - end - - return "" if items.nil? - - # result = "" - - # if self.check_feedback.is_a?(Hash) && self.check_feedback["version"] && self.check_feedback["version"].start_with?("3") - # v3=true - # items = self.check_feedback["results"] - # return self.check_feedback["error"]["value"] if items.nil? - # elsif self.check_feedback.is_a?(Array) && self.check_feedback[0].is_a?(Hash) && self.check_feedback[0]["nTests"].is_a?(Integer) - # # checkpy multiple tests (module) - # elsif self.check_feedback.is_a?(Hash) && self.check_feedback["nTests"].is_a?(Integer) - # # checkpy single test - # elsif self.check_feedback.is_a?(Array) && self.check_feedback[0].is_a?(Array) - # v3=false - # items = self.check_feedback.flatten(1) - # else - # end - - puts items.inspect - puts v3 - items.each do |item| - puts item - case v3 && item["passed"] || item["status"] - when true - result << ":)" - when false - puts "FALSE" - result << ":(" - when nil - result << ":|" - end - result << " " + item["description"] + "\n" - if item["cause"].present? - result << " " + item["cause"]["rationale"] + "\n" - end - end - - return result - end - end diff --git a/app/models/user.rb b/app/models/user.rb index 8add0ddc..a8ed4b45 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -159,7 +159,7 @@ def files_for_module(mod) self.submits.where(pset: mod.psets).each do |submit| if submit.file_contents submit.file_contents.each do |filename, contents| - files["(#{submit.check_score}) #{submit.pset.name}/#{filename}"] = contents + files["(#{submit.correctness_score}) #{submit.pset.name}/#{filename}"] = contents end end end diff --git a/app/views/grades/_form.html.erb b/app/views/grades/_form.html.erb index 7f629002..91aa556e 100644 --- a/app/views/grades/_form.html.erb +++ b/app/views/grades/_form.html.erb @@ -35,7 +35,7 @@ <% @submit.pset.config['subgrades'].each do |subgrade_name, subgrade_type| %> <% if subgrade_type == 'integer' || subgrade_type == 'float' %>
- +
<% elsif subgrade_type == 'boolean' %> diff --git a/app/views/grades/_show.html.erb b/app/views/grades/_show.html.erb index 907f7d1c..6b9869a5 100644 --- a/app/views/grades/_show.html.erb +++ b/app/views/grades/_show.html.erb @@ -26,7 +26,7 @@ <% @submit.pset.config['subgrades'].each do |subgrade_name, subgrade_type| %> <% if subgrade_type == 'integer' || subgrade_type == 'float' %>
- +
<% elsif subgrade_type == 'boolean' %> diff --git a/app/views/grading/show.html.erb b/app/views/grading/show.html.erb index 084b73d5..556fffc9 100644 --- a/app/views/grading/show.html.erb +++ b/app/views/grading/show.html.erb @@ -49,7 +49,7 @@
- <% if @files.present? || @submit.has_feedback? %> + <% if @files.present? || @submit.has_auto_feedback? %>
<% end if @files.present? %> - <% if @submit.has_feedback? %> - 
<%= @submit.check_feedback_formatted %>
+ <% if @submit.has_auto_feedback? %> + 
<%= @submit.formatted_auto_feedback %>
<% end %> <% end %> diff --git a/app/views/items/_submit.html.erb b/app/views/items/_submit.html.erb index d40a58e2..64e6bcff 100644 --- a/app/views/items/_submit.html.erb +++ b/app/views/items/_submit.html.erb @@ -14,7 +14,7 @@ URL: <%= link_to submit.url, submit.url %>

<% end %> -<% if submit.has_feedback? %> +<% if submit.has_auto_feedback? %> <%= link_to t("check_results"), profile_feedback_path(submit), class:"btn btn-secondary", data: { toggle:'modal', target:'#fb-modal', submit: submit.id } %> <% end %> diff --git a/app/views/submits/_grading_info.erb b/app/views/submits/_grading_info.erb index d9db451e..b85b97a5 100644 --- a/app/views/submits/_grading_info.erb +++ b/app/views/submits/_grading_info.erb @@ -1,5 +1,5 @@

<%= @submit.user_suspect_name %>/<%= @submit.pset_name %> <%= @grade.status %>
- <% if @submit.submitted_at %>Last submitted: <%= @submit.last_submitted %> (<%= "#{@submit.used_login}" if @submit.used_login %>)
<% end %> + <% if @submit.submitted_at %>Last submitted: <%= some_time_or_never(@submit.submitted_at) %> (<%= "#{@submit.used_login}" if @submit.used_login %>)
<% end %> Last graded: <%= @submit.last_graded %> (<%= @grade.grader_initials %>)

diff --git a/app/views/submits/_info.html.erb b/app/views/submits/_info.html.erb index bd10e175..753f9a41 100644 --- a/app/views/submits/_info.html.erb +++ b/app/views/submits/_info.html.erb @@ -1,3 +1,3 @@

- Last submitted: <%= @submit.last_submitted %> <%= "via login #{@submit.used_login}" if @submit.used_login %> + Last submitted: <%= some_time_or_never(@submit.submitted_at) %> <%= "via login #{@submit.used_login}" if @submit.used_login %>

From 80c04deca7ebc3f144abc7c459a27169cda4d46d Mon Sep 17 00:00:00 2001 From: Martijn Stegeman Date: Tue, 14 Jan 2020 21:37:29 +0100 Subject: [PATCH 07/41] cleanup --- app/models/grade.rb | 6 +----- app/models/group.rb | 17 ----------------- app/models/user.rb | 8 -------- .../schedules/_overview_table.html.erb | 1 - app/views/schedules/schedules/_table.html.erb | 1 - app/views/users/_show.html.erb | 1 - 6 files changed, 1 insertion(+), 33 deletions(-) diff --git a/app/models/grade.rb b/app/models/grade.rb index fc155e21..e0cb628a 100644 --- a/app/models/grade.rb +++ b/app/models/grade.rb @@ -12,7 +12,7 @@ class Grade < ApplicationRecord scope :showable, -> { where(status: [Grade.statuses[:published], Grade.statuses[:exported]]) } - before_save :set_calculated_grade, :unpublicize_if_undone # :update_grades_cache, + before_save :set_calculated_grade, :unpublicize_if_undone serialize :auto_grades @@ -170,8 +170,4 @@ def unpublicize_if_undone true end - def update_grades_cache - user.update_grades_cache - end - end diff --git a/app/models/group.rb b/app/models/group.rb index 5942399f..1dfcebc4 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -17,22 +17,5 @@ class Group < ApplicationRecord has_many :submits, -> { where(users: {active: true}) } , through: :users has_many :grades, -> { where(users: {active: true}) }, through: :submits - - # def self.import_user(user_id, group_name, user_name, user_mail) - # if login = Login.where(login: user_id).first - # if user = login.user - # user.update_columns(name: user_name, mail: user_mail) if user.name.blank? or user.name =~ /,/ - # if !group_name.blank? and group_name != "No group" - # group = Group.where(:name => group_name).first_or_create - # user.group = group - # user.save - # else - # user.group = nil - # user.save - # end - # end - # end - # end - end diff --git a/app/models/user.rb b/app/models/user.rb index a8ed4b45..7632eb1a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,7 +1,5 @@ class User < ApplicationRecord - serialize :grades_cache - # normal users belongs_to :group, optional: true delegate :name, to: :group, prefix: true, allow_nil: true @@ -133,12 +131,6 @@ def stagnated? end end - def update_grades_cache - grades = self.grades.select(:id, :submit_id, :pset_id, :grade, :calculated_grade) - grouped = grades.group_by(&:pset_id).transform_values { |v| v[0].serializable_hash } - update(grades_cache: grouped) - end - def update_last_submitted_at if last = submits.order("submitted_at").last update(last_submitted_at: last.submitted_at) diff --git a/app/views/schedules/schedules/_overview_table.html.erb b/app/views/schedules/schedules/_overview_table.html.erb index 360d575f..785f7cae 100644 --- a/app/views/schedules/schedules/_overview_table.html.erb +++ b/app/views/schedules/schedules/_overview_table.html.erb @@ -23,7 +23,6 @@ <% subs = user.submits.group_by(&:pset_id) - #subs = @student.grades_cache || [] %> <% @overview.each do |name, psets| %> diff --git a/app/views/schedules/schedules/_table.html.erb b/app/views/schedules/schedules/_table.html.erb index e2cc731d..ca369f8b 100644 --- a/app/views/schedules/schedules/_table.html.erb +++ b/app/views/schedules/schedules/_table.html.erb @@ -53,7 +53,6 @@ <% subs = user.submits.group_by(&:pset_id) - #subs = @student.grades_cache || [] %> <% @psets.each do |pset| %> diff --git a/app/views/users/_show.html.erb b/app/views/users/_show.html.erb index 617983a7..3bdd8f5a 100644 --- a/app/views/users/_show.html.erb +++ b/app/views/users/_show.html.erb @@ -1,7 +1,6 @@
<% subs = @student.submits.group_by(&:pset_id) - #subs = @student.grades_cache || [] %> <% @psets.each do |pset| %> From 4cc7fea1a031247787c2a539359f829f12c12ccf Mon Sep 17 00:00:00 2001 From: Martijn Stegeman Date: Tue, 14 Jan 2020 22:34:42 +0100 Subject: [PATCH 08/41] refactoring --- app/concerns/auto_check_receiver.rb | 4 +- ...eter.rb => auto_check_score_calculator.rb} | 16 +---- app/concerns/grade_calculator.rb | 42 +++++++++++ app/models/grade.rb | 69 ++++--------------- app/models/submit.rb | 2 +- 5 files changed, 61 insertions(+), 72 deletions(-) rename app/concerns/{auto_check_score_interpreter.rb => auto_check_score_calculator.rb} (76%) create mode 100644 app/concerns/grade_calculator.rb diff --git a/app/concerns/auto_check_receiver.rb b/app/concerns/auto_check_receiver.rb index ba77f173..e7c77718 100644 --- a/app/concerns/auto_check_receiver.rb +++ b/app/concerns/auto_check_receiver.rb @@ -11,7 +11,9 @@ def register_auto_check_results(json) grade = self.grade || self.build_grade # check via the grade if this submit is OK - grade.reset_automatic_grades(self.automatic_scores) + self.automatic_scores.each do |k,v| + grade.subgrades[k] = v + end grade.set_calculated_grade grade.status = Grade.statuses[:published] grade.save diff --git a/app/concerns/auto_check_score_interpreter.rb b/app/concerns/auto_check_score_calculator.rb similarity index 76% rename from app/concerns/auto_check_score_interpreter.rb rename to app/concerns/auto_check_score_calculator.rb index 9a842d60..41ab0f8d 100644 --- a/app/concerns/auto_check_score_interpreter.rb +++ b/app/concerns/auto_check_score_calculator.rb @@ -1,19 +1,16 @@ -module AutoCheckScoreInterpreter +module AutoCheckScoreCalculator extend ActiveSupport::Concern def automatic_scores - # puts "HIER #{self.inspect}" f = pset.config return {} if f.nil? || f['automatic'].nil? # take all automatic rules and use it to create hash of grades results = f['automatic'].transform_values do |rule| - # logger.debug rule begin self.instance_eval(rule) rescue - # puts "FAIL" nil end end @@ -21,13 +18,12 @@ def automatic_scores return results end + # method will be called from grading.yml expressions def correctness_score check_results = self.check_results return nil if !self.check_results.present? check_results.keys.each do |tool| - puts tool - puts check_results[tool].inspect case tool when "check50v2" return check_results[tool].count { |x| x["status"].present? } / check_results[tool].size.to_f @@ -36,21 +32,15 @@ def correctness_score return check_results[tool]["results"].count { |x| x["passed"].present? } / check_results[tool]["results"].size.to_f when "checkpy" if check_results[tool].is_a?(Array) - puts "arr" return check_results[tool].collect { |f| f["nPassed"] }.sum elsif check_results[tool].is_a?(Hash) - puts "hash" return [check_results[tool]].collect { |f| f["nPassed"] }.sum end end end - - # didn't document what kind of tool generates the below - # elsif self.check_feedback.is_a?(Array) && self.check_feedback[0].is_a?(Array) - # fb = self.check_feedback.flatten(1) - # fb.count { |x| x["status"].present? } / fb.size.to_f end + # method will be called from grading.yml expressions def style_score check_results = self.check_results check_results.keys.each do |tool| diff --git a/app/concerns/grade_calculator.rb b/app/concerns/grade_calculator.rb new file mode 100644 index 00000000..28b852a6 --- /dev/null +++ b/app/concerns/grade_calculator.rb @@ -0,0 +1,42 @@ +module GradeCalculator + + extend ActiveSupport::Concern + + included do + before_save :set_calculated_grade + end + + def set_calculated_grade + if subgrades_changed? + calculated_grade = calculate_grade(self) + if calculated_grade.present? + case self.pset.grade_type + when 'float' + # calculated_grade = calculated_grade + else # integer, pass + calculated_grade = calculated_grade.round + end + self.calculated_grade = calculated_grade * 10 + else + self.calculated_grade = nil + end + end + end + + def calculate_grade + calculations = Settings['grading']['grades'] if Settings['grading'] + return nil if calculations.nil? + + pset_name = self.pset.name + return nil if calculations[pset_name].nil? or calculations[pset_name]['calculation'].nil? + + begin + cg = self.subgrades.instance_eval(calculations[pset_name]['calculation']) + rescue + cg = nil + end + + return cg + end + +end diff --git a/app/models/grade.rb b/app/models/grade.rb index e0cb628a..97e8013a 100644 --- a/app/models/grade.rb +++ b/app/models/grade.rb @@ -1,8 +1,12 @@ class Grade < ApplicationRecord + + include GradeCalculator belongs_to :submit + has_one :user, through: :submit delegate :name, to: :user, prefix: true, allow_nil: true + has_one :pset, through: :submit delegate :name, to: :pset, prefix: true, allow_nil: true @@ -10,37 +14,25 @@ class Grade < ApplicationRecord delegate :name, to: :grader, prefix: true, allow_nil: true delegate :initials, to: :grader, prefix: true, allow_nil: true - scope :showable, -> { where(status: [Grade.statuses[:published], Grade.statuses[:exported]]) } - - before_save :set_calculated_grade, :unpublicize_if_undone - - serialize :auto_grades - # this is an OpenStruct to make sure that subgrades can be referenced as a method # for use in the grade calculation formulae in grading.yml serialize :subgrades, OpenStruct - enum status: [:unfinished, :finished, :published, :discussed, :exported] - - # scope :published, -> { where(status: Grade.statuses[:published]) } + scope :showable, -> { where(status: [Grade.statuses[:published], Grade.statuses[:exported]]) } + enum status: [:unfinished, :finished, :published, :discussed, :exported] + before_save :unpublicize_if_undone # this adds automatic grades to the subgrades quite aggressively after_initialize do if !self.persisted? # add any newly found autogrades to the subgrades as default - self.submit.automatic_scores.to_h.each do |k,v| + self.submit.automatic_scores.each do |k,v| self.subgrades[k] = v if not self.subgrades[k].present? end end end - def reset_automatic_grades(auto_grades) - auto_grades.to_h.each do |k,v| - self.subgrades[k] = v - end - end - def public? published? or discussed? or exported? end @@ -60,11 +52,6 @@ def first_graded created_at && created_at.to_formatted_s(:short) || "not yet" end - def auto_grades - # provides default value - super || {} - end - def subgrades=(val) # we would like this to be stored as an OpenStruct #return super if val.is_a? OpenStruct @@ -106,8 +93,8 @@ def calculated_grade end def any_final_grade - # this function prefers hard-coded grades but can also provide the calculated grade - self.grade or self.calculated_grade + # this function prefers hard-coded grades but can also provide the calculated grade + self.grade || self.calculated_grade end def grade=(new_grade) @@ -131,42 +118,10 @@ def grade=(new_grade) end end - def set_calculated_grade - if subgrades_changed? - calculated_grade = calculate_grade(self) - if !calculated_grade.blank? - # puts calculated_grade.inspect - case self.pset.grade_type - when 'float' - # calculated_grade = calculated_grade - else # integer, pass - calculated_grade = calculated_grade.round - end - self.calculated_grade = calculated_grade * 10 - else - self.calculated_grade = nil - end - end - end - - def calculate_grade(grade) - f = Settings['grading']['grades'] if Settings['grading'] - return nil if f.nil? - pset_name = grade.pset.name - return nil if f[pset_name].nil? or f[pset_name]['calculation'].nil? - begin - cg = grade.subgrades.instance_eval(f[pset_name]['calculation']) - puts "HIER #{grade.subgrades.inspect}" - rescue - cg = nil - end - return cg - end - private - + def unpublicize_if_undone - self.status = Grade.statuses[:unfinished] unless self.any_final_grade.present? + self.status = Grade.statuses[:unfinished] if self.any_final_grade.blank? true end diff --git a/app/models/submit.rb b/app/models/submit.rb index d40b8431..86cafc83 100644 --- a/app/models/submit.rb +++ b/app/models/submit.rb @@ -1,7 +1,7 @@ class Submit < ApplicationRecord include AutoCheckReceiver - include AutoCheckScoreInterpreter + include AutoCheckScoreCalculator include AutoCheckFeedbackFormatter belongs_to :user From 4d74f7d5007fdbe12c63e0be086a01f6d949a353 Mon Sep 17 00:00:00 2001 From: Martijn Stegeman Date: Wed, 15 Jan 2020 10:21:14 +0100 Subject: [PATCH 09/41] update user last submit time on each submit --- app/models/submit.rb | 2 ++ config/initializers/rufus_tasks.rb | 21 --------------------- 2 files changed, 2 insertions(+), 21 deletions(-) diff --git a/app/models/submit.rb b/app/models/submit.rb index 86cafc83..c5315be0 100644 --- a/app/models/submit.rb +++ b/app/models/submit.rb @@ -54,6 +54,8 @@ def record(used_login: nil, archive_folder_name: nil, url: nil, attachments: nil self.auto_graded = false self.save + + user.update(last_submitted_at: self.submitted_at) # update the submission for the parent module, if there is one if pset.parent_pset diff --git a/config/initializers/rufus_tasks.rb b/config/initializers/rufus_tasks.rb index 4bedfa77..61095c56 100644 --- a/config/initializers/rufus_tasks.rb +++ b/config/initializers/rufus_tasks.rb @@ -32,27 +32,6 @@ def safely end end end - - scheduler.cron '00 06 * * *' do - safely do - ActiveRecord::Base.transaction do - User.all.each &:update_last_submitted_at - end - end - end - - # scheduler.every '20m' do - # if Settings.automatic_grading_enabled - # Submit.where(check_feedback: nil).limit(20).each do |s| - # sleep 1 - # s.retrieve_check_feedback - # end - # Submit.where(check_feedback: nil).limit(20).each do |s| - # sleep 1 - # s.retrieve_style_feedback - # end - # end - # end scheduler.every '135m' do safely do From 83bdb4b6bd02e47f9904cf0e73b3dc0b1f7a5182 Mon Sep 17 00:00:00 2001 From: Martijn Stegeman Date: Wed, 15 Jan 2020 11:19:06 +0100 Subject: [PATCH 10/41] bugfix --- app/concerns/grade_calculator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/concerns/grade_calculator.rb b/app/concerns/grade_calculator.rb index 28b852a6..6d6b9558 100644 --- a/app/concerns/grade_calculator.rb +++ b/app/concerns/grade_calculator.rb @@ -8,7 +8,7 @@ module GradeCalculator def set_calculated_grade if subgrades_changed? - calculated_grade = calculate_grade(self) + calculated_grade = calculate_grade if calculated_grade.present? case self.pset.grade_type when 'float' From 4e7e533ffed265e5d4ee4f3e410194129b4859fd Mon Sep 17 00:00:00 2001 From: Martijn Stegeman Date: Wed, 15 Jan 2020 19:46:16 +0100 Subject: [PATCH 11/41] add grading details to modal grade viewer --- app/concerns/auto_check_feedback_formatter.rb | 33 ++++++++++++++++--- app/controllers/submits_controller.rb | 6 ++++ app/views/grading/_file_panels.html.erb | 8 +++++ app/views/grading/_file_tabs.html.erb | 10 ++++++ app/views/grading/show.html.erb | 22 ++----------- app/views/items/_submit.html.erb | 2 +- app/views/submits/_show.html.erb | 9 +++-- app/views/submits/_title.html.erb | 8 ++++- 8 files changed, 69 insertions(+), 29 deletions(-) create mode 100644 app/views/grading/_file_panels.html.erb create mode 100644 app/views/grading/_file_tabs.html.erb diff --git a/app/concerns/auto_check_feedback_formatter.rb b/app/concerns/auto_check_feedback_formatter.rb index 1a128ee5..4ee350f4 100644 --- a/app/concerns/auto_check_feedback_formatter.rb +++ b/app/concerns/auto_check_feedback_formatter.rb @@ -10,15 +10,12 @@ def has_auto_feedback? def formatted_auto_feedback check_results = self.check_results - result = "" items = nil v3=nil # each tool has different output formats that we detect here check_results.keys.each do |tool| - # puts tool - # puts check_results[tool].is_a?(Array) case tool when "check50v2" v3=false @@ -33,9 +30,11 @@ def formatted_auto_feedback return check_results[tool]["error"]["value"] if items.nil? when "checkpy" if check_results[tool].is_a?(Array) - # puts "ARR" v3=true - items = check_results[tool].collect {|f| f["results"]}.compact.flatten + # items = check_results[tool].collect {|f| f["results"]}.compact.flatten + return check_results[tool].collect do |item| + format_checkpy_feedback(item) + end.join elsif check_results[tool].is_a?(Hash) v3=true items = [check_results[tool]].collect {|f| f["results"]}.flatten @@ -48,6 +47,7 @@ def formatted_auto_feedback return "" if items.nil? # now generate basic feedback + result = "" items.each do |item| # puts item @@ -69,4 +69,27 @@ def formatted_auto_feedback return result end + def format_checkpy_feedback(part) + "- #{part['name']}\n" + + part['results'].collect { |item| format_line(item["passed"], item['description'], item['message']) }.join + end + + def format_line(success, description, explanation) + result = '' + case success + when true + result << " :)" + when false + # puts "FALSE" + result << " :(" + when nil + result << " :|" + end + result << " #{description}\n" + if explanation.present? + result << " #{explanation}\n" + end + result + end + end diff --git a/app/controllers/submits_controller.rb b/app/controllers/submits_controller.rb index 298b7a64..281ac4a2 100644 --- a/app/controllers/submits_controller.rb +++ b/app/controllers/submits_controller.rb @@ -59,6 +59,12 @@ def load_submit(id) # load the submit and any grade that might be attached @submit = Submit.includes(:grade, :user, :pset).find(id) @grade = @submit.grade || @submit.build_grade({ grader: current_user }) + + # load files submitted in child psets if we want to grade a parent module + @files_from_module = @submit.user.files_for_module(@submit.pset.mod) if @submit.pset.mod + + # either use the files attached to this specific submit, or all gathered from the module + @files = @submit.file_contents || @files_from_module end end diff --git a/app/views/grading/_file_panels.html.erb b/app/views/grading/_file_panels.html.erb new file mode 100644 index 00000000..121bec7e --- /dev/null +++ b/app/views/grading/_file_panels.html.erb @@ -0,0 +1,8 @@ +<% @files.each do |filename, contents| %> +
+ <%= link_to 'd', grading_download_path(grading_submit_id: @submit.id, filename: filename), remote: false, class: 'pull-right' %><%= CodeRay.scan(contents, CodeRay::FileType.fetch(filename, :text)).div(:line_numbers => :inline).html_safe %> +
+<% end if @files.present? %> +<% if @submit.has_auto_feedback? %> +
<%= @submit.formatted_auto_feedback %>
+<% end %> diff --git a/app/views/grading/_file_tabs.html.erb b/app/views/grading/_file_tabs.html.erb new file mode 100644 index 00000000..808803fe --- /dev/null +++ b/app/views/grading/_file_tabs.html.erb @@ -0,0 +1,10 @@ +<% @files.each_key do |filename| %> +
  • + <%= filename %> +
    • +<% end if @files.present? %> +<% if @submit.has_auto_feedback? %> +
  • + check +
    • +<% end %> diff --git a/app/views/grading/show.html.erb b/app/views/grading/show.html.erb index 556fffc9..4990ce0f 100644 --- a/app/views/grading/show.html.erb +++ b/app/views/grading/show.html.erb @@ -51,28 +51,10 @@ <% if @files.present? || @submit.has_auto_feedback? %>
    - <% first = true %> - <% @files.each do |filename, contents| %> -
    - <%= link_to 'd', grading_download_path(grading_submit_id: @submit.id, filename: filename), class: 'pull-right' %><%= CodeRay.scan(contents, CodeRay::FileType.fetch(filename, :text)).div(:line_numbers => :inline).html_safe %> -
    - <% end if @files.present? %> - <% if @submit.has_auto_feedback? %> - 
    <%= @submit.formatted_auto_feedback %>
    - <% end %> + <%= render partial: 'file_panels', locals: { first: true } %>
    <% end %> diff --git a/app/views/items/_submit.html.erb b/app/views/items/_submit.html.erb index 64e6bcff..cfa084ba 100644 --- a/app/views/items/_submit.html.erb +++ b/app/views/items/_submit.html.erb @@ -19,7 +19,7 @@ URL: <%= link_to submit.url, submit.url %> <% end %>