🐛 Fix AppConfig survey_info backwards compat + add tests for dynamicConfig
#1115
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While looking at dynamicConfig.ts I realized `labelVars` should be optional - if the `labelTemplate` doesn't reference any vars, `labelVars` can be omitted.
The fix is in getConfig(), lines 257 and 265 - _backwardsCompatSurveyFill needed to be applied there. But while I was here, something else I noticed is that these functions mutate the config object parameter, which means they are not pure functions. For best practice, I changed them so they return a new object with the change rather than mutating their inputs. While I was doing that, I applied the AppConfig type definition we have declared in types/appConfigTypes.ts. And I cleaned up the log statements in readConfigFromServer()
JGreenlee
changed the title
survey_info backwards compatsurvey_info backwards compat
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## service_rewrite_2023 #1115 +/- ##
========================================================
+ Coverage 58.80% 65.26% +6.46%
========================================================
Files 26 26
Lines 1420 1428 +8
Branches 320 318 -2
========================================================
+ Hits 835 932 +97
+ Misses 585 496 -89
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
I didn't do anything for tests in this PR because it does not have any breaking changes for tests. But as I've realized, we don't even have tests yet for So, I'm marking this as a draft and will add tests before I mark it as ready again. |
To be more consistent with the rest of the codebase, as a rule of thumb we're having top-level functions use 'function' synax while one-liners or nested functions are arrow functions. Also, resetStoredConfig was renamed to _test_resetStoredConfig because that change is coming anyway in e-mission#1113 and this will make it easier to resolve merge conflicts
This will allow us to test how configs get stored (and then retrieved)
Instead of applying the backwards compat to fill 'survey_info' and 'name' by 2 function calls every time it needs to be done, let's have one function that does both
The dynamic config tests will want to actually test how the config is stored, so we don't want this to always fallback to some default 'fakeConfig'. Before the config gets stored we'll want to be able to tell if it's missing or not. We'll remove that fallback, and any test that *does* want to use that 'fakeConfig' can pass it as an argument to the mockBEMUserCache() function
Tests for the biggest functions: getConfig and initByUser Going to check coverage and see whether it's beneficial to test the smaller (helper) functions too
- test initByUser and getConfig using stub configs for 'nrel-commute' (which is old-style / does not have subgroups) and 'denver-casr' (which is new style with subgroups) - check that the 'joined' field gets filled in - isolate tests from one another by using await properly on async tests and by clearing storage in between test runs
When we split the string by '_', the resulting parts could be empty strings if the opcode is something like `nrelop_nrel-commute_`, or `nrelop__user1`. We should throw an 'invalid opcode' error in this case
JGreenlee
changed the title
survey_info backwards compatsurvey_info backwards compat + add tests for dynamicConfig
shankari
approved these changes
Jan 20, 2024
|
|
||
| mockBEMUserCache(); | ||
| mockBEMUserCache(fakeConfig); |
There was a problem hiding this comment.
This is not your change, but I would like to understand this better. Why do we pass in the config instead of calling putRWDocument
Comment on lines
+308
to
+309
| storedConfig = _backwardsCompatFill(config); | ||
| return storedConfig; |
There was a problem hiding this comment.
I know we didn't do this before, but a future enhancement might be to also save the config after backwards filling it. That would ensure that we could eventually remove the backwards compat fill (or turn it into a NOP) instead of having it grow over time.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes e-mission/e-mission-docs#1025
I reproduced the issue in the devapp by logging into
dev-emulator-study, clearing localStorage and user cache, then manually storing into the usercache an app config that was missingsurvey_info.Here is what that caused:
Immediately after applying 8c31b65 and waiting for autoreload, it was fixed: