fix: sync LMS_BASE_URL for bookmark API if changed

[ghassanmas]

This change makes it possible to use the latest LMS_BASE_API
if it was changed because of dynamic config API, which is the
default case of tutor.

This changes closes

• Bookmark tool is not saving bookmarks wg-build-test-release#270

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

Testing

Tutor

• Create a plugin tutor file nano "$(tutor plugins printroot)/learning.py"

from tutor import hooks
from tutormfe.plugin import MFE_APPS

@MFE_APPS.add()
def _add_my_mfe(mfes):
  mfes["learning"] = {
      "repository": "https://github.com/ghassanmas/frontend-app-learning",
      "port": 2000,
      "version": "fix-btr-270", # optional, will default to the Open edX current tag.
  }
  return mfes

tutor plugins enable leanring
tutor images buile mfe
tutor local start mfe -d 

Devstack

• It should still be working as expected assuming config api is enabled and LMS_BASE url is fetched only through the dynamic config api

Last step

• Try reproduce the bug by following the test report @ Bookmark tool is not saving bookmarks wg-build-test-release#270

Todos

• Test thse changes on tutor local latest plam branch
• Ensure the test don't fail locally
• Ensure the test don't fail on CI

[openedx-webhooks]

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.

[ghassanmas]

Note to self: add to backport list

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

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (79b65da) 87.26% compared to head (5fa2020) 87.26%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1120   +/-   ##
=======================================
  Coverage   87.26%   87.26%           
=======================================
  Files         253      253           
  Lines        4391     4391           
  Branches     1109     1109           
=======================================
  Hits         3832     3832           
  Misses        545      545           
  Partials       14       14           

Impacted Files	Coverage Δ
src/courseware/course/bookmark/data/api.js	100.00% <100.00%> (ø)

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

[regisb]

I'm not a reviewer but this looks great to me.
Can you please explain to me as if I was a five year old what is going on here? My interpretation is that because the variable was not exported, webpack evaluated it at build time? Is that correct?

[ghassanmas]

Can you please explain to me as if I was a five year old what is going on here? My interpretation is that because the variable was not exported, webpack evaluated it at build time? Is that correct?

Humm... not exactly, I export it, so I can import it in the test file, so the function would be tested, not related to webpack.

As for the fix, it's related to JS hoisting and getConfig API timeline, if we define a variale from getConfig() as a const at top of the file this will get executed before the dynamic config API update the configuration value, thus const variable be still pointing to the old configuration, while if we define the variable as a getFunX then when we retrive the variable it will always get the last config that was updated by getConfig.

To my understanding the timeline of things:

1. JS get execaute statements at top files, i.e. global function, variable...etc Note it will not a call a function
   1. We might here add a const variable const PI = getConfig().PI assume by default PI = 3.14 getConfig() will be called
   2. we might here define a function to get PI const getPI = () => getConfig().PI. getConfig will not be called
2. Now then APP get init
   1. The dynamic config is called, it would update the value of PI which is retrived by getConfig
      1. Assume this happens by calling a function called setConfig({PI: 3.14159265 })
   2. Now later on our app logic we need to use the value of PI
      1. if we retrive it by by using PI varaible then we will get 3.14 from step 1.i The value of PI would equal value of getConfig().PI at the time JS parsed the file.
      2. if we retrive it using getPI we would get 3.14159265 from step 1.ii 1. The returned value will equal the value of getConfig().PI at the time before setConfig was last called hence step 2.i.a

So my chages here in a nutshell is jsut that I force the App to use LMS_BASE_URL in the context bookmarks component simliar to case 1.ii as opposite to how it was before case 1.i

[regisb]

Sounds good, thanks for the clear explanations. I think that we should backport this change both to Olive and Palm.

[ghassanmas]

@regisb yeah for Palm I intend to do it once it's merged. For olive, do you anticipate we are going to do olive.5 release, or is that not necessary for doing a backport given that tutor would use the tag, unless you would override the version value for learning in tutor 15.x to user olive.master instead of olive.4

[regisb]

For olive, do you anticipate we are going to do olive.5 release

No, I don't think we should make olive.5. But it would be good to have the patch out there.

[arbrandes]

Makes sense to me. Thanks, @ghassanmas!

[itsjeyd]

Thank you for this contribution @ghassanmas!

Looks like you already got a 👍, so I'm marking the changes as ready for merge.

[openedx-webhooks]

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

[duraiganesh0]

Hi all,
Did anyone try to apply this on an olive version of tutor? I'm getting this error

File "/home/ubuntu/.local/lib/python3.8/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/ubuntu/.local/lib/python3.8/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/ubuntu/.local/lib/python3.8/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
  File "/home/ubuntu/.local/lib/python3.8/site-packages/click/decorators.py", line 45, in new_func
    return f(get_current_context().obj, *args, **kwargs)
  File "/home/ubuntu/.local/lib/python3.8/site-packages/tutor/commands/plugins.py", line 140, in enable
    plugins.load(plugin)
  File "/home/ubuntu/.local/lib/python3.8/site-packages/tutor/plugins/__init__.py", line 93, in load
    hooks.Actions.PLUGIN_LOADED(name).do()
  File "/home/ubuntu/.local/lib/python3.8/site-packages/tutor/core/hooks/actions.py", line 122, in do
    self.do_from_context(None, *args, **kwargs)
  File "/home/ubuntu/.local/lib/python3.8/site-packages/tutor/core/hooks/actions.py", line 141, in do_from_context
    callback.do(
  File "/home/ubuntu/.local/lib/python3.8/site-packages/tutor/core/hooks/actions.py", line 35, in do
    self.func(*args, **kwargs)
  File "/home/ubuntu/.local/lib/python3.8/site-packages/tutor/plugins/v1.py", line 55, in load
    spec.loader.exec_module(module)
  File "<frozen importlib._bootstrap_external>", line 848, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/ubuntu/.local/share/tutor-plugins/learning.py", line 2, in <module>
    from tutormfe.plugin import MFE_APPS
ImportError: cannot import name 'MFE_APPS' from 'tutormfe.plugin' (/home/ubuntu/.local/lib/python3.8/site-packages/tutormfe/plugin.py)