fix: use getConfig in order to support runtime configuration #286

kdmccormick · 2022-11-28T00:40:35Z

Description

Before, gradebook was reading config from process.env
directly, which locked the app into using only
static (build-time) configuration. In order to
enable dynamic (runtime) configuration, we update
gradebook to use frontend-platform's standard
configuration interface: mergeConfig() and getConfig().

Bump version from 1.5.0 to 1.6.0.
(I would normally just do a patch release for a fix, but the version
was hasn't been bumped for a while, so adding in full runtime
configuration support seemed like it warranted a proper
minor version bump.)

References

This is a fix to support of overhangio/tutor-mfe#69, which implements openedx/wg-build-test-release#235, which is necessary for the Dec 9 (!) Olive release.

I also have a draft PR to backport this to Olive.master queued up. Once this PR merges, we can rebase and then merge the backport.

Developer Checklist

Test suites passing
- ~~Reviewers: I'm looking for help on how to resolve the one failing test case -- see my comment below.~~ Thanks Ghassan!
Codecov passing
Documentation and test plan updated, if applicable (N/A)
Received code-owner approving review
Bumped version number package.json

Testing Instructions

Making sure it won't break anything:: Just start up Gradebook locally with Devstack or Tutor and do a general smoke test.

Making sure it actually fixes runtime configuration: Follow @ghassanmas's tutor-mfe PR test instructions, but change MFE_GRADEBOOK_MFE_APP["repository"] to "https://github.com/kdmccormick/frontend-app-gradebook".

Reviewer Checklist

Collectively, these should be completed by reviewers of this PR:

I've done a visual code review
I've tested the new functionality

FYI: @openedx/content-aurora

Before, gradebook was reading config from `process.env` directly, which locked the app into using only static (build-time) configuration. In order to enable dynamic (runtime) configuration, we update gradebook to use frontend-platform's standard configuration interface: `mergeConfig()` and `getConfig()`. Bump version from 1.5.0 to 1.6.0. (I would normally just do a patch release for a fix, but the version was hasn't been bumped for a while, so adding in full runtime configuration support seemed like it warranted a proper minor version bump.)

kdmccormick · 2022-11-28T01:28:17Z

