-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: [FC-0056] Waffle flag for opening discussions/notifications by default #34650
feat: [FC-0056] Waffle flag for opening discussions/notifications by default #34650
Conversation
Thanks for the pull request, @NiedielnitsevIvan! 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. |
Wasn't this just added via #34561? |
I don't think so, the work from referenced PR is not related to the scope of FC-0056, #34561 PR contains an additional configuration for fully new functionality of the discussions on the course home page, this PR instead adds waffle flag for logic control on unit page only. |
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.
Aside from the comments below, I have one question regarding performance: was there no way to return the state of these new flags via an existing request that the MFE already makes? It would be great not to add another server request unless there's no other (clean) way.
lms/djangoapps/courseware/toggles.py
Outdated
# .. toggle_description: If waffle flag disabled | ||
# Discussions or Notifications sidebar shouldn't be displayed at all on Learning MFE. | ||
# If waffle flag enabled - Discussions opens always on the pages with discussions, | ||
# if user is in Audit and course has verified mode - show Notifications.# .. toggle_use_cases: temporary |
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.
As discussed with @GlugovGrGlib yesterday, we're going to need a more extensive description of what happens when this flag is toggled.
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.
Added
lms/djangoapps/courseware/toggles.py
Outdated
# .. toggle_tickets: FC-0056 | ||
# .. toggle_warning: None. | ||
COURSEWARE_SHOW_DEFAULT_RIGHT_SIDEBAR = CourseWaffleFlag( | ||
f'{WAFFLE_FLAG_NAMESPACE}.show_default_right_sidebar', __name__ |
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.
To be honest, I think assuming this will be on the right is a mistake: it might not be on the right in all circumstances. We should come up with a more semantic name for it. If we're calling the other one the navigation sidebar, we could maybe call this one the "notification" one. Or something even more generic, like "auxiliary" sidebar. @jmakowski1123, thoughts?
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, and we should call it something more immediately descriptive like always_open_auxiliary_sidebar
.
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 for the suggestions, I agree that generic name is more suitable for this
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.
Applied
lms/djangoapps/courseware/toggles.py
Outdated
# .. toggle_name: courseware.always_open_auxiliary_sidebar | ||
# .. toggle_implementation: WaffleFlag | ||
# .. toggle_default: True | ||
# .. toggle_description: TBD |
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.
TODO: @GlugovGrGlib to finalize description for toggle
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.
Added
Co-authored-by: Adolfo R. Brandes <[email protected]>
5db8b8e
to
f4763a3
Compare
I can't update description, I'll ask @NiedielnitsevIvan as he back in office, for now I will put the latest description there. The updated PR description: Settings
DescriptionThis PR adding a waffle toggles for the Navigation and auxiliary sidebar features within the learning MFE. How to configure waffle flag:
API to get waffle flag state:{LMS_DOMAIN}/courses/{course_id}/courseware-navigation-sidebar/toggles/ Testing API instructions:
|
f4763a3
to
956e59e
Compare
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.
Looks good, thanks!
@NiedielnitsevIvan 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
This PR is the part of - openedx/platform-roadmap#329 |
Settings
Description
This PR adding a waffle toggles for the Navigation and auxiliary sidebar features within the learning MFE.
The main goal is to implement two WaffleFlags and create an endpoint to get the enabled status for them.
GH ticket for tracking - openedx/platform-roadmap#329
How to configure waffle flag:
API to get waffle flag state:
{LMS_DOMAIN}/courses/{course_id}/courseware-navigation-sidebar/toggles/
Testing API instructions:
/courses/{course_id}/courseware-navigation-sidebar/toggles/