Skip to content
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

Add duplicate submission checks #1048

Merged
merged 1 commit into from
Aug 9, 2022

Conversation

ihalaij1
Copy link
Contributor

@ihalaij1 ihalaij1 commented Jun 21, 2022

Description

What?

When submitting a duplicate submission a modal dialog now asks for confirmation from the user whether or not they want to submit the identical submission.

Why?

Students sometimes accidentally submit identical code / questionnaire answers.

How?

An md5 hash based on the submission data is created in the frontend JavaScript (chapter.js) and appended to the submitted formData.
In the case of a file submit exercise, the hash is created from the files concatenated into a string.
In the case of a questionnaire, a JSON string from the questionnaire fields and answers is created, and this string is hashed.

The hash is rendered in the exercise HTML the next time, and a check is performed to see if the subsequent submissions have an identical hash. A modal dialog is shown if the same hash already exists in the HTML.

Fixes #1006

This PR is based on the draft #1040

Testing

What type of test did you run?

  • Accessibility test using the WAVE extension.
  • Django unit tests.
  • Selenium tests.
  • Other test. (Add a description below)
  • Manual testing.

Tested that duplicate submissions are detected correctly and that the modal dialog works.

Did you test the changes in

  • Chrome
  • Firefox
  • This pull request cannot be tested in the browser.

Translation

Programming style

  • Did you follow our style guides?
  • Did you use Python type hinting in all functions that you added or edited? (type hints for function parameters and return values)

Have you updated the README or other relevant documentation?

  • documents inside the doc directory.
  • README.md.
  • Aplus Manual.
  • Other documentation (mention below which documentation).

Is it Done?

  • Reviewer has finished the code review
  • After the review, the developer has made changes accordingly
  • Customer/Teacher has accepted the implementation of the feature

@ihalaij1
Copy link
Contributor Author

I couldn't figure out an easy way to add information to the modal dialog about which submissions are the detected duplicates.
Do you have any ideas or should it be left like this?

Copy link
Contributor

@markkuriekkinen markkuriekkinen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is a good start!

  • I think the JavaScript code should be reorganized and cleaned up a lot.
  • The full-view exercise pages don't seem to have any duplicate checks yet (only chapter pages have it now).
  • git commit history: is Muhammad's code used any more? His commits could be removed if his code isn't used in the final version. At any rate, the commit message may acknowledge that the work started from his draft pull request.
    If Muhammad's code is still used, then we can squash his commits into one. Since a git commit has only one author, we can avoid squashing commits from different authors into a combined commit.

@markkuriekkinen
Copy link
Contributor

I couldn't figure out an easy way to add information to the modal dialog about which submissions are the detected duplicates. Do you have any ideas or should it be left like this?

If the hashes are stored in a list/array instead of a set and the indexes match the order of the submissions, then you would later know which submission is the duplicate one. It would be best to show the duplicate submission number to the user so that they can more easily check the submission contents.

@ihalaij1 ihalaij1 force-pushed the dupe-submissions branch 2 times, most recently from 0ee775e to 5299d9a Compare August 2, 2022 03:59
@ihalaij1
Copy link
Contributor Author

ihalaij1 commented Aug 2, 2022

I made improvements based on your suggestions and replied to some of the comments.

Copy link
Contributor

@markkuriekkinen markkuriekkinen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, it's getting better!

Duplicate submission check on the full-view exercise page and using chapter.js

One problem is that the full-view page has slightly different HTML structure than the chapter pages. There are some top-level div elements that do not exist in both views (such as <div id="exercise-all">). The attributes data-aplus-quiz and data-aplus-active-element might not exist in the full view, but I didn't check that now. The navigation selectors in chapter.js might not work in the full view. In the full view, we don't want to use the AJAX form submission that is used in chapter.js. Submitting the form is supposed to redirect to the separate submission feedback page.

We need to be careful in case using chapter.js in the old full-view exercise pages introduces some new surprises or incompatibilities (if chapter.js worked at all there).

I have a feeling chapter.js is not going to work in the full-view exercise page without major changes. You may check this more carefully.

If setting up duplicate checks in the full-view exercise page becomes too hard or too laborious, then we ignore that in this pull request. Don't spend a ton of time with it!

