Feature: Option to retain old grading data

[pranavrao145]

Proposed Changes

Closes #5212.

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)	X
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

• I have performed a self-review of my changes.
  • Check that all changed files included in this pull request are intentional changes.
  • Check that all changes are relevant to the purpose of this pull request, as described above.
• I have added tests for my changes, if applicable.
  • This is required for all bug fixes and new features.
• I have updated the project documentation, if applicable.
  • Associated PR: Add info about option to retain existing grading data on recollected submissions Wiki#221

After opening your pull request:

• I have updated the project Changelog (this is required for all changes).
• I have verified that the pre-commit.ci checks have passed.
• I have verified that the CI tests have passed.
• I have reviewed the test coverage changes reported by Coveralls.
• I have requested a review from a project maintainer.

Questions and Comments

Within inline comments.

[pranavrao145]

I wonder whether there is a better way to do this (rather than by filename and path).

[coveralls]

Pull Request Test Coverage Report for Build 11606631947

Details

• 173 of 173 (100.0%) changed or added relevant lines in 11 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.05%) to 91.608%

---

Totals
Change from base Build 11545120245:	-0.05%
Covered Lines:	40847
Relevant Lines:	43904

---

💛 - Coveralls

[pranavrao145]

I moved this out of the onClick logic and into the onSubmit logic for the form so I could test this properly, otherwise there were errors about a lack of implementation for onSubmit

[david-yz-liu]

Hi @pranavrao145, great work! I just did a quick review of the back-end implementation and left some comments. Please work on those, and then I'll do another review in a few days.

[david-yz-liu]

Overall, please make this a Submission instance method. You can just raise an exception if there's an error, and then in the perform method here, catch the exception to display the error message in the flash. You can call it copy_grading_data.

Make the argument to pass in old_submission, not the grouping. Compute the old_submission in the perform method, and use the grouping.current_submission_used method to grab this, rather than checking the created_at timestamps.

[pranavrao145]

Hi @david-yz-liu, thanks for the feedback! Code is looking a lot cleaner now. One question, seems the flash message on error is already taken care of implicitly when the Submissions model raises an exception, and it displays a clean message in red. However, this stops progress on all submissions if any one of them has a problem.

Would you like me to explicitly rescue Submission class errors here and add it only as a warning-level alert (using add_warning_messages), or just leave it at the error-level alert with a stop in processing other submissions? Content-wise, messages on both are pretty much the same (neither give the group info), so I thought I'd verify what the intended behaviour is.

[pranavrao145]

I'm just going to convert this back to a draft while I work on addressing comments just so it doesn't run tests on intermediate work, and I'll make it ready for review again once I'm finished!

EDIT: back to ready now, but Jest tests still wrongly failing

[david-yz-liu]

@pranavrao145 great work. I just left a few minor comments.

A wrinkle: after some discussion, I now understand that a common use case for this feature is that when an instructor receives a remark request, that's what triggers a new submission to be collected (i.e., the submission files need to be changed). Given this, I do think it's best to copy over both the original result and new remark result after all. Can you please modify the code to make this work (you might want to refactor again to create a Result instance method). Sorry about this!

[david-yz-liu]

Keep the checkbox label the same for both checked and unchecked states. Instead, if the option is unchecked, show a warning div (same styling as warning flash messages) with the extra warning message. Since the div will have good styling already, you don't need to bold the warning message.

[david-yz-liu]

Ah yes, can you actually move these mocks into one of the Jest setup files (and remove them from existing places in the codebase)?

[david-yz-liu]

Similar to below, here I suspect you can do a params[:retain_existing_grading] == 'true'. You can modify the apply_late_penalty setting above as well.