-
Notifications
You must be signed in to change notification settings - Fork 4k
Fix notes visibility per user preference #32872
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
Conversation
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.
@VanDavv For now, the solution doesn't seem to fix the issue. Can you please update the testing instructions and update your fix.
from unittest.mock import Mock # lint-amnesty, pylint: disable=wrong-import-order | ||
from lms.djangoapps.courseware.tests.factories import StudentInfoFactory | ||
from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase | ||
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order | ||
from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order |
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.
@VanDavv Why are the # lint-amnesty, pylint: disable=wrong-import-order
necessary here?
Thanks for the pull request, @VanDavv! 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. Once you've signed the CLA, please allow 1 business day for it to be processed. After this time, you can re-run the CLA check by editing the PR title. If the problem persists, you can tag the |
Hi @VanDavv , we need to have a valid Contributor License Agreement (CLA) in place for all contributions. See the welcome message above for the details about how to enroll. The process is different depending upon whether you are making this contribution as an individual or on behalf of your employer. |
We fixed it in #33096 and openedx/frontend-app-learning#1170, so we can close this. |
@VanDavv Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
1 similar comment
@VanDavv Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This Pull Request addresses the issue mentioned in this ticket - openedx/frontend-app-learning#1005. Before this change, notes visibility preference was only taking into account whether the notes are visible for a course or not (globally), not checking the user preference available in XModuleStudentInfoField (which is used to determine whether the notes are highlighted or not)
Supporting information
Directly addresses the following ticket - openedx/frontend-app-learning#1005
Can also resolve the following ticket - openedx/wg-build-test-release#227 - although I was not yet able to consistently reproduce the issue there, will investigate
Testing instructions
A new test case has been created, called
test_notes_visibility.py
, which contains two tests, one with user preference set to True, and one to false. Running these tests without changes in this PR will result in one of them failing, as it assumes the courseware API call will return notes visibility asFalse
when user preference for visibility is also set toFalse
.To execute the test, please run the following command
Deadline
3rd of August