src/data/store.test.js

      describe('if no SEGMENT_KEY', () => {
-        const key = configuration.SEGMENT_KEY;
+        const key = getConfig().SEGMENT_KEY;
        beforeEach(() => {
-          configuration.SEGMENT_KEY = false;
+          getConfig().SEGMENT_KEY = false;
        });
        it('exports thunk and logger middleware, composed and applied with dev tools', () => {
          expect(createStore().middleware).toEqual(
            composeWithDevTools(applyMiddleware(thunkMiddleware, createLogger())),
          );
        });
        afterEach(() => {
-          configuration.SEGMENT_KEY = key;
+          getConfig().SEGMENT_KEY = key;
        });


I'm looking for help with this case. I'm not sure how to change the result of getConfig() for just one test case.

Currently it fails with:

FAIL src/data/store.test.js ● store aggregator module › exported store › middleware › if no SEGMENT_KEY › exports thunk and logger middleware, composed and applied with dev tools expect(received).toEqual(expected) // deep equality - Expected - 0 + Received + 4 Object { "withDevTools": Object { "applied": Array [ "thunkMiddleware", "logger", + Object { + "map": "EVENTS MAP", + "model": "Segment", + }, ], }, } 66 | }); 67 | it('exports thunk and logger middleware, composed and applied with dev tools', () => { > 68 | expect(createStore().middleware).toEqual( | ^ 69 | composeWithDevTools(applyMiddleware(thunkMiddleware, createLogger())), 70 | ); 71 | }); at Object.<anonymous> (src/data/store.test.js:68:44)

I presume this is because getConfig().SEGMENT_KEY = false isn't actually setting SEGMENT_KEY to false within the test body.

codecov · 2022-11-28T13:46:55Z

Codecov Report

Base: 100.00% // Head: 99.84% // Decreases project coverage by -0.15% ⚠️

Coverage data is based on head (7ba6d04) compared to base (50bf7d2).
Patch coverage: 60.00% of modified lines in pull request are covered.

❗ Current head 7ba6d04 differs from pull request most recent head c3b16d0. Consider uploading reports for the commit c3b16d0 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##            master     #286      +/-   ##
===========================================
- Coverage   100.00%   99.84%   -0.16%     
===========================================
  Files          110      109       -1     
  Lines         1264     1264              
  Branches       248      248              
===========================================
- Hits          1264     1262       -2     
- Misses           0        2       +2

Impacted Files	Coverage Δ
src/index.jsx	`60.00% <0.00%> (-40.00%)`	⬇️
src/components/GradebookHeader/index.jsx	`100.00% <100.00%> (ø)`
src/data/services/lms/urls.js	`100.00% <100.00%> (ø)`
src/data/store.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.

kdmccormick · 2022-11-28T13:53:01Z

Oops, looks like I need to add a test that hits the initialize(...) call in index.jsx. Looking at that now.

kdmccormick · 2022-11-28T13:57:05Z

@arbrandes since you're reviewing, let me know if you think of any way to cover the initialize(...) call in index.jsx with a test, or whether we should just ignore coverage on those lines. I checked codecov for profile, account, and learning, and none of those apps cover initialize(...) with a test.

arbrandes

Looks good! Left a couple of comments. Will test it next.

src/data/store.test.js

arbrandes · 2022-11-28T14:02:32Z

@kdmccormick

any way to cover the initialize(...) call in index.jsx

This is what frontend-app-ora-grading does. Looks like a reasonable test, except for the fact having each-and-every MFE do the same thing feels kinda redundant, just for coverage. 🤷🏼‍♂️

kdmccormick · 2022-11-28T14:18:03Z

@arbrandes Hold on, gradebook is testing the same thing: https://github.com/kdmccormick/frontend-app-gradebook/blob/master/src/index.test.jsx#L49-L57. And that test is definitely running (I know, because I had to fix it :)

Now to figure out why that isn't enough to make Coverage happy...

arbrandes · 2022-11-28T14:30:22Z

I tested the change, and AFAICT it works just fine.

Add a heavily-mocked test to make Coverage happy. Based on a similar test from: https://github.com/edx/frontend-app-ora-grading/blob/open-release/olive.master/src/index.test.jsx#L76-L86 I guess what we are testing here is that `initialize` and `mergeConfig` are being called with the correct arguments. I'm not convinced this test is valuable, but 100% coverage is required on this repo, so here it is.

kdmccormick · 2022-11-28T15:33:10Z

I pushed c3b16d0 , and coverage is now 100%.

kdmccormick

@arbrandes Ready for another round of review! I have two questions for you: this one, and the one posted below.

kdmccormick · 2022-11-28T15:39:19Z

src/index.jsx

+        BASE_URL: process.env.BASE_URL,
+        LMS_BASE_URL: process.env.LMS_BASE_URL,
+        LOGIN_URL: process.env.LOGIN_URL,
+        LOGOUT_URL: process.env.LOGOUT_URL,
+        CSRF_TOKEN_API_PATH: process.env.CSRF_TOKEN_API_PATH,
+        REFRESH_ACCESS_TOKEN_ENDPOINT: process.env.REFRESH_ACCESS_TOKEN_ENDPOINT,
+        DATA_API_BASE_URL: process.env.DATA_API_BASE_URL,
+        SECURE_COOKIES: process.env.NODE_ENV !== 'development',
+        SEGMENT_KEY: process.env.SEGMENT_KEY,
+        ACCESS_TOKEN_COOKIE_NAME: process.env.ACCESS_TOKEN_COOKIE_NAME,


I have included the config settings that were previously in config/index.js. I did not pull in everything from .env, which contains a lot of unused settings.

I grepped for process.env and found two remaining usages:

A few checks for process.env.NODE_ENV == 'development'|'production'. These feel like anti-patterns, but I assume it is OK to leave them as they were, since runtime config won't be overriding NODE_ENV.

<Footer logo={process.env.LOGO_POWERED_BY_OPEN_EDX_URL_SVG} /> -- This one confuses me, because AFAICT no other MFE has such a setting. Do other MFEs use the "Powered by Open edX" logo? If so, do they configure it some other way? I didn't see it in the default brand package. Anyway, I left this as-is, because tests failed when I tried to put it behind the getConfig() interface. Let me know if this seems like an issue Adolfo.

The footer thing does indeed seem like an issue, but not a blocker here.

+1 for not including all the .env settings.

+1 for leaving process.env.NODE_ENV usages.

arbrandes

LGTM!

…#286) Before, gradebook was reading config from `process.env` directly, which locked the app into using only static (build-time) configuration. In order to enable dynamic (runtime) configuration, we update gradebook to use frontend-platform's standard configuration interface: `mergeConfig()` and `getConfig()`. Bumps version from 1.5.0 to 1.6.0. (I would normally just do a patch release for a fix, but the version was hasn't been bumped for a while, so adding in full runtime configuration support seemed like it warranted a proper minor version bump.) Co-authored-by: Ghassan Maslamani <ghassan.maslamani@gmail.com>

…287) Before, gradebook was reading config from `process.env` directly, which locked the app into using only static (build-time) configuration. In order to enable dynamic (runtime) configuration, we update gradebook to use frontend-platform's standard configuration interface: `mergeConfig()` and `getConfig()`. Bumps version from 1.5.0 to 1.6.0. (I would normally just do a patch release for a fix, but the version was hasn't been bumped for a while, so adding in full runtime configuration support seemed like it warranted a proper minor version bump.) Co-authored-by: Ghassan Maslamani <ghassan.maslamani@gmail.com>