Instead of using chapter.js in the full-view exercise page, writing a new similar JS script could be much easier. That's why most of the work is done in reusable functions that can be called anywhere.

Adding the duplicate submission number to the modal dialog for improved usability

The duplicate submission is identified only when the user tries to submit again. Thus, the submission number can not be known when the html templates are rendered in the Django backend. However, you can modify the html in the browser JavaScript and add the submission number there.

The translated string DUPLICATE_SUBMISSION could include some html inside the string (not optimal, but easy to do; the same html is repeated in both the English and Finnish strings). E.g. <span data-dup-submission=""></span>.

This could be called in duplicate_check.js - function modalOrSubmit.

// Set the number of the existing duplicate submission.
$("#duplicate-submission-modal").find("[data-dup-submission]").text("123");

Setting up external libraries in the A-plus git repo - blueimp-md5

blueimp-md5 JS should not be copied to a-plus/assets_src as a whole directory. Use package.json and install.sh that are compatible with our drone exec setup (build pipeline).
I wrote more details in a comment in the file assets_src/blueimp-md5/README.md.

@ihalaij1
Copy link
Contributor Author

ihalaij1 commented Aug 9, 2022

The attributes data-aplus-quiz and data-aplus-active-element do not exist in the full-view exercise pages.

Adding an event listener to the form submit button on full-view exercise pages might work (since active element exercises don't have those?) And I can differentiate between a quiz and a submit exercise by checking the amount of files included in the submission.

Active element exercises also have an iframe with class="acos-iframe", which could be used to identify them.

But then should I create a function that submits the submission and redirects to the separate submission feedback page, and use that as the submitCallback function in full-view exercise pages?

@markkuriekkinen
Copy link
Contributor

The attributes data-aplus-quiz and data-aplus-active-element do not exist in the full-view exercise pages.

Adding an event listener to the form submit button on full-view exercise pages might work (since active element exercises don't have those?) And I can differentiate between a quiz and a submit exercise by checking the amount of files included in the submission.

Active element exercises also have an iframe with class="acos-iframe", which could be used to identify them.

But then should I create a function that submits the submission and redirects to the separate submission feedback page, and use that as the submitCallback function in full-view exercise pages?

I'm not sure what you are thinking of when you say "active elements". A+ active elements have never worked on the full-view exercise pages because they are very dependent on the chapter.js code.

Active elements are also not related to Acos server exercises.
The iframe with class="acos-iframe" comes from Acos server.

https://plus.cs.aalto.fi/aplus-manual/master/active_elements/introduction/

Questionnaire exercises could also include file upload fields. They are very rarely used in practice, but for example, the teaching assistant pool questionnaire uses file uploads.

Nonetheless, we shall just ignore the full-view exercises now! We can return to them at some point if there is a real demand for it.

Thanks for taking a deeper look at this! 👍

@ihalaij1
Copy link
Contributor Author

ihalaij1 commented Aug 9, 2022

Oh yeah, I mixed up active elements and Acos exercises, probably because they are both interactive and start with the letter A.
Questionnaires with file uploads don't cause issues with the current duplicate check code.

I think the commit messages can be squashed into one, since Muhammad's code is pretty much all gone.
Do you agree?

Copy link
Contributor

@markkuriekkinen markkuriekkinen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful, this is finished after you fix a couple of tiny issues!

@markkuriekkinen
Copy link
Contributor

Oh yeah, I mixed up active elements and Acos exercises, probably because they are both interactive and start with the letter A. Questionnaires with file uploads don't cause issues with the current duplicate check code.

I think the commit messages can be squashed into one, since Muhammad's code is pretty much all gone. Do you agree?

Yes, I agree on squashing the commits. If Muhammad's code has been replaced, then we don't need those original commits in the history. However, you should write in the final commit message that this work started from Muhammad's pull request. You can link his old pull request too.

@ihalaij1
Copy link
Contributor Author

ihalaij1 commented Aug 9, 2022

This should be ready for merging!

Copy link
Contributor

@markkuriekkinen markkuriekkinen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!

@markkuriekkinen markkuriekkinen merged commit 65a2915 into apluslms:master Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Detect and ignore duplicate submissions
2 participants