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: open response assesment detail (problem steps) UI #263

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

johnvente
Copy link
Contributor

@johnvente johnvente commented Oct 9, 2023

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

ora-frontend-dem

BEFORE NOW

Responses list

responses

Assesment list

now-ora-fix

Assesment feedback received

assesment-received

Assesment feedback given

assesment-given

Here's what you see in the video explained:

  • There is an ORA open response type "Staff Assessment Only."
  • The instructor opens the new experience of ORA that contains the submissions of that Open response.
  • The instructor can see the details and more information, such as email, full name, list of responses, and feedback in detail.

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

  • Create an unit with an open response type "Staff Assessment Only."
  • Submit the response
  • Go to instructor tab
  • In the unit click in "View and grade responses"

Now you can see the UI implemented

NOTE: it depends on this PR: openedx/edx-platform#33632

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 9, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Oct 9, 2023

Thanks for the pull request, @johnvente! 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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@johnvente johnvente changed the title feat problem steps detail UI feat: Open Response Assesment detail (problem steps) UI Oct 9, 2023
@johnvente johnvente changed the title feat: Open Response Assesment detail (problem steps) UI feat: open response assesment detail (problem steps) UI Oct 9, 2023
@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (9cfab58) to head (6736a65).
Report is 3 commits behind head on master.

Current head 6736a65 differs from pull request most recent head f19f7c0

Please upload reports for the commit f19f7c0 to get more accurate results.

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.
📢 Have feedback on the report? Share it here.

@johnvente johnvente force-pushed the jv/feat-problem-steps-detail-ui branch 2 times, most recently from 9f6f802 to 51c5394 Compare October 9, 2023 20:33
@e0d
Copy link
Contributor

e0d commented Oct 20, 2023

@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.

Comment on lines +27 to +32
const problemSteps = {
problemStepsTraining: true,
problemStepsPeers: false,
problemStepsSelf: true,
problemStepsStaff: true,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it does

Copy link
Member

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}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the destination "#"?

Copy link
Contributor Author

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

Copy link
Member

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',
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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.

},
problemSteps: {
id: 'ora-grading.ListView.problemSteps',
defaultMessage: 'Problem Steps',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
defaultMessage: 'Problem Steps',
defaultMessage: 'Problem steps',

Copy link
Contributor Author

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

Copy link
Member

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.

defaultMessage: 'Error',
description: 'Error title when response list has failed',
},
responseListMessageError: {
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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/index.jsx Outdated Show resolved Hide resolved
Comment on lines +36 to +37
* @param {boolean} showReview - show modal for the review
* @param {string} submissionUUIDParam - to set an expecifict submission ID to load
Copy link
Member

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?

@johnvente johnvente force-pushed the jv/feat-problem-steps-detail-ui branch 3 times, most recently from 948551e to 78b5a29 Compare December 26, 2023 21:21
Comment on lines +27 to +32
const problemSteps = {
problemStepsTraining: true,
problemStepsPeers: false,
problemStepsSelf: true,
problemStepsStaff: true,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it does

@arbrandes
Copy link

Before I do a proper review, do we have a thumbs-up from Product, for this one?

@johnvente
Copy link
Contributor Author

johnvente commented Mar 21, 2024

Hi @arbrandes this is the thumbs-up from Product: openedx/edx-ora2#1974
I will add it to the description as well

@mphilbrick211
Copy link

Hi @arbrandes just checking in on this!

@BryanttV
Copy link

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.

@arbrandes
Copy link

@BryanttV, mind tackling that rebase so I can test this on master?

@BryanttV BryanttV force-pushed the jv/feat-problem-steps-detail-ui branch from f8af29c to 6e41dee Compare May 17, 2024 18:07
@BryanttV
Copy link

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U waiting for eng review PR is ready for review. Review and merge it, or suggest changes.
Projects
Status: Ready for Review
Development

Successfully merging this pull request may close these issues.

None yet

7 participants