kdmccormick force-pushed the master branch from 79315d0 to fe39051 Compare November 28, 2022 00:42

kdmccormick mentioned this pull request Nov 28, 2022

MFE dynamic config overhangio/tutor-mfe#69

Merged

kdmccormick force-pushed the master branch from fe39051 to a6755e2 Compare November 28, 2022 01:26

kdmccormick commented Nov 28, 2022

View reviewed changes

kdmccormick marked this pull request as ready for review November 28, 2022 01:28

kdmccormick requested a review from a team as a code owner November 28, 2022 01:28

kdmccormick mentioned this pull request Nov 28, 2022

[🫒 backport] fix: use getConfig in order to support runtime configuration #287

Merged

test: fix failing test for using platform config

7ba6d04

kdmccormick mentioned this pull request Nov 28, 2022

test: fix failing test for using platform config kdmccormick/frontend-app-gradebook#1

Merged

6 tasks

arbrandes self-requested a review November 28, 2022 13:48

arbrandes reviewed Nov 28, 2022

View reviewed changes

src/data/store.test.js Show resolved Hide resolved

src/data/store.test.js Show resolved Hide resolved

arbrandes mentioned this pull request Nov 28, 2022

Reviews for sprint 2022-11-25 openedx/axim-engineering#539

Closed

36 tasks

kdmccormick commented Nov 28, 2022

View reviewed changes

kdmccormick requested a review from arbrandes November 28, 2022 15:41

arbrandes approved these changes Nov 28, 2022

View reviewed changes

kdmccormick merged commit 4f43e65 into openedx:master Nov 28, 2022

ghassanmas mentioned this pull request Nov 30, 2022

feat: ensure lms api synced with latest value in config #289

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use getConfig in order to support runtime configuration #286

fix: use getConfig in order to support runtime configuration #286

kdmccormick commented Nov 28, 2022 •

edited

Loading

kdmccormick Nov 28, 2022

kdmccormick Nov 28, 2022

codecov bot commented Nov 28, 2022 •

edited

Loading

kdmccormick commented Nov 28, 2022

kdmccormick commented Nov 28, 2022

arbrandes left a comment

arbrandes commented Nov 28, 2022 •

edited

Loading

kdmccormick commented Nov 28, 2022

arbrandes commented Nov 28, 2022

kdmccormick commented Nov 28, 2022

kdmccormick left a comment •

edited

Loading

kdmccormick Nov 28, 2022

arbrandes Nov 28, 2022

arbrandes left a comment

fix: use getConfig in order to support runtime configuration #286

fix: use getConfig in order to support runtime configuration #286

Conversation

kdmccormick commented Nov 28, 2022 • edited Loading

Description

References

Developer Checklist

Testing Instructions

Reviewer Checklist

kdmccormick Nov 28, 2022

Choose a reason for hiding this comment

kdmccormick Nov 28, 2022

Choose a reason for hiding this comment

codecov bot commented Nov 28, 2022 • edited Loading

Codecov Report

kdmccormick commented Nov 28, 2022

kdmccormick commented Nov 28, 2022

arbrandes left a comment

Choose a reason for hiding this comment

arbrandes commented Nov 28, 2022 • edited Loading

kdmccormick commented Nov 28, 2022

arbrandes commented Nov 28, 2022

kdmccormick commented Nov 28, 2022

kdmccormick left a comment • edited Loading

Choose a reason for hiding this comment

kdmccormick Nov 28, 2022

Choose a reason for hiding this comment

arbrandes Nov 28, 2022

Choose a reason for hiding this comment

arbrandes left a comment

Choose a reason for hiding this comment

kdmccormick commented Nov 28, 2022 •

edited

Loading

codecov bot commented Nov 28, 2022 •

edited

Loading

arbrandes commented Nov 28, 2022 •

edited

Loading

kdmccormick left a comment •

edited

Loading