-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes to assignment upload to fix exisitng bugs and add new features (Part 1) #82
base: master
Are you sure you want to change the base?
Conversation
Gemfile
Outdated
gem 'timeout' | ||
|
||
# to access rails variables in javascript | ||
gem 'gon' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add nl?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have completed the change.
@@ -1,4 +1,4 @@ | |||
class ApplicationMailer < ActionMailer::Base | |||
default from: 'from@example.com' | |||
default from: '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a default configurable address? Is this line necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems related to L4-5 in app/mailers/assignment_mailer.rb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will address this in the next PR which will include progress bar, assignment timeout and email functionalites.
@@ -51,10 +52,19 @@ class Assignment < ActiveRecord::Base | |||
ocaml: "ocaml" | |||
} | |||
|
|||
FILE_STRUCTURES = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment as to what this FILE_STRUCTURES enum is doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some comments.
|
||
</div> | ||
<div id="file_to_ignore_details" style="display:none;"> | ||
<%= f.label :filesIgnore, "Files to be ignored \u24D8" , title: "If you want to ignore certain files from the code zip file that you uploaded, add those filenmes under here. \n\nIf you want to ignore filenames that exactly match a certain filename, enter the filename as it is. For instance, entering \"question1\", will mean that all the filenames that are named as \"question1\" (irrespective of their file extensions) will all be ignored. \n\nIf you want to ignore filenames that partially contain a substring, add \"/ \" in front of the filename. For instance, if you enter, \"/question1\", filenames such as \"alice_question1.py\", \"bob_question1Test.c\", \"question1_charlie.java\" will all get ignored. \n\nIf you need to add muliple regex, leave a space in between them. For instance, if you enter \"question1 /question2 \", filesnames that either exactly match \"question1 \" or contain the substring \"question2 \" will all get ignored" %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any suggested defaults? .pdf .doc(x)? .bin .ps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to ignore code files. For non code files like pdf and doc files, these files will naturally get ignored without any users' input.
<%= f.label :filesIgnore, "Files to be ignored \u24D8" , title: "If you want to ignore certain files from the code zip file that you uploaded, add those filenmes under here. \n\nIf you want to ignore filenames that exactly match a certain filename, enter the filename as it is. For instance, entering \"question1\", will mean that all the filenames that are named as \"question1\" (irrespective of their file extensions) will all be ignored. \n\nIf you want to ignore filenames that partially contain a substring, add \"/ \" in front of the filename. For instance, if you enter, \"/question1\", filenames such as \"alice_question1.py\", \"bob_question1Test.c\", \"question1_charlie.java\" will all get ignored. \n\nIf you need to add muliple regex, leave a space in between them. For instance, if you enter \"question1 /question2 \", filesnames that either exactly match \"question1 \" or contain the substring \"question2 \" will all get ignored" %> | ||
<%= f.text_field :filesIgnore %> | ||
</div> | ||
<div id="file_to_process_details" style="display:none;"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May need to be clear which of ignore
or to process
has precedence. Should be ordered as such too (overriding one listed first).
<%= f.label :filesProcess, "Files to be only processed \u24D8" , title: "If you want to process and compare only certain files from the code zip file that you uploaded, add those filenmes under here. \n\nIf you want to process filenames that exactly match a certain filename, enter the filename as it is. For instance, entering \"question1\", will mean that all the filenames that are named as \"question1\" (irrespective of their file extensions) will all be processed and the rest will be ignored. \n\nIf you want to only process filenames that partially contain a substring, add \"/ \" in front of the filename. For instance, if you enter, \"/question1\", filenames such as \"alice_question1.py\", \"bob_question1Test.c\", \"question1_charlie.java\" will all get processed and the rest that does not contain the substring will all get ignored. \n\nIf you need to add muliple regex, leave a space in between them. For instance, if you enter \"question1 /question2 \", filesnames that either exactly match \"question1 \" or contain the substring \"question2 \" will all get processed." %> | ||
<%= f.text_field :filesProcess %> | ||
</div> | ||
<div id="ref_dir_details" style="display:none;"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will need to point to documentation about how this works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will create an issue to update user guide so that I can add documentation for all newly added changes.
</div> | ||
<div id="email_details" style="display:none;"> | ||
<%= f.label :email, "Send email upon completion \u24D8" , title: "If you want to receive an email to notify you after the assignment has been processed, enter the email here. Ensure that you enter a valid email." %> | ||
<%= f.text_field :email %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fill in with user's default email, to be edited?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I have changed it accordingly.
<head> | ||
<meta charset="utf-8"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1"> | ||
<title>jQuery UI Dialog - Modal message</title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix title, even if not shown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed the title accordingly.
|
||
<pre> | ||
Language: <%= @assignment.language%> | ||
Min Match Length: <%= @assignment.min_match_length%> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested defaults and \u24D8 for this line and next?
@@ -26,9 +26,9 @@ | |||
</div> | |||
<div class="mark_as_suspicious"> | |||
<% if @submission_similarity.status == SubmissionSimilarity::STATUS_NOT_PLAGIARISM %> | |||
<p><%= link_to "Mark these similarities as suspicious", assignment_submission_similarity_suspect_as_plagiarism_url(@assignment, @submission_similarity), :method => :put, data: {confirm: "Mark these similarities as suspicious?"} %> or <%= link_to "Confirm these similarities as plagiarism", assignment_submission_similarity_confirm_as_plagiarism_url(@assignment, @submission_similarity), :method => :put, data: {confirm: "Confirm these similarities as plagiarism?"} %></p> | |||
<%= button_to "Confirm these similarities as plagiarism", assignment_submission_similarity_confirm_as_plagiarism_url(@assignment, @submission_similarity), :method => :put, data: {confirm: "Confirm these similarities as plagiarism?"} %> <%= link_to "or mark these similarities as suspicious...", assignment_submission_similarity_suspect_as_plagiarism_url(@assignment, @submission_similarity), :method => :put, data: {confirm: "Mark these similarities as suspicious?"} %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important: These should show up as buttons and not links because they are processes/actions and not views.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR#92 (#92) has fixed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Riyas97 please take a look at the comments. When you are done you can merge. We can fix more later if needed.
<% elsif @submission_similarity.status == SubmissionSimilarity::STATUS_SUSPECTED_AS_PLAGIARISM %> | ||
<p>These similarities were marked as suspicious. <%= link_to "Remove suspicion", assignment_submission_similarity_unmark_as_plagiarism_url(@assignment, @submission_similarity), :method => :put, data: {confirm: "Remove suspicion of plagiarism?"} %> or <%= link_to "Confirm as plagiarism", assignment_submission_similarity_confirm_as_plagiarism_url(@assignment, @submission_similarity), :method => :put, data: {confirm: "Confirm these similarities as plagiarism?"} %></p> | ||
<p>These similarities were marked as suspicious. <%= button_to "Remove suspicion", assignment_submission_similarity_unmark_as_plagiarism_url(@assignment, @submission_similarity), :method => :put, data: {confirm: "Remove suspicion of plagiarism?"} %> <%= button_to "Confirm as plagiarism", assignment_submission_similarity_confirm_as_plagiarism_url(@assignment, @submission_similarity), :method => :put, data: {confirm: "Confirm these similarities as plagiarism?"} %></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important: These should show up as buttons and not links because they are processes/actions and not views.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR#92 (#92) has fixed it.
<% elsif @submission_similarity.status == SubmissionSimilarity::STATUS_SUSPECTED_AS_PLAGIARISM %> | ||
<p>These similarities were marked as suspicious. <%= link_to "Remove suspicion", assignment_submission_similarity_unmark_as_plagiarism_url(@assignment, @submission_similarity), :method => :put, data: {confirm: "Remove suspicion of plagiarism?"} %> or <%= link_to "Confirm as plagiarism", assignment_submission_similarity_confirm_as_plagiarism_url(@assignment, @submission_similarity), :method => :put, data: {confirm: "Confirm these similarities as plagiarism?"} %></p> | ||
<p>These similarities were marked as suspicious. <%= button_to "Remove suspicion", assignment_submission_similarity_unmark_as_plagiarism_url(@assignment, @submission_similarity), :method => :put, data: {confirm: "Remove suspicion of plagiarism?"} %> <%= button_to "Confirm as plagiarism", assignment_submission_similarity_confirm_as_plagiarism_url(@assignment, @submission_similarity), :method => :put, data: {confirm: "Confirm these similarities as plagiarism?"} %></p> | ||
<% elsif %> | ||
<p>These similarities were confirmed as plagiarism. <%= link_to "Remove", assignment_submission_similarity_unmark_as_plagiarism_url(@assignment, @submission_similarity), :method => :put, data: {confirm: "Remove confirmation as plagiarism?"} %></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important: These should show up as buttons and not links because they are processes/actions and not views.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR#92 (#92) has fixed it.
@@ -0,0 +1,5 @@ | |||
class AddEmailToAssignments < ActiveRecord::Migration[6.0] | |||
def change | |||
add_column :assignments, :email, :string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need model validation or leave as simple string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, we can leave it as simple string.
@@ -152,11 +177,12 @@ | |||
create_table "users", options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb3", force: :cascade do |t| | |||
t.string "name" | |||
t.string "full_name" | |||
t.string "password_digest", default: "$2a$12$6ONW8hZL3q2oOABLut6NOO4cDU9uULpu0If1OVVsfTgaURpk5SCAW" | |||
t.string "password_digest", default: "$2a$12$Ifr1zMyr9ZCydGsN0TGlru/fYL7CA/BXkcIO6W/QPQFzOZdA0.XDO" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the password digest
be here in the VCS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to remove it but it seems this is a default. Anyways, I don't think this will pose any security threat.
@@ -0,0 +1,7 @@ | |||
require 'test_helper' | |||
|
|||
class AssignmentMailerTest < ActionMailer::TestCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the assignment mailer is new, do you need test cases to ensure that the mailer works? Or create separate issue now to handle it this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will create an separate issue as there will be another PR on this as well.
</div> | ||
<div> | ||
<%= f.submit class: "submit" %> | ||
<%= f.submit 'Create Assignment', name: 'create' %> | ||
<%= f.submit 'Preview Uploaded Code Files', name: 'preview' %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear what is the difference. Add a doc link or \u24D8 to describe what is the difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added the necessary changes.
# submitted code files or dir size (depending on which code file structure was selected) | ||
@codeFilesOrDirSize = 0 | ||
# dir hash to display contents inside dir (applicable if code file structure is 'By Dir') | ||
@dirHash = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any of these vars potentially too large to handle?
# file path of extracted ref file | ||
refFilePath = "" | ||
@dirSize = 0 | ||
@dirHash = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any of these vars potentially too large to handle?
refFile_path = "" | ||
|
||
if (!isValidCodeFile) | ||
@assignment.errors.add :file, "containing student submission files must be a valid zip file" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalize all of these messages "containing..."
|
||
def string_from_combined_files(path) | ||
strings = [] | ||
if File.directory? path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any problem if v v big directory or listing comes in? Will it crash?
When you close make sure to mention all of the issues which this PR (temporarily) closes. 👍 |
Description
This PR aims to fix some of the issues related to assignment upload and also add a few new functionalities based on the existing issues in GitHub. The issues that this PR aims to fix and the new functionalities added are as below:
This feature allows users to preview which files will be processed by the application. As described above, users can select the file structure, enter the files to be processed or ignored. And then, to view and double check whether the right files will get processed, users can click "Preview Uploaded Code Files" to view those filenames.
Preview files if "By Files" option selected.
Some examples include the user selecting the "By Directory" option but uploading a zip file that does not have have student folders, empty zip files, zip files that only contain 1 student code file (hence cannot run plagiarism algorithm) etc. For those case, when the user wants to upload a zip file, he/she will be denied and a dialog will appear to show the issue.
Empty zip file
Do note that there are other features such as email which are reflected in the UI but the code for those will be separately done as another PR (as this PR is already quite huge).