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

feat: add handling for peer submission timeout / cancel #2157

Closed
wants to merge 3 commits into from

Conversation

nsprenkle
Copy link
Contributor

@nsprenkle nsprenkle commented Jan 5, 2024

TL;DR - Send the ERR_SUBMISSION_NO_LONGER_AVAILABLE error code and a 404 error when trying to submit a grade in the following circumstances:

  1. Lease on a peer assessment has expired.
  2. Submission that a student was grading has been cancelled.

JIRA: AU-1647

Developer Checklist

Testing Instructions

[ How should a reviewer test this PR? ]

Reviewer Checklist

Collectively, these should be completed by reviewers of this PR:

  • I've done a visual code review
  • I've tested the new functionality

FYI: @openedx/content-aurora

@nsprenkle nsprenkle requested a review from jansenk January 5, 2024 21:02
Copy link

codecov bot commented Jan 5, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (dae4b35) 94.92% compared to head (cdb98e6) 94.92%.

Files Patch % Lines
openassessment/xblock/ui_mixins/mfe/mixin.py 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2157      +/-   ##
==========================================
- Coverage   94.92%   94.92%   -0.01%     
==========================================
  Files         191      191              
  Lines       20571    20574       +3     
  Branches     1865     1866       +1     
==========================================
+ Hits        19528    19529       +1     
- Misses        779      780       +1     
- Partials      264      265       +1     
Flag Coverage Δ
unittests 94.92% <60.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nsprenkle nsprenkle marked this pull request as ready for review January 5, 2024 21:12
@nsprenkle nsprenkle requested a review from a team as a code owner January 5, 2024 21:12
@jansenk
Copy link
Contributor

jansenk commented Jan 8, 2024

This actually won't fix the issue, and I think the issue is different than what we think it will be anyways:

Old code:

  • Pass the current submission uuid along with the assessment info
  • Backend calls get_peer_whatever which discards "old" PeerWorkflowItem and grabs a new one
  • passed old uuid doesn't match new uuid and assess fails

We don't pass a submission uuid back to the mfe view. What I think will happen now is:

  • Call peer_assess with assessment for old submission
  • Old peer workflow item is discarded and we grab a new one
  • apply assessment for old submission to new submission

No bueno!! We should probably make a new ticket to address this

@nsprenkle
Copy link
Contributor Author

Okay, will close this PR, awaiting a different fix.

This actually won't fix the issue, and I think the issue is different than what we think it will be anyways:

Old code:

  • Pass the current submission uuid along with the assessment info
  • Backend calls get_peer_whatever which discards "old" PeerWorkflowItem and grabs a new one
  • passed old uuid doesn't match new uuid and assess fails

We don't pass a submission uuid back to the mfe view. What I think will happen now is:

  • Call peer_assess with assessment for old submission
  • Old peer workflow item is discarded and we grab a new one
  • apply assessment for old submission to new submission

No bueno!! We should probably make a new ticket to address this

@nsprenkle nsprenkle closed this Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants