feat: allow runtieme configuration #253

dcoa · 2022-07-13T19:32:52Z

Description

This PR is part of the work to make it possible to configure the frontend applications at runtime (you can referer to this openedx/wg-frontend#103).

Changes

Update frontend-platform to version 2.5.0
Update the footer and header to avoid peer dependency errors.
Create a component Head, that uses Helmet library and integrates internationalization to change the MFE name in the title tag according to the language, and change the favicon in runtime.

Screenshot

Values come from runtime configuration

How to test

To allow runtime configuration set a new environment variables MFE_CONFIG_API_URL and APP_ID in the env file and add the api url ( To test this you can use the API from this feat: add mfe config api edx-platform#30473 or use an external tool to mock the API response).
The API should respond with a JSON with the config values, something like:

{
  "SITE_NAME": "Test Site",
  "LOGO_URL": "https://testimage.com/logo.svg",
  "LOGO_TRADEMARK_URL": "https://testimage.com/logo.svg",
  "LOGO_WHITE_URL": "https://testimage.com/logo.svg",
  "FAVICON_URL": "https://testimage.com/favicon.ico",
}

The initialize process should work normally.
Note: You can combine buildtime and runtime configuration

openedx-webhooks · 2022-07-13T19:32:55Z

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

natabene · 2022-07-20T20:03:20Z

@dcoa Thank you for your contribution. Is this ready for our review?

codecov · 2022-07-20T20:11:02Z

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (b26b9fb) compared to base (03fa143).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #253   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          108       110    +2     
  Lines         1260      1264    +4     
  Branches       248       248           
=========================================
+ Hits          1260      1264    +4

Impacted Files	Coverage Δ
src/App.jsx	`100.00% <ø> (ø)`
src/head/Head.jsx	`100.00% <100.00%> (ø)`
src/head/messages.js	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

dcoa · 2022-07-20T23:04:23Z

Hi @natabene , yes is ready.

schenedx · 2022-09-09T14:06:20Z

@dcoa I had a review on this PR. I would like to request the following:

Could you share the business reason behind this change? Is this purely translation related?
Could you pictures on this PR? A picture describing the area of change on the UI, and what the effect of it is?
Am I understanding correctly this repo is forked within your work environment, and you are just trying merge back the changes from your fork to help the main repo?

schenedx

Added some inline comments as well.

.env

schenedx · 2022-09-09T14:10:19Z

src/App.jsx

@@ -28,6 +29,7 @@ const App = () => (
        <Footer logo={process.env.LOGO_POWERED_BY_OPEN_EDX_URL_SVG} />
      </div>
    </Router>
+    <Head />


It's weird to me that Head element is lower than the Footer element. Can you help me understand the reason for this sequence?

I agree, I fix this

Why not just put this (without the head component, just directly in here) at the top of this component?

I use a component because there are specific related changes (title and favicon) and I thought was better to maintain a simple App component and because I'm using injectIntl (from @edx/frontend-platform/i18n) which wraps the component to make it possible to change the MFE name according to the language and that needs access the IntlProvider that is inside AppProvider

src/head/Head.test.jsx

dcoa · 2022-09-12T16:37:28Z

Hi @schenedx,

Could you share the business reason behind this change? Is this purely translation related?

The main idea form this PR is to allow gradebook to use the runtime configuration option when this is activated (now in frontend-platform check this PR openedx/frontend-platform#335).

Could you pictures on this PR? A picture describing the area of change on the UI, and what the effect of it is?

Yes, I added a screenshot of the area impacted by Head component the main idea is to maintain the actual UI.

The main idea of runtime configuration is to set or override any variable this includes SITE_NAME and FAVICON_URL, currenly, these variables are only read at build time https://github.com/openedx/frontend-app-gradebook/blob/master/public/index.html#L4 if I want to use runtime configuration and update the values those changes aren't applied.

@muselesscreator, understanding that, is necessary to apply these changes with code (accessing the method getConfig). After evaluate the different ways to do that, I chose react-helmet that has been tested and used in other MFEs like frontend-app-learning and fronted-app-auth to make changes in the title tag at runtime.

Am I understanding correctly this repo is forked within your work environment, and you are just trying to merge back the changes from your fork to help the main repo?

Yes, I tried to make a contribution to the main code.

Like the issue openedx/wg-frontend#103 describes I want to implement those changes in different apps (availables by default in tutor-mfe by now), that includes Gradebook and Account where was approved openedx/frontend-app-account#603

I will be attentive if you require more information 🤓

kdmccormick · 2022-10-12T17:23:44Z

@schenedx and @muselesscreator -- I plan to approve and merge this on Monday 10/17 unless you folks had any specific concerns.

MaferMazu · 2022-10-13T14:49:43Z

I tested it, and it works. Thanks for your explanation and the images @dcoa 🙌

kdmccormick

@dcoa would you mind rebasing this one last time before we merge it?

@schenedx , did you want to re-review this at all?

kdmccormick · 2022-10-13T14:51:57Z

.env

+APP_ID=
+MFE_CONFIG_API_URL=


Nit -- I recommend using single quotes, just for consistency with the rest of the file.

Suggested change

APP_ID=

MFE_CONFIG_API_URL=

APP_ID=''

MFE_CONFIG_API_URL=''

kdmccormick · 2022-10-13T14:52:11Z

.env.development

+APP_ID=
+MFE_CONFIG_API_URL=


Suggested change

APP_ID=

MFE_CONFIG_API_URL=

APP_ID=''

MFE_CONFIG_API_URL=''

@kdmccormick No. If @muselesscreator approved. I am good.
@muselesscreator When is a good time for us to merge and deploy this change?

dcoa · 2022-10-13T20:11:22Z

Hi @kdmccormick I already rebase it and add the single quotes.

#253 (comment)

openedx-webhooks · 2022-10-17T16:12:45Z

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

Allows frontend-app-gradebook to be configured at runtime using the LMS's new MFE Configuration API. Part of openedx/wg-frontend#103

dcoa requested a review from a team as a code owner July 13, 2022 19:32

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 13, 2022

dcoa mentioned this pull request Jul 13, 2022

MFE runtime configuration openedx/wg-frontend#103

Closed

6 tasks

kdmccormick mentioned this pull request Aug 4, 2022

feat: build an image that uses runtime configuration kdmccormick/tutor-mfe#1

Closed

schenedx previously requested changes Sep 9, 2022

View reviewed changes

dcoa requested a review from schenedx September 14, 2022 13:59

dcoa requested review from muselesscreator and schenedx and removed request for schenedx and muselesscreator October 12, 2022 12:27

muselesscreator approved these changes Oct 13, 2022

View reviewed changes

kdmccormick reviewed Oct 13, 2022

View reviewed changes

dcoa added 3 commits October 13, 2022 15:04

feat: allow runtieme configuration

5ed97f2

test: update App test and fix Head text typo

c016764

fix: add single quotes to env variables

b26b9fb

dcoa force-pushed the dcoa/runtime-favicon-title branch from c00d62b to b26b9fb Compare October 13, 2022 20:09

kdmccormick approved these changes Oct 13, 2022

View reviewed changes

ghassanmas mentioned this pull request Oct 16, 2022

MFE dynamic config overhangio/tutor-mfe#69

Merged

kdmccormick merged commit d2723e5 into openedx:master Oct 17, 2022

ghassanmas mentioned this pull request Oct 24, 2022

PR(s) Needs to be backported or created to Olive openedx/wg-build-test-release#205

Closed

arbrandes pushed a commit that referenced this pull request Nov 14, 2022

feat: allow runtime configuration (#253)

bccd87f

Allows frontend-app-gradebook to be configured at runtime using the LMS's new MFE Configuration API. Part of openedx/wg-frontend#103

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow runtieme configuration #253

feat: allow runtieme configuration #253

dcoa commented Jul 13, 2022 •

edited

Loading

openedx-webhooks commented Jul 13, 2022

natabene commented Jul 20, 2022

codecov bot commented Jul 20, 2022 •

edited

Loading

dcoa commented Jul 20, 2022

schenedx commented Sep 9, 2022

schenedx left a comment

schenedx Sep 9, 2022

dcoa Sep 12, 2022

muselesscreator Sep 12, 2022

dcoa Sep 12, 2022 •

edited

Loading

dcoa commented Sep 12, 2022

kdmccormick commented Oct 12, 2022

MaferMazu commented Oct 13, 2022

kdmccormick left a comment

kdmccormick Oct 13, 2022

kdmccormick Oct 13, 2022

schenedx Oct 13, 2022

dcoa commented Oct 13, 2022

openedx-webhooks commented Oct 17, 2022

-APP_ID=
-MFE_CONFIG_API_URL=
+APP_ID=''
+MFE_CONFIG_API_URL=''

feat: allow runtieme configuration #253

feat: allow runtieme configuration #253

Conversation

dcoa commented Jul 13, 2022 • edited Loading

openedx-webhooks commented Jul 13, 2022

natabene commented Jul 20, 2022

codecov bot commented Jul 20, 2022 • edited Loading

Codecov Report

dcoa commented Jul 20, 2022

schenedx commented Sep 9, 2022

schenedx left a comment

Choose a reason for hiding this comment

schenedx Sep 9, 2022

Choose a reason for hiding this comment

dcoa Sep 12, 2022

Choose a reason for hiding this comment

muselesscreator Sep 12, 2022

Choose a reason for hiding this comment

dcoa Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

dcoa commented Sep 12, 2022

kdmccormick commented Oct 12, 2022

MaferMazu commented Oct 13, 2022

kdmccormick left a comment

Choose a reason for hiding this comment

kdmccormick Oct 13, 2022

Choose a reason for hiding this comment

kdmccormick Oct 13, 2022

Choose a reason for hiding this comment

schenedx Oct 13, 2022

Choose a reason for hiding this comment

dcoa commented Oct 13, 2022

openedx-webhooks commented Oct 17, 2022

dcoa commented Jul 13, 2022 •

edited

Loading

codecov bot commented Jul 20, 2022 •

edited

Loading

dcoa Sep 12, 2022 •

edited

Loading