-
Notifications
You must be signed in to change notification settings - Fork 18
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
fix: update the misleading proctoring verified message #63
fix: update the misleading proctoring verified message #63
Conversation
Thanks for the pull request, @arslanashraf7! I've created OSPR-6526 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
@arslanashraf7 Thank you for your contribution. Let's see how tests turn out. |
Some feedback on the language here. I think The case brought up in the ticket makes sense, would we be able to simply modify the second statement regarding grades to be a bit more generic to fit your use case? |
@zacharis278 Thanks for taking a look at this.
This also makes sense.
For the above, let me take input from @pdpinch |
@@ -6,8 +6,7 @@ const VerifiedProctoredExamInstructions = () => ( | |||
<h3 className="h3" data-testid="proctored-exam-instructions-title"> | |||
<FormattedMessage | |||
id="exam.VerifiedProctoredExamInstructions.title" | |||
defaultMessage={'Your proctoring session was reviewed successfully. ' | |||
+ 'Go to your progress page to view your exam grade.'} | |||
defaultMessage={'Exams are being reviewed and a final grade will be available soon.'} |
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.
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 like this. I realize it's more vague than the 'reviewed' version, but it is still accurate, and and won't lead learners to think they have definitely passed review when we still may fail them for cheating.
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.
The PR has been updated with the suggested text and the screenshots in the description have also been updated.
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'm a bit unclear on the change from reviewed to submitted. There already exists a view that will confirm successful submission prior to a review being completed (SubmittedProctoredExamInstructions). This view however is only shown after the review has been verified is that not what's happening?
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.
@zacharis278 we want to try to make a distinction between reviewed by the proctoring provider and reviewed by the course team.
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.
is there any state in the platform we can inspect to know if these attempts require the additional course team review step? Maybe a new interstitial page to account for that state? My overall concern here is we rely on these messages to communicate if the exam is awaiting review (submitted), or has been rejected/verified. This change would make attempts appear to be 'submitted' indefinitely.
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.
@zacharis278 There is not really an interstitial state. The course team does a lot of review and grade calculation after the proctoring review by the proctoring vendor. I see your point that you don’t want the reviews to appear to be in a submitted state indefinitely. If you feel that the first statement should remain ‘Your proctoring session was reviewed successfully.’ then we are okay with that as long as the second sentence is ‘A final grade will be available soon.’
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.
@sfrucht ‘A final grade will be available soon.’ would be good, we'd just like to avoid losing the first statement for all exams. Let me run this past the team but I don't expect any issues with that change.
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.
@zacharis278 I've updated the text based on the above discussion. Screenshots are also added in the PR description.
@natabene Could you re-run the tests? I don't think I have permission to run them. |
@zacharis278 Could you review this PR? |
@arslanashraf7 I kicked off the tests again. |
Codecov Report
@@ Coverage Diff @@
## main #63 +/- ##
=======================================
Coverage 88.63% 88.63%
=======================================
Files 68 68
Lines 906 906
Branches 242 242
=======================================
Hits 803 803
Misses 96 96
Partials 7 7
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
@zacharis278 Does this PR need anything else or is it fit to merge? |
@natabene left a comment in the thread above. I still have some questions about the proposed language change. |
@arslanashraf7 can you squash your commits? And a nit: I think this is a "fix" not a "refactor", but I could be wrong. |
9fd267c
to
3ab94c5
Compare
@pdpinch Squashed. I was generally following this edx guide. But I think we can use "fix", I think it makes more sense than "refactor". Updated the commit message and PR title accordingly. |
3ab94c5
to
212b25c
Compare
@arslanashraf7 this looks good to me. Just shoot me a ping when you believe this is all set to merge |
Thanks @zacharis278 -- Arslan asked me to take a last look, and it looks good to me. Can you merge for us? |
@arslanashraf7 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
🎉 This PR is included in version 1.16.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Related Ticket:
mitodl/mitxonline#374
What's this PR do?
Your proctoring session was reviewed successfully. Go to your progress page to view your exam grade.
toYour proctoring session was reviewed successfully. A final grade will be available soon.
Testing Instructions:
/frontend-app-learning/packages
(If packages dir doesn't exist you can create one, that's easy to test it around)module.config.js
inside your/frontend-app-learning
directory and add the following:frontend-app-learning
container e.g.docker-compose restart frontend-app-learning
Documentation/Discussion:
A relevant discussion can be seen on this ticket
Reviewer Note:
Screenshot:
New
Old