[FC-0056][Plugin] Course outline sidebar (plugin wrapper) #1349

ihor-romaniuk · 2024-04-09T09:49:09Z

Settings

EDX_PLATFORM_REPOSITORY: https://github.com/raccoongang/edx-platform.git
EDX_PLATFORM_VERSION: ts-develop

Description

This pull request adds an important feature to our platform: displaying a navigation sidebar within a given course.

Interaction with feature flag courseware.show_default_right_sidebar has been added to control the default display of the discussion sidebar.

Discussions or Notifications sidebar shouldn't be opened on Learning MFE by default. If waffle flag enabled - Discussions always opens on the pages with discussions, if user is in Audit and course has verified mode - show Notifications.

Note: Initial solution without using Frontend Plugin Framework #1342

Depends on BE PRs

feat: [FC-0056] Implement Sidebar Navigation
Waffle flag to control of displaying the right (discussions & notifications) sidebar by default - Will be opened soon

Design

https://www.figma.com/file/gew5tORDX4Q7wxOS8vjqZu/side-nav-OEX?type=design&node-id=318-3234&mode=design&t=rBe1ToNYP8RY6QOp-0

Testing instructions

Run master devstack.
Start platform make dev.up.lms+cms+frontend-app-course-authoring and make checkout on this branch.
Go to Course Unit page from the Course Outline page.

openedx-webhooks · 2024-04-09T09:49:14Z

Thanks for the pull request, @ihor-romaniuk! 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.

This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate.

arbrandes

I have left some actual requests for changes, but mostly I'm just having a hard time understanding how sidebars are supposed to work... and that is not a good sign. So, would you mind doing the following?

Rebasing on master and squashing all commits into one, with a proper conventional commit message that describes any breaking changes (if there are any)
Removing the ENABLE_NEW_SIDEBAR environment variable so that the new one is the only one that exists; this includes removing all traces of the old sidebar
Making sure any PluginSlots introduced here don't require env.config.jsx to exist for the content they wrap to actually work
Making sure that the new sidebar is actually in a plugin slot

Let me know if there are reasons why any of the above don't make sense.

arbrandes · 2024-04-25T12:21:50Z

src/courseware/course/Course.jsx

