fix: force LMS url to reload when changed #136

ghassanmas · 2023-04-18T00:49:27Z

What does this change do:

1- This change makes it possible if LMS_BASE_URL is updated by dynamic config to get the latest value. see context below resolved in first commit
2- It make contacte course search url when it's relative resolved in second commit

Testing:

While using tutor-mfe

add new this mfe to the list of MFEs how
Ensure those settings are set, can be done creating file plugin
If you use the master branch it will not work, while when using this branch shall fix it from step 1 you can alter repository and versoin values

More context

Fixes that are simlar to this

gradebook feat: ensure lms api synced with latest value in config frontend-app-gradebook#290
course authoring fix: force studio url to reload if changed frontend-app-authoring#389

openedx-webhooks · 2023-04-18T00:49:36Z

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

codecov · 2023-04-18T01:21:08Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.21 🎉

Comparison is base (5db95b0) 95.75% compared to head (8d91a12) 95.96%.

❗ Current head 8d91a12 differs from pull request most recent head ce88d29. Consider uploading reports for the commit ce88d29 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
+ Coverage   95.75%   95.96%   +0.21%     
==========================================
  Files         165      170       +5     
  Lines        1436     1486      +50     
  Branches      255      274      +19     
==========================================
+ Hits         1375     1426      +51     
+ Misses         56       55       -1     
  Partials        5        5

Impacted Files	Coverage Δ
...Card/components/CourseCardBanners/CourseBanner.jsx	`95.23% <ø> (ø)`
src/containers/CourseList/NoCoursesView/index.jsx	`100.00% <ø> (ø)`
...arnerDashboardHeader/AuthenticatedUserDropdown.jsx	`100.00% <ø> (ø)`
src/containers/LearnerDashboardHeader/index.jsx	`100.00% <ø> (ø)`
src/data/redux/hooks/app.js	`97.95% <ø> (ø)`
src/data/services/lms/api.js	`100.00% <ø> (ø)`
src/widgets/LookingForChallengeWidget/index.jsx	`100.00% <ø> (ø)`
src/widgets/RecommendationsPanel/LoadedView.jsx	`100.00% <ø> (ø)`
...HeaderVariant/CollapsedHeader/CollapseMenuBody.jsx	`93.75% <100.00%> (ø)`
...nerDashboardHeaderVariant/ExpandedHeader/index.jsx	`100.00% <100.00%> (ø)`
... and 2 more

... and 15 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

itsjeyd · 2023-04-20T12:48:01Z

label: core contributor

itsjeyd · 2023-04-20T12:54:55Z

@ghassanmas Thanks for your contribution!

Except for needing a rebase, it looks like this is ready for review?

ghassanmas · 2023-04-20T22:42:10Z

@itsjeyd yes you are correct, I have rebased it, also fixed a similar issue found while testing it. It's now ready for review.

itsjeyd · 2023-04-27T07:12:06Z

@mattcarter This PR is ready for review by Aurora.

ghassanmas · 2023-05-03T07:51:17Z

For refrence this should be added to:

Backport before palm.1 release wg-build-test-release#261

ghassanmas · 2023-05-03T21:06:00Z

This branch has been rebased more than 5 times, should I keep an eye on it? is rebasing necessary assuming it doens't cause conflict?

itsjeyd · 2023-05-09T09:28:04Z

@ghassanmas Good question. The PR branch should be up-to-date with master when engineering review starts and when it's about to be merged. But if master is moving fast and it takes a while for engineering review to start, it might not make sense to rebase the changes "every five minutes". Once or twice per week might be more reasonable.

CC @mphilbrick211, in case you have any additional input to share here.

ghassanmas · 2023-05-09T10:40:27Z

@itsjeyd Understandable, frankly the situation seems like, you need to wait for the bus in a cold weather, so you plan to leave your house at the time you would wait the least for the bus. Albeit here a timeable for the bus schedule doesn't exist 😄.

itsjeyd · 2023-05-10T08:47:14Z

@ghassanmas You're not wrong 😅

The situation should change once this repo joins the maintainership program.

mphilbrick211 · 2023-05-10T18:01:19Z

@itsjeyd @ghassanmas If the branch becomes out-of-date because we're waiting on a reviewer, the repo owner/merger should be able to fix the date issue before merging - that way, you don't need to keep rebasing as changes to the master happen. Hope this helps!

ghassanmas · 2023-05-11T06:57:33Z

@itsjeyd @ghassanmas If the branch becomes out-of-date because we're waiting on a reviewer, the repo owner/merger should be able to fix the date issue before merging - that way, you don't need to keep rebasing as changes to the master happen. Hope this helps!

@mphilbrick211

So does that mean the label waiting on author shouldn't stop the reviewer from reviewing the PR? assuming the label is assigend just because of rebase
secondaly is that the case for both when the PR is based upon a fork (the current state of this PR) or the original repo?

mphilbrick211 · 2023-05-12T17:29:05Z

