feat: option to show/hide ungraded assignment in progress page #1380
Conversation
|
Thanks for the pull request, @navinkarkera! 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. |
openedx-webhooks
added
the
open-source-contribution
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1380 +/- ##
==========================================
- Coverage 87.74% 87.73% -0.01%
==========================================
Files 310 310
Lines 5319 5326 +7
Branches 1349 1322 -27
==========================================
+ Hits 4667 4673 +6
- Misses 635 636 +1
Partials 17 17 ☔ View full report in Codecov by Sentry. |
navinkarkera
force-pushed
the
navin/show-ungraded-progress
branch
2 times, most recently
from
62e36e7
to
bc3c4eb
Compare
itsjeyd
added
the
core contributor
|
Hey @navinkarkera, this seems to be affecting user-facing features -- is there a product proposal for it? If not, you'll need to create one. See Product Review Process to get started. CC @Agrendalath |
|
@itsjeyd, this is still under the client QA, and we'll need to make some adjustments to the current approach. Edit: I don't have permission to change this to a draft, but please feel free to do this if you can. |
|
@Agrendalath I don't have permission to change to draft either, looks like @navinkarkera would have to do it himself.
Good to know! If you don't expect to be making massive changes, I'd suggest to get started on the product proposal sooner rather than later, as it can take a while to complete. |
navinkarkera
force-pushed
the
navin/show-ungraded-progress
branch
from
bc3c4eb
to
993ab24
Compare
|
@Agrendalath @itsjeyd Created a ticket: openedx/platform-roadmap#359 in platform-roadmap GitHub repository |
|
Thanks @navinkarkera. I think you'll also need to add a proposal to the wiki (per step 1a of Product Review Process). CC @ali-hugo @cassiezamparini for confirmation -- the process hasn't changed, right? Starting the product review process requires both a wiki proposal and a road map ticket? |
|
@itsjeyd There doesn't seem to be a hard and fast rule about this. I've seen some smaller proposals accepted with just the road map ticket, and no wiki proposal - I think this proposal might fall into that category (like @navinkarkera mentioned on the wg-product-core Slack channel, the change is relatively minor, and reintroduces a legacy feature that's missing from the new MFE). I will confirm this in the Core Product meeting tomorrow and let you both know. |
|
@navinkarkera @itsjeyd Ok, I got some clarification about the review process (I will be updating the wiki procedure soon). If openedx/platform-roadmap#359 simply reintroduces missing legacy functionality, and does not introduce/change any user-facing features, it does not require a separate wiki proposal (@navinkarkera Can you confirm that this is the case?) What we do need, is a second reviewer who is not from OpenCraft. Do either of you know of a Product person who might have some experience with the Progress Page? |
|
Thanks a lot for the follow-up and info @ali-hugo!
I don't, unfortunately. |
|
@ali-hugo Thanks for the clarification!
I haven't used this feature before but I think @Agrendalath can confirm that it was part of legacy.
Unfortunately, no. |
|
@navinkarkera I decided to take a look through the screenshots in the meantime. Everything looks good to me! Once we have confirmation that this is indeed a legacy feature, I will find a second person to do a product review. |
|
@ali-hugo, this is how ungraded and graded subsections look on the legacy Progress page. We just changed "Practice Scores" to "Ungraded Problem Scores", as the latter is more self-explanatory. |
|
@Agrendalath Thanks for the screenshot. That wording update makes sense to me. 👍 @asma-ahmedd Would you mind taking a look through this PR when you have a moment, please? We need a second product thumbs up before it can move forward. Here is a link to the roadmap ticket with some background. Feel free to reach out to me if you have any questions. |
|
@ali-hugo Positive Aspect It's crucial to maintain consistency between the legacy system and the new MFE, and adding the option to display ungraded assignments/problems addresses this need effectively. Enabling the feature via a flag in MFE_CONFIG allows for easy toggling and testing without affecting the entire system immediately. The display of ungraded assignment progress will undoubtedly benefit both learners and instructors.
Recommendations
Questions
Conclusion |
|
@asma-ahmedd Thanks for responding.
I believe that all problems under a graded subsection are considered as graded problems except for the problems with
Not sure if I understand the question here. I think until a learner completes the problem, no score is awarded which translates to 0 in the progress page. |
|
@asma-ahmedd Thanks for the detailed review!
|
This is the up-to-date version of this document. We could update the linked section to add information about the option to display ungraded assignments. cc: @navinkarkera |
|
Thanks @Agrendalath 🙂 |
navinkarkera
force-pushed
the
navin/show-ungraded-progress
branch
from
993ab24
to
26e9860
Compare
…ss page Implementation PR: openedx/frontend-app-learning#1380
|
@ali-hugo The mockup looks good to me. I agree with this approach. Let's proceed and conclude this 👍 |
|
@asma-ahmedd Thanks for the quick response, and for your help reviewing this PR! @Agrendalath @navinkarkera This has the "go ahead" from product. 🚀 |
|
@ali-hugo @Agrendalath Updated.
|
|
@navinkarkera Looks great! Just one thing - I think we decided to change the term "Problem Scores" to "Graded Scores", didn't we? (see this comment). |
|
@navinkarkera Friendly ping on this ⬆️ 🙂 |
|
Hey @navinkarkera, when do you think you could get back to @ali-hugo's comment above? |
navinkarkera
force-pushed
the
navin/show-ungraded-progress
branch
from
7188314
to
3ab1792
Compare
|
Thanks @navinkarkera! Looks like this is back in draft state now. Let me know when it's ready so we can get the engineering review process started. |
|
@navinkarkera Looks good! Thank you. |
navinkarkera
force-pushed
the
navin/show-ungraded-progress
branch
from
3ab1792
to
bd4715b
Compare
…ss page Implementation PR: openedx/frontend-app-learning#1380
|
Hey @navinkarkera, any updates on when this PR will be ready to move out of draft state and get a technical review? |
|
@itsjeyd @Agrendalath Is planning to deploy this for client's QA this week. |
There was a problem hiding this comment.
@navinkarkera, @itsjeyd, we're good to move forward with this.
👍
- I tested this: checked that the ungraded assignments are displayed on the Progress page, along with the explanation at the top
- I read through the code
- I checked for accessibility issues
- Includes documentation: feat: document mfe_config flag to display ungraded problems in progress page edx-documentation#2268
|
@navinkarkera, @itsjeyd, who should we ask to review and merge this? |
|
@navinkarkera, @itsjeyd, a quick reminder on the ping above ^ |
|
@Agrendalath Sorry for the late response, I was off when you pinged me and only came back to work earlier this week. Maintainership for this repo was transferred from 2U (Aurora) to OpenCraft (@bradenmacdonald and @farhaanbukhsh), so the two of them should be able to merge the changes once @navinkarkera marks them as ready and the build turns green. |
test: add tests for show ungraded toggle feat: update score label in progress page based on grading refactor: update score label text and add tooltip refactor: move label tooltip near header as normal text refactor: update problem score label Graded scores
navinkarkera
force-pushed
the
navin/show-ungraded-progress
branch
from
bd4715b
to
c484d0c
Compare
|
Rebased, ready to be merged. |
|
I can review this cc: @itsjeyd @Agrendalath @bradenmacdonald |
itsjeyd
added
the
waiting for eng review
There was a problem hiding this comment.
@navinkarkera Thank you for the work on the PR, overall the approach looks good but can you look into this one comment
| @@ -28,6 +29,11 @@ const DetailedGrades = ({ intl }) => { | |||
| } = useModel('progress', courseId); | |||
|
|
|||
| const hasSectionScores = sectionScores.length > 0; | |||
| const showUngradedAssignments = ( | |||
There was a problem hiding this comment.
This can be moved to a separate place since this function is getting duplicated.
Adds option to show ungraded assignment/problems in progress page.
Private-ref: BB-8824Test instructions:
tutor mounts add /path/to/frontend-app-learning.npm installin this directory.tutor dev launch -I."SHOW_UNGRADED_ASSIGNMENT_PROGRESS": True,toMFE_CONFIGdict$(tutor config printroot)/env/apps/openedx/settings/lms/development.pyScreenshots:
SHOW_UNGRADED_ASSIGNMENT_PROGRESS = TrueSHOW_UNGRADED_ASSIGNMENT_PROGRESS = False(default)