-
Notifications
You must be signed in to change notification settings - Fork 41
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: open response assesment detail (problem steps) UI #263
feat: open response assesment detail (problem steps) UI #263
Conversation
Thanks for the pull request, @johnvente! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #263 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 109 138 +29
Lines 1079 1346 +267
Branches 160 227 +67
==========================================
+ Hits 1079 1346 +267 ☔ View full report in Codecov by Sentry. |
9f6f802
to
51c5394
Compare
@johnvente noting that there are some check failures and conflicts that need attention. Also, note that this is still marked draft and the title says not to review. |
const problemSteps = { | ||
problemStepsTraining: true, | ||
problemStepsPeers: false, | ||
problemStepsSelf: true, | ||
problemStepsStaff: true, | ||
}; |
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.
Yep, it 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.
Good! Thanks
); | ||
|
||
formatStatus = ({ value }) => (<StatusBadge status={value} />); | ||
emailAddressCell = ({ value }) => ( | ||
<Hyperlink destination="#" showLaunchIcon={false}> |
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.
why is the destination "#"?
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.
Because it's a link but won't redirect to any destination
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.
Oh! Thank you
@@ -38,9 +38,14 @@ const messages = defineMessages({ | |||
}, | |||
learnerSubmissionDate: { | |||
id: 'ora-grading.ListView.tableHeaders.learnerSubmissionDate', | |||
defaultMessage: 'Learner submission date', | |||
defaultMessage: 'Submission date', |
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.
why are we changing Learner submission date
for Submission date
?
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.
It was a request in the view by @juancamilom
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.
So it got through product review. Thanks.
src/containers/ListView/messages.js
Outdated
}, | ||
problemSteps: { | ||
id: 'ora-grading.ListView.problemSteps', | ||
defaultMessage: 'Problem Steps', |
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.
defaultMessage: 'Problem Steps', | |
defaultMessage: 'Problem steps', |
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.
These titles are columns in the table they should be in capitalize
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.
Didn't know that. Thanks.
src/containers/ReviewProblemStepsModal/ReviewProblemStepsContent.jsx
Outdated
Show resolved
Hide resolved
defaultMessage: 'Error', | ||
description: 'Error title when response list has failed', | ||
}, | ||
responseListMessageError: { |
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.
why do we need multiple messages for the same error?
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.
We do not need the same error message, I did not know which one to use here they should be different
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.
Should we ask internally?
src/containers/ReviewProblemStepsModal/components/ReviewProblemStepsContent/utils.js
Outdated
Show resolved
Hide resolved
* @param {boolean} showReview - show modal for the review | ||
* @param {string} submissionUUIDParam - to set an expecifict submission ID to load |
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.
should we use this kind of documentation for the modules we're adding?
948551e
to
78b5a29
Compare
const problemSteps = { | ||
problemStepsTraining: true, | ||
problemStepsPeers: false, | ||
problemStepsSelf: true, | ||
problemStepsStaff: true, | ||
}; |
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.
Yep, it does
Before I do a proper review, do we have a thumbs-up from Product, for this one? |
Hi @arbrandes this is the thumbs-up from Product: openedx/edx-ora2#1974 |
Hi @arbrandes just checking in on this! |
We have just updated the PR including the necessary changes to consume the API in edx-ora2. There is still a need to rebase with master, resolve conflicts, and correct the unit tests. |
@BryanttV, mind tackling that rebase so I can test this on master? |
f8af29c
to
6e41dee
Compare
Hi @arbrandes, I already rebased with master, you can test now. I'm not sure why the tests are failing, but we will spend some time trying to fix it internally. Thanks! |
@BryanttV @arbrandes just checking in on this! |
Hello, @mphilbrick211. Thanks for the ping! We currently don't have enough capacity to continue this work, so it's best to close it until someone can pick it up again. Thanks for the patience :) |
Description
This PR allows us to see in detail each step of a open response, which means 'Training,' 'Peer,' 'Self,' 'Staff,' and can also see feedback received and feedback given
Further information
openedx/edx-ora2#1974
Responses list
Assesment list
Assesment feedback received
Assesment feedback given
Here's what you see in the video explained:
Why are we proposing these changes?
This improvement enables instructors to access detailed information for each Open Response, making it easier for them to review ORA responses and maintain better control over the feedback process.
How to test
Now you can see the UI implemented
NOTE: it depends on this PR: openedx/edx-platform#33632