search

openedx / frontend-app-gradebook

Instructor grade book tool
GNU Affero General Public License v3.0
11 stars 87 forks source link

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

Closed kdmccormick closed 1 year ago

kdmccormick commented 1 year ago

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 https://github.com/overhangio/tutor-mfe/pull/69, which implements https://github.com/openedx/build-test-release-wg/issues/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

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:

FYI: @openedx/content-aurora

codecov[bot] commented 1 year ago

Codecov Report

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

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

:exclamation: 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 ```diff @@ 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](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/286?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx) | Coverage Δ | | |---|---|---| | [src/index.jsx](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/286/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-c3JjL2luZGV4LmpzeA==) | `60.00% <0.00%> (-40.00%)` | :arrow_down: | | [src/components/GradebookHeader/index.jsx](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/286/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-c3JjL2NvbXBvbmVudHMvR3JhZGVib29rSGVhZGVyL2luZGV4LmpzeA==) | `100.00% <100.00%> (ø)` | | | [src/data/services/lms/urls.js](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/286/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-c3JjL2RhdGEvc2VydmljZXMvbG1zL3VybHMuanM=) | `100.00% <100.00%> (ø)` | | | [src/data/store.js](https://codecov.io/gh/openedx/frontend-app-gradebook/pull/286/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-c3JjL2RhdGEvc3RvcmUuanM=) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx)

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

kdmccormick commented 1 year ago

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

kdmccormick commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

kdmccormick commented 1 year ago

I pushed https://github.com/openedx/frontend-app-gradebook/pull/286/commits/c3b16d026b726691caf86ea7b3791032312663a0 , and coverage is now 100%.