@@ -37,7 +40,7 @@ const Course = ({
  const sequence = useModel('sequences', sequenceId);
  const section = useModel('sections', sequence ? sequence.sectionId : null);
  const enableNewSidebar = getConfig().ENABLE_NEW_SIDEBAR;


Can we get rid of this env var and just enable it by default?

I'm not sure if we should include refactoring the existing sidebars and removing the old one.
If we want to keep only the new sidebar, then we need to delete all links and files regarding the old sidebar, which can take a lot of time.

example.env.config.jsx

arbrandes · 2024-04-25T13:14:49Z

src/courseware/course/Course.jsx

+          <PluginSlot id={OUTLINE_SIDEBAR_DESKTOP_PLUGIN_SLOT_ID}>
+            <CourseOutlineTrigger isMobileView />
+          </PluginSlot>
+          {enableNewSidebar === 'true' ? <NewSidebarTriggers /> : <SidebarTriggers /> }


Again, it would be best if we just went with it and removed the old triggers.

This all involves refactoring the new and old sidebars, which I wouldn't include in this PR.

arbrandes · 2024-04-25T13:25:06Z

src/courseware/course/sidebar/sidebars/course-outline/utils.js

+  ];
+
+  return sidebarPluginSlots.every(
+    key => Object.prototype.hasOwnProperty.call(pluginSlots, key) && pluginSlots[key].keepDefault === true,


This is very nicely implemented (congrats), but the thing is, we want the sidebars to be enabled whether somebody has bothered to create an env.config.jsx or not.

In other words, the sidebar should just be on for everybody, except if they do something about it.

In another case, we must check whether the Plugin slot that relates to the navigation sidebar is disabled.

Suggested change

key => Object.prototype.hasOwnProperty.call(pluginSlots, key) && pluginSlots[key].keepDefault === true,

key => (Object.prototype.hasOwnProperty.call(pluginSlots, key) ? pluginSlots[key].keepDefault !== false : true),

arbrandes · 2024-04-25T13:29:56Z

src/courseware/data/api.js

+ */
+export async function getCourseOutline(courseId) {
+  const { data } = await getAuthenticatedHttpClient()
+    .get(`${getConfig().LMS_BASE_URL}/api/course_home/v1/navigation/${courseId}`);


Does this mean this depends on openedx/edx-platform#34457? Can you please add this fact to the description of the PR?

Also, the PR sandbox settings is one of the additional ways to document this stuff. Please make it so the edx-platform branch points to that PR, instead of the raccoongang development branch.

~~OK, this API call should be removed as this check is not required because of the wrapping sidebar into PluginSlot~~
UPD: I missread, this API call won't be removed, and is indeed related to openedx/edx-platform#34457, this will be added to description

Added the related BE PR to the "Depends on BE PRs" PR description block

arbrandes · 2024-04-25T13:32:05Z

src/courseware/data/redux.test.js

@@ -43,6 +44,7 @@ describe('Data layer integration tests', () => {
  const sequenceUrl = `${sequenceBaseUrl}/${sequenceMetadata.item_id}`;
  const sequenceId = sequenceBlocks[0].id;
  const unitId = unitBlocks[0].id;
+  const rightSidebarSettingsUrl = `${getConfig().LMS_BASE_URL}/courses/${courseId}/show-default-right-sidebar/enabled/`;


Where is this implemented in the backend? It's not in openedx/edx-platform#34457, as far as I can see. Or is it just a mock?

I believe the PR with waffle flag will be opened in the follow up PR, as we've got a recent refactoring of the flag, cause of product feedback - raccoongang/edx-platform#2514

Added a draft note about relation to BE PR to the "Depends on BE PRs" PR description block and will be updated when PR will be open

arbrandes · 2024-04-25T13:40:04Z

src/courseware/data/slice.js

+      const sequenceId = Object.keys(state.courseOutline.sequences)
+        .find(id => state.courseOutline.sequences[id].unitIds.includes(unitId));
+      const sequenceUnits = state.courseOutline.sequences[sequenceId].unitIds;
+      const isAllUnitsAreComplete = sequenceUnits.every((id) => state.courseOutline.units[id].complete);


Suggested change

const isAllUnitsAreComplete = sequenceUnits.every((id) => state.courseOutline.units[id].complete);

const areAllUnitsComplete = sequenceUnits.every((id) => state.courseOutline.units[id].complete);

Of course, thanks

arbrandes · 2024-04-25T13:40:58Z

src/courseware/data/slice.js

+      const sectionId = Object.keys(state.courseOutline.sections)
+        .find(id => state.courseOutline.sections[id].sequenceIds.includes(sequenceId));
+      const sectionSequences = state.courseOutline.sections[sectionId].sequenceIds;
+      const isAllSequencesAreComplete = sectionSequences.every((id) => state.courseOutline.sequences[id].complete);


Suggested change

const isAllSequencesAreComplete = sectionSequences.every((id) => state.courseOutline.sequences[id].complete);

const areAllSequencesComplete = sectionSequences.every((id) => state.courseOutline.sequences[id].complete);

Renamed, thanks

arbrandes · 2024-04-25T14:25:32Z

src/courseware/course/sidebar/Sidebar.jsx

+    return null;
+  }
+
+  if (layout !== SIDEBARS[currentSidebar]?.LAYOUT) {


What are we protecting against, here? An MFE developer trying to insert a sidebar on the wrong side? But if each sidebar has a hard-coded side, then why let them specify the layout as a prop in the first place?

In general, I would not remove this protection in case of further expansion and potential problems.

arbrandes · 2024-04-25T14:28:15Z

src/courseware/course/sequence/Sequence.jsx

+        <PluginSlot id={OUTLINE_SIDEBAR_MOBILE_PLUGIN_SLOT_ID}>
+          <CourseOutlineTrigger />
+        </PluginSlot>
+        <Sidebar layout={LAYOUT_LEFT} />


If the sidebar is outside the PluginSlot, how will people be able to customize it?

itsjeyd · 2024-04-26T07:50:30Z

OSPR management note: This is part of a set of PRs for implementing Phase 1 changes for the FC-0056 project. We can consider it approved from the product perspective.

CC @GlugovGrGlib

codecov · 2024-04-26T09:22:38Z

Codecov Report

Attention: Patch coverage is 95.58360% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 88.60%. Comparing base (72381a7) to head (219ecb8).

Files	Patch %	Lines
...urseware/course/sidebar/SidebarContextProvider.jsx	80.00%	4 Missing ⚠️
...ebar/sidebars/course-outline/CourseOutlineTray.jsx	91.66%	4 Missing ⚠️
...se/sidebar/sidebars/course-outline/SidebarUnit.jsx	81.81%	2 Missing ⚠️
src/courseware/data/utils.js	96.00%	2 Missing ⚠️
...r/sidebars/course-outline/CourseOutlineTrigger.jsx	95.23%	1 Missing ⚠️
src/courseware/data/slice.js	96.96%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1349      +/-   ##
==========================================
+ Coverage   88.28%   88.60%   +0.31%     
==========================================
  Files         292      312      +20     
  Lines        5002     5238     +236     
  Branches     1269     1299      +30     
==========================================
+ Hits         4416     4641     +225     
- Misses        570      581      +11     
  Partials       16       16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ihor-romaniuk · 2024-04-30T08:53:02Z

@arbrandes This PR will be closed due to open new PR without PluginSlots and managing the outline sidebar by Waffle flag #1375

openedx-webhooks · 2024-04-30T08:53:05Z

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

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

ihor-romaniuk requested review from leangseu-edx, arbrandes and KristinAoki April 9, 2024 09:49

ihor-romaniuk changed the title ~~[FC-0056][Plugin] Course outline sidebar~~ [FC-0056][Plugin] Course outline sidebar (plugin wrapper) Apr 9, 2024

ihor-romaniuk mentioned this pull request Apr 11, 2024

[FC-0056][Plugin] Course outline sidebar #1344

Closed

ihor-romaniuk force-pushed the romaniuk/sidebar-main-plugin-wrapper branch from 86fedd4 to ddb94b0 Compare April 15, 2024 09:51

itsjeyd added the product review PR requires product review before merging label Apr 22, 2024

arbrandes requested changes Apr 25, 2024

View reviewed changes

itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Apr 26, 2024

ihor-romaniuk and others added 2 commits April 26, 2024 11:10

feat: [FC-0056] create course outline sidebar

b70509f

feat: [FC-0056] cover course outline sidebar into plugin slot

219ecb8

ihor-romaniuk force-pushed the romaniuk/sidebar-main-plugin-wrapper branch from ddb94b0 to 219ecb8 Compare April 26, 2024 09:15

ihor-romaniuk mentioned this pull request Apr 26, 2024

[FC-0056] Course outline sidebar #1342

Closed

ihor-romaniuk closed this Apr 30, 2024

itsjeyd removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label May 23, 2024

GlugovGrGlib mentioned this pull request Jun 14, 2024

Reintroduce left-sidebar navigation openedx/platform-roadmap#329

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FC-0056][Plugin] Course outline sidebar (plugin wrapper) #1349

[FC-0056][Plugin] Course outline sidebar (plugin wrapper) #1349

ihor-romaniuk commented Apr 9, 2024 •

edited

Loading

openedx-webhooks commented Apr 9, 2024

arbrandes left a comment •

edited

Loading

arbrandes Apr 25, 2024

ihor-romaniuk Apr 26, 2024

arbrandes Apr 25, 2024

ihor-romaniuk Apr 26, 2024 •

edited

Loading

arbrandes Apr 25, 2024

ihor-romaniuk Apr 26, 2024 •

edited

Loading

arbrandes Apr 25, 2024

GlugovGrGlib Apr 25, 2024 •

edited

Loading

ihor-romaniuk Apr 26, 2024

arbrandes Apr 25, 2024

GlugovGrGlib Apr 25, 2024

ihor-romaniuk Apr 26, 2024

arbrandes Apr 25, 2024

ihor-romaniuk Apr 26, 2024

arbrandes Apr 25, 2024

ihor-romaniuk Apr 26, 2024

arbrandes Apr 25, 2024

ihor-romaniuk Apr 26, 2024 •

edited

Loading

arbrandes Apr 25, 2024

itsjeyd commented Apr 26, 2024

codecov bot commented Apr 26, 2024

ihor-romaniuk commented Apr 30, 2024

openedx-webhooks commented Apr 30, 2024

	key => Object.prototype.hasOwnProperty.call(pluginSlots, key) && pluginSlots[key].keepDefault === true,
	key => (Object.prototype.hasOwnProperty.call(pluginSlots, key) ? pluginSlots[key].keepDefault !== false : true),

	const isAllUnitsAreComplete = sequenceUnits.every((id) => state.courseOutline.units[id].complete);
	const areAllUnitsComplete = sequenceUnits.every((id) => state.courseOutline.units[id].complete);

	const isAllSequencesAreComplete = sectionSequences.every((id) => state.courseOutline.sequences[id].complete);
	const areAllSequencesComplete = sectionSequences.every((id) => state.courseOutline.sequences[id].complete);

[FC-0056][Plugin] Course outline sidebar (plugin wrapper) #1349

[FC-0056][Plugin] Course outline sidebar (plugin wrapper) #1349

Conversation

ihor-romaniuk commented Apr 9, 2024 • edited Loading

Description

Depends on BE PRs

Design

Testing instructions

openedx-webhooks commented Apr 9, 2024

arbrandes left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihor-romaniuk Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihor-romaniuk Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlugovGrGlib Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihor-romaniuk Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsjeyd commented Apr 26, 2024

codecov bot commented Apr 26, 2024

Codecov Report

ihor-romaniuk commented Apr 30, 2024

openedx-webhooks commented Apr 30, 2024

ihor-romaniuk commented Apr 9, 2024 •

edited

Loading

arbrandes left a comment •

edited

Loading

ihor-romaniuk Apr 26, 2024 •

edited

Loading

ihor-romaniuk Apr 26, 2024 •

edited

Loading

GlugovGrGlib Apr 25, 2024 •

edited

Loading

ihor-romaniuk Apr 26, 2024 •

edited

Loading