@itsjeyd @ghassanmas If the branch becomes out-of-date because we're waiting on a reviewer, the repo owner/merger should be able to fix the date issue before merging - that way, you don't need to keep rebasing as changes to the master happen. Hope this helps!

@mphilbrick211

So does that mean the label waiting on author shouldn't stop the reviewer from reviewing the PR? assuming the label is assigend just because of rebase

secondaly is that the case for both when the PR is based upon a fork (the current state of this PR) or the original repo?

@ghassanmas I'm not sure about the fork question (maybe Tim can weigh in here - I'm not sure if it would make a difference if it's a fork or not), but re: Waiting for Author... The LABEL "Waiting for Author" is used when we're waiting on the author to answer a question, usually when the PR is already in review, or in the early stages of the PR if more information is needed.

The STATUS "Waiting for Author" means the PR is still in-progress and not yet ready-for-review.

ghassanmas · 2023-05-15T12:17:47Z

The STATUS "Waiting for Author" means the PR is still in-progress and not yet ready-for-review.

@mphilbrick211 If I understand you correctly then, this label shall not be used when "branch becomes out-of-date"?

itsjeyd · 2023-06-07T08:04:59Z

Sounds good @arbrandes, thanks for the info.

@mattcarter An early heads-up that this will require a second look from Aurora once Adolfo is done with his review.

arbrandes · 2023-06-07T17:12:14Z

@ghassanmas, do you mind updating this to resolve conflicts? I'm planning to review it ASAP.

ghassanmas · 2023-06-07T17:39:26Z

@arbrandes I have resolved locally and pushed, but github isn't getting it, checkig the status of github, it seems it's down
https://www.githubstatus.com/

ghassanmas · 2023-06-07T18:32:49Z

It's ready now, the diff coverage is failing after rebase hence this #152 (comment)

src/data/services/lms/urls.js

arbrandes

One minor requested change, otherwise LGTM. I tested it with Tutor and it works.

src/data/services/lms/urls.js

arbrandes · 2023-06-12T17:55:39Z

Oh, and @ghassanmas, is there anything you can do to not reduce the project coverage?

Just change makes it possible if LMS_BASE_URL is updated by dynamic config to get the latest value.

when platformSettings.courseSearchUrl is relative which is returned from a call to api/learner_home/init, use LMS_BASE_URL as origin. This fixes uses the same pattern which already used for different url, this probably wasn't an issue for edx.org given platformSettings.courseSearchUrl value is abosulte at edx.org

ghassanmas · 2023-06-12T18:04:42Z

@arbrandes the diff was because of previous rebaese, now the lastest rebase it might be resolved, otherwise we see what we can do.

ghassanmas · 2023-06-12T18:15:40Z

@arbrandes it's now ready the code cov is back as it was hence #136 (comment) it was just edited and previous negative diff comment got delete.

arbrandes

Can't find anything else to object to, thanks @ghassanmas!

@muselesscreator, what do you think?

muselesscreator

seems reasonable to me :-)

openedx-webhooks · 2023-06-13T15:24:29Z

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

arbrandes · 2023-06-13T15:25:34Z

@ghassanmas, mind backporting to Palm?

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

ghassanmas force-pushed the dynamic-config-tutor-mfe branch from 68cd4d9 to 9791014 Compare April 18, 2023 01:14

ghassanmas mentioned this pull request Apr 18, 2023

New MFE: Learner Dashboard openedx/platform-roadmap#217

Closed

25 tasks

github-actions bot added the core contributor PR author is a Core Contributor (who may or may not have write access to this repo). label Apr 20, 2023

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

ghassanmas force-pushed the dynamic-config-tutor-mfe branch from 9791014 to 9f3af88 Compare April 20, 2023 22:29

ghassanmas force-pushed the dynamic-config-tutor-mfe branch from b41fbca to 010b2c8 Compare April 24, 2023 13:32

itsjeyd removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Apr 27, 2023

ghassanmas force-pushed the dynamic-config-tutor-mfe branch from 010b2c8 to 98f28d5 Compare April 29, 2023 11:34

ghassanmas force-pushed the dynamic-config-tutor-mfe branch 2 times, most recently from 91eb62d to ded6b25 Compare May 3, 2023 21:03

itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label May 9, 2023

ghassanmas force-pushed the dynamic-config-tutor-mfe branch from ded6b25 to fe0eed8 Compare May 9, 2023 10:31

ghassanmas force-pushed the dynamic-config-tutor-mfe branch from fe0eed8 to 75c6d58 Compare May 9, 2023 15:06

ghassanmas force-pushed the dynamic-config-tutor-mfe branch from 8d91a12 to a51953b Compare May 15, 2023 12:15

arbrandes mentioned this pull request May 31, 2023

Adolfo's reviews openedx/axim-engineering#696

Closed

arbrandes self-assigned this May 31, 2023

ghassanmas force-pushed the dynamic-config-tutor-mfe branch 3 times, most recently from 749239a to f17a376 Compare June 7, 2023 18:24

muselesscreator reviewed Jun 8, 2023

View reviewed changes

src/data/services/lms/urls.js Outdated Show resolved Hide resolved

arbrandes requested changes Jun 9, 2023

View reviewed changes

src/data/services/lms/urls.js Outdated Show resolved Hide resolved

src/data/services/lms/urls.js Outdated Show resolved Hide resolved

src/data/services/lms/urls.js Outdated Show resolved Hide resolved

arbrandes mentioned this pull request Jun 9, 2023

feat: Add a plugin for the Learner Dashboard MFE openedx/openedx-tutor-plugins#4

Merged

2 tasks

ghassanmas force-pushed the dynamic-config-tutor-mfe branch from f17a376 to a88cb0e Compare June 10, 2023 07:32

arbrandes requested changes Jun 12, 2023

View reviewed changes

src/data/services/lms/urls.js Outdated Show resolved Hide resolved

ghassanmas added 2 commits June 12, 2023 21:01

fix: force LMS url to reload when changed

48ec29d

Just change makes it possible if LMS_BASE_URL is updated by dynamic config to get the latest value.

ghassanmas force-pushed the dynamic-config-tutor-mfe branch from a88cb0e to ce88d29 Compare June 12, 2023 18:04

ghassanmas requested a review from arbrandes June 12, 2023 18:16

arbrandes approved these changes Jun 12, 2023

View reviewed changes

arbrandes requested a review from muselesscreator June 13, 2023 12:52

arbrandes approved these changes Jun 13, 2023

View reviewed changes

muselesscreator approved these changes Jun 13, 2023

View reviewed changes

arbrandes merged commit f92bd9c into openedx:master Jun 13, 2023

arbrandes mentioned this pull request Jun 14, 2023

[Palm] fix: force LMS url to reload when changed #163

Merged

sambapete pushed a commit to EDUlib/frontend-app-learner-dashboard that referenced this pull request Jun 14, 2023

fix: force LMS url to reload when changed (openedx#136)

091fbf0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: force LMS url to reload when changed #136

fix: force LMS url to reload when changed #136

ghassanmas commented Apr 18, 2023 •

edited

Loading

openedx-webhooks commented Apr 18, 2023

codecov bot commented Apr 18, 2023 •

edited

Loading

itsjeyd commented Apr 20, 2023

itsjeyd commented Apr 20, 2023

ghassanmas commented Apr 20, 2023

itsjeyd commented Apr 27, 2023

ghassanmas commented May 3, 2023

ghassanmas commented May 3, 2023

itsjeyd commented May 9, 2023

ghassanmas commented May 9, 2023

itsjeyd commented May 10, 2023

mphilbrick211 commented May 10, 2023

ghassanmas commented May 11, 2023

mphilbrick211 commented May 12, 2023

ghassanmas commented May 15, 2023

itsjeyd commented Jun 7, 2023

arbrandes commented Jun 7, 2023

ghassanmas commented Jun 7, 2023

ghassanmas commented Jun 7, 2023

arbrandes left a comment

arbrandes commented Jun 12, 2023

ghassanmas commented Jun 12, 2023 •

edited

Loading

ghassanmas commented Jun 12, 2023

arbrandes left a comment

muselesscreator left a comment

openedx-webhooks commented Jun 13, 2023

arbrandes commented Jun 13, 2023

fix: force LMS url to reload when changed #136

fix: force LMS url to reload when changed #136

Conversation

ghassanmas commented Apr 18, 2023 • edited Loading

What does this change do:

Testing:

More context

openedx-webhooks commented Apr 18, 2023

codecov bot commented Apr 18, 2023 • edited Loading

Codecov Report

itsjeyd commented Apr 20, 2023

itsjeyd commented Apr 20, 2023

ghassanmas commented Apr 20, 2023

itsjeyd commented Apr 27, 2023

ghassanmas commented May 3, 2023

ghassanmas commented May 3, 2023

itsjeyd commented May 9, 2023

ghassanmas commented May 9, 2023

itsjeyd commented May 10, 2023

mphilbrick211 commented May 10, 2023

ghassanmas commented May 11, 2023

mphilbrick211 commented May 12, 2023

ghassanmas commented May 15, 2023

itsjeyd commented Jun 7, 2023

arbrandes commented Jun 7, 2023

ghassanmas commented Jun 7, 2023

ghassanmas commented Jun 7, 2023

arbrandes left a comment

Choose a reason for hiding this comment

arbrandes commented Jun 12, 2023

ghassanmas commented Jun 12, 2023 • edited Loading

ghassanmas commented Jun 12, 2023

arbrandes left a comment

Choose a reason for hiding this comment

muselesscreator left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Jun 13, 2023

arbrandes commented Jun 13, 2023

ghassanmas commented Apr 18, 2023 •

edited

Loading

codecov bot commented Apr 18, 2023 •

edited

Loading

ghassanmas commented Jun 12, 2023 •

edited

Loading