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

fix: [FC-0044] Course unit - Replaced the LMS endpoints #882

Conversation

PKulkoRaccoonGang
Copy link
Contributor

@PKulkoRaccoonGang PKulkoRaccoonGang commented Mar 10, 2024

Settings

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

TUTOR_GROVE_WAFFLE_FLAGS:
  - name: contentstore.new_studio_mfe.use_new_unit_page
    everyone: true

TUTOR_GROVE_MFE_LMS_COMMON_SETTINGS:
  MFE_CONFIG:
    ENABLE_UNIT_PAGE: true

Description

This PR is the final stage in the process of replacing the LMS of endpoints for navigation on the units of the course.

The primary features were implemented:

  • Code refactoring and replacing LMS endpoints with Studio endpoints.

Issue: openedx/platform-roadmap#321

Developer notes

  • To view the units of the course, now it is not necessary to publish sections, etc.

Testing instructions

  1. Run master devstack.
  2. Start platform make dev.up.lms+cms+frontend-app-course-authoring and make checkout on this branch.
  3. Enable the new Unit page by adding a waffle flag contentstore.new_studio_mfe.use_new_unit_page in the CMS admin panel.
  4. Make sure that the MFE setting ENABLE_UNIT_PAGE=true is enabled.
  5. Go to the Course Unit page from the Course Outline page.
Screen.Recording.2024-03-10.at.17.21.22.mov

@PKulkoRaccoonGang PKulkoRaccoonGang requested a review from a team as a code owner March 10, 2024 11:23
@openedx-webhooks
Copy link

openedx-webhooks commented Mar 10, 2024

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

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 10, 2024
@PKulkoRaccoonGang PKulkoRaccoonGang changed the title fix: [AXIMST-416] replaced the LMS endpoint for navigating the course… fix: [AXIMST-416] Course unit - Replaced the LMS endpoints Mar 10, 2024
@PKulkoRaccoonGang PKulkoRaccoonGang marked this pull request as draft March 10, 2024 11:23
Copy link

codecov bot commented Mar 10, 2024

Codecov Report

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

Project coverage is 91.19%. Comparing base (17b1360) to head (e396b1c).

Files Patch % Lines
src/course-unit/CourseUnit.jsx 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #882      +/-   ##
==========================================
+ Coverage   90.64%   91.19%   +0.54%     
==========================================
  Files         578      578              
  Lines       10037     9913     -124     
  Branches     2142     2122      -20     
==========================================
- Hits         9098     9040      -58     
+ Misses        899      839      -60     
+ Partials       40       34       -6     

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

@PKulkoRaccoonGang PKulkoRaccoonGang added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Mar 10, 2024
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@PKulkoRaccoonGang PKulkoRaccoonGang changed the title fix: [AXIMST-416] Course unit - Replaced the LMS endpoints fix: [FC-0044] Course unit - Replaced the LMS endpoints Mar 10, 2024
@PKulkoRaccoonGang PKulkoRaccoonGang self-assigned this Mar 10, 2024
@PKulkoRaccoonGang PKulkoRaccoonGang marked this pull request as ready for review March 10, 2024 15:23
fix: [AXIMST-424] Course unit - Fixed network connection behavior (#138)

* fix: [AXIMST-424] fixed network connetcion behavior

* fix: added placeholder for unsuccessful loading for the page

* refactor: code refactoring
@PKulkoRaccoonGang PKulkoRaccoonGang force-pushed the Peter_Kulko/replacing-endpoints-from-LMS-to-CMS branch from 713ca4b to e396b1c Compare March 10, 2024 15:37
@PKulkoRaccoonGang PKulkoRaccoonGang added create-sandbox open-craft-grove should create a sandbox environment from this PR and removed create-sandbox open-craft-grove should create a sandbox environment from this PR labels Mar 10, 2024
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@PKulkoRaccoonGang PKulkoRaccoonGang added create-sandbox open-craft-grove should create a sandbox environment from this PR and removed create-sandbox open-craft-grove should create a sandbox environment from this PR labels Mar 11, 2024
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@bradenmacdonald
Copy link
Contributor

This PR seems to depend on openedx/edx-platform#34055 and openedx/edx-platform#34337 , so I don't think it should be merged until those ones are merged.

Also, the error handling is not great; when I didn't have those PRs checked out, I just see something like this:
Screenshot 2024-03-11 at 12 30 28 PM

Instead of "Error loading navigation" or "Error loading components" or something.

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.

@PKulkoRaccoonGang, in the future, please make an explicit note in the description when the PR depends on another one being merged first. That way we avoid merging broken stuff accidentally.

It might also make sense to always have such PRs be drafts. Let's discuss this further.

Also, what @bradenmacdonald said. We probably need to improve the error handling.

@PKulkoRaccoonGang
Copy link
Contributor Author

PKulkoRaccoonGang commented Mar 12, 2024

@arbrandes The current PR is independent and does not depend on other open PRs.
In the new PR that was recently opened, I included important information about PR dependencies as a separate message.

@PKulkoRaccoonGang
Copy link
Contributor Author

@arbrandes @bradenmacdonald thanks 👍

error handling is not great

Yes, it would be great to handle errors that may occur and show a user-friendly message. We discussed this with the team today and propose to do this later in a separate PR.

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.

I love PRs that remove more code than they add. Nice, approved! I do have one question below, though it's not blocking.

courseExitPageIsActive: data.course_exit_page_is_active,
certificateData: camelCaseObject(data.certificate_data),
entranceExamData: camelCaseObject(data.entrance_exam_data),
timeOffsetMillis: getTimeOffsetMillis(headers && headers.date, requestTime, responseTime),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: what did we need this for, before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, we transferred functionality from MFE Learning, so the data received from the LMS endpoint is formatted there. We decided not to refactor the code that was awaiting replacement and after switching to Studio endpoints, delete it.

@arbrandes arbrandes merged commit 8acd27d into openedx:master Mar 13, 2024
6 checks passed
@openedx-webhooks
Copy link

@PKulkoRaccoonGang 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
create-sandbox open-craft-grove should create a sandbox environment from this PR 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.

6 participants