-
Notifications
You must be signed in to change notification settings - Fork 226
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
Update the error message for failed partial feedback & refactor getPartialFeedback logic #1671
Conversation
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.
Tested and left some comments
function refreshPartial(clicked) { | ||
const lastPartialFeedback = localStorage.getItem(<%= @job_id %>); |
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.
job_id does not uniquely identify a job - this counter resets whenever Tango server resets.
You probably need the submission number (and maybe the user).
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.
At the same time, I believe it isn't as appropriate to use localStorage for storing the feedback as the they do not expire (and won't be deleted automatically) So a students localstorage will get bloated after many submissions to autolab.
Either somehow set an expiry, or explore sessionStorage
https://www.digitalocean.com/community/tutorials/js-introduction-localstorage-sessionstorage
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.
job_id only uniquely identifies on the Tango side - basically there will not be two jobs running that has the same job_id. So you won't get a situation where you get someone else's partial feedback
…age key to include submission info
…into new-error-message
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.
Tested - LGTM!
Left few comments, but PR functional
submission: <%= @submission_id %>, | ||
job: <%= @job_id %>, | ||
user: <%= @cud&.user_id %>, | ||
} |
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 seems like a good key - issue resolved while testing
function refreshPartial(clicked) { | ||
const lastPartialFeedback = sessionStorage.getItem(sessionKey); |
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.
SessionStorage use here resolved the issue of persistence previously.
$('.feedback-status').html("In-Progress"); | ||
$('.feedback-status').removeClass("feedback-status__queued"); | ||
$('.feedback-status').addClass("feedback-status__inprogress"); | ||
$( "#result-body" ).load(window.location.href + " #result-body" ); |
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 I ask what's going on in this line of code? I don't quite get what this line does.
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.
Updates the badge and the loading message in the right-hand panel to the in-progress states when going from queued to in-progress
Description
tango_get_partial_feedback
to prevent possible error that occurs when attempting to access partial feedback for job that is being taken off the queue.How Has This Been Tested?
Types of changes
Checklist:
overcommit --install && overcommit --sign
to use pre-commit hook for linting