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: [FC-0056] Waffle flag for opening discussions/notifications by default #34650

Conversation

NiedielnitsevIvan
Copy link
Contributor

@NiedielnitsevIvan NiedielnitsevIvan commented Apr 29, 2024

Settings

COURSEWARE_WAFFLE_FLAGS:
  - name: courseware.enable_navigation_sidebar
    everyone: true
  - name: courseware.always_open_auxiliary_sidebar
    everyone: true

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:

  • Go to /admin/waffle/flag/.
  • Add courseware.enable_navigation_sidebar waffle flag
  • Add courseware.always_open_auxiliary_sidebar waffle flag

API to get waffle flag state:

{LMS_DOMAIN}/courses/{course_id}/courseware-navigation-sidebar/toggles/

Testing API instructions:

  • Set waffle flags as described in "How to configure waffle flag"
  • Go to the API endpoint /courses/{course_id}/courseware-navigation-sidebar/toggles/
  • Assess the returned result. The API endpoint should return the correct WaffleFlags statuses.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 29, 2024
@openedx-webhooks
Copy link

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:

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

@arbrandes
Copy link
Contributor

Wasn't this just added via #34561?

@GlugovGrGlib
Copy link
Member

GlugovGrGlib commented Apr 30, 2024

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.

Copy link
Contributor

@arbrandes arbrandes left a 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.

Comment on lines 102 to 105
# .. 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
Copy link
Contributor

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.

Copy link
Member

Choose a reason for hiding this comment

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

Added

# .. toggle_tickets: FC-0056
# .. toggle_warning: None.
COURSEWARE_SHOW_DEFAULT_RIGHT_SIDEBAR = CourseWaffleFlag(
f'{WAFFLE_FLAG_NAMESPACE}.show_default_right_sidebar', __name__
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member

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

Copy link
Member

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 Show resolved Hide resolved
lms/djangoapps/courseware/toggles.py Outdated Show resolved Hide resolved
lms/djangoapps/courseware/toggles.py Outdated Show resolved Hide resolved
lms/djangoapps/courseware/toggles.py Outdated Show resolved Hide resolved
lms/urls.py Outdated Show resolved Hide resolved
lms/urls.py Outdated Show resolved Hide resolved
# .. toggle_name: courseware.always_open_auxiliary_sidebar
# .. toggle_implementation: WaffleFlag
# .. toggle_default: True
# .. toggle_description: TBD
Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

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

Added

@GlugovGrGlib GlugovGrGlib requested a review from arbrandes May 3, 2024 19:18
@GlugovGrGlib GlugovGrGlib force-pushed the NiedielnitsevIvan/FC-0056/feature/waffle-flag-for-displaying-discussions-tab branch from 5db8b8e to f4763a3 Compare May 3, 2024 19:21
@GlugovGrGlib
Copy link
Member

GlugovGrGlib commented May 3, 2024

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

COURSEWARE_WAFFLE_FLAGS:
  - name: courseware.enable_navigation_sidebar
    everyone: true
  - name: courseware.always_open_auxiliary_sidebar
    everyone: true

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.

How to configure waffle flag:

  • Go to /admin/waffle/flag/.
  • Add courseware.enable_navigation_sidebar waffle flag
  • Add courseware.always_open_auxiliary_sidebar waffle flag

API to get waffle flag state:

{LMS_DOMAIN}/courses/{course_id}/courseware-navigation-sidebar/toggles/

Testing API instructions:

  • Set waffle flags as described in "How to configure waffle flag"
  • Go to the API endpoint /courses/{course_id}/courseware-navigation-sidebar/toggles/
  • Assess the returned result. The API endpoint should return the correct WaffleFlags statuses.

@GlugovGrGlib GlugovGrGlib force-pushed the NiedielnitsevIvan/FC-0056/feature/waffle-flag-for-displaying-discussions-tab branch from f4763a3 to 956e59e Compare May 3, 2024 19:33
Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@arbrandes arbrandes merged commit 333f1ff into openedx:master May 6, 2024
45 of 109 checks passed
@openedx-webhooks
Copy link

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

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@GlugovGrGlib
Copy link
Member

This PR is the part of - openedx/platform-roadmap#329

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
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants