-
Notifications
You must be signed in to change notification settings - Fork 211
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: option to show/hide ungraded assignment in progress page #1380
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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1380 +/- ##
==========================================
- Coverage 88.69% 88.69% -0.01%
==========================================
Files 312 313 +1
Lines 5501 5509 +8
Branches 1365 1334 -31
==========================================
+ Hits 4879 4886 +7
- Misses 605 606 +1
Partials 17 17 ☔ View full report in Codecov by Sentry. |
62e36e7
to
bc3c4eb
Compare
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. |
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 🙂 |
993ab24
to
26e9860
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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. |
bd4715b
to
c484d0c
Compare
Rebased, ready to be merged. |
I can review this cc: @itsjeyd @Agrendalath @bradenmacdonald |
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be moved to a separate place since this function is getting duplicated.
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.
@farhaanbukhsh Done. 1686988
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
c484d0c
to
1686988
Compare
@farhaanbukhsh This is ready for another review pass :) |
on it :) |
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.
👍
Thanks @navinkarkera and @Agrendalath for the persistence.
- ✅ I tested this on tutor devstack, tested with the flag on and off
- ✅ I read through the code
- ✅ I checked for accessibility issues
- ✅ Includes documentation
…ss page Implementation PR: openedx/frontend-app-learning#1380
* feat: option to show/hide ungraded assignment in progress page 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 * refactor: move config check to utils
…ss page Implementation PR: openedx/frontend-app-learning#1380
Adds option to show ungraded assignment/problems in progress page.
Private-ref
: BB-8824Test instructions:
tutor mounts add /path/to/frontend-app-learning
.npm install
in this directory.tutor dev launch -I
."SHOW_UNGRADED_ASSIGNMENT_PROGRESS": True,
toMFE_CONFIG
dict$(tutor config printroot)/env/apps/openedx/settings/lms/development.py
Screenshots:
SHOW_UNGRADED_ASSIGNMENT_PROGRESS = True
SHOW_UNGRADED_ASSIGNMENT_PROGRESS = False
(default)