Refactor: app config runtime

[chrismclarke]

PR Checklist

• PR title descriptive (can be used in release notes)

Description

Further work to make core app runtime independent to specific deployment config

Main Changes

• Update frontend code to read appConfig from runtime service instead of hardcoded environment.
• Remove legacy hardcoded environment variables
• Tidy appConfigService code
• Deprecate use of appConfig$ observable and add support for using an appConfig signal instead

Additional Changes

• Change the way app_config is stored in deployments to only retain the diff of what is changed from default config. This should make it easier to interpret the parsed config, and avoid unnecessary side-effects when loading a config that is identical to the default.
• Add utilities and spec tests for object utils
• Move code to handle app config route default change effects from skinService to appConfig service, and simplify code.

Review Notes

Inspect changes to config handling by setting a deployment yarn workflow deployment set debug and inspecting the generated activeDeployment.json. The output will only contain app_config variables specifically overridden in the config.ts file (previously would include all properties)

Changes to deployment-specific route handling are automatically tested as the default app-routing.module.ts no longer includes the overrides, instead initialising as part of the appConfig service. So the app loading the correct home screen indicates that the config update is being processed as expected. Could still test within another deployment that has skin-specific route changes.

Can assume environment variables now safe to remove as only references in the src code path is environment.production

Dev Notes

Following this PR the only deployment-specific code that appears to be imported into the codebase is the list of sheets and assets (via src\app\data\app-data.ts). So will still require another follow-up pr to remove, but once that is done then the app runtime and deployment config should be fully decoupled.

To keep authoring compatibility but minimise the amount of data stored in deployment app config, a new empty config tree is produced that just has placeholder values for all nested paths e.g.

{"app_config":{"NOTIFICATION_DEFAULTS":{"time":{}}}}

That way an author can still specify just app_config.NOTIFICATION_DEFAULTS.time.hour. When compiling additional methods are used to then delete any unnecessary config (empty jsons)

As deployment now only stores partial config, the full config is generated as part of the appConfig service. This also handles side-effects from config updates (currently just updating routes to include home and fallback templates) - previously this was handled by the skinService and had more complex logic to check for fine-grained changes, now it's a bit more simpler as it just always reconstructs the full set of routes on config change.

The appConfig service now also exposed the config as a signal to make it easier to develop components in the future that use computed values. I've marked the current observables as deprecated but haven't refactored any other component code to use for now.

For Follow-up

this PR

• Refactor src\app\data\app-data.ts to load sheet contents from json instead of hardcoded assets. This should be done as part of appDataService initialisation, and all direct references to the hardcoded data removed.
• Refactor src\app\shared\utils\utils.ts object methods to shared and add spec tests (lots of duplicate code)
• Add similar diff handling to only store changed deployment config values for top-level config
• Refactor code that accesses appConfig$ subject to use signals instead
• Refactor emit actions to be declared from services (e.g. set_skin)
• Address [FEATURE] Fix tests and incorporate into CI pipeline #2196 as higher priority

#2412

• refactor config services to same folder and make clear difference between deployment and app configs
• refactor app_config service to use signals instead of observables (possibly start using both?)
• Add new API service to make it easier to opt-in/out of backend API functionality. Include type-safe HTTP client based on openapi export, and proxy all requests via /api endpoint, e.g. /api/app_users. This should also be able to replace interceptors
• Refactor app config generators to create from partial (deep merge)

#2409

• Refactor APP_CONFIG references to use service instead of hardcoded (service can init from deployment token)
• Remove rest of references to deploymentConfig from environment
• Remove deprecated environment variables (e.g. rapidpro)

#2404

• Refactor all existing code to access runtime config through service instead of environment
• Tidy up environment variables (review which are still required or not)

Additional future considerations (from #2404)

• Consider splitting out app_config variables to separate json/file (e.g. separate app and feature/service config files)
• Consider removing config function stringification (only runtime config needs load from json and doesn't include any properties with functions)
• Review work from Refactor/deployment scripts #2086 and try to revive as useful
• Attempt to rewrite config as ts/js export that can access named utils. Enable to be fully compiled in advance or evaluated at runtime (e.g. import js string and use eval with util variable contexts).
• Update config to export as generator functions (module.exports)
• Config validation through zod or similar
• Create local and gh action workflows that allow serve/build deployments without angular dev server (i.e. use pre-built app core and load deployment config, sheets and assets as app_data only)

Git Issues

Closes #

Screenshots/Videos

If useful, provide screenshot or capture to highlight main changes

[chrismclarke]

@jfmcquade - This can be marked as ready-for-review once #2412 merged
Sorry there's been so many config update PRs, quite a lot of inter-connected changes to slowly unravel but hopefully should only be a couple high-priority items from the rolling TODOs and then just lower-priority follow-ups

[jfmcquade]

Looking good, nice to have the tests.

I've left one comment inline that is blocking if my understanding is correct.

We do have skin configured for the debug deployment that has skin-specific route changes (.idems_app/deployments/debug/skins/debug.ts), so I've tested this functionality with positive results:

skin-based.route.change.mov

[jfmcquade]

nit(blocking): Since a skin is a minimal app config that specifies overrides on top of the deployment config (see e.g. .idems_app/deployments/debug/skins/debug.ts), I think this won't produce the expected behaviour when called by SkinService.setSkin(). If I'm interpreting this correctly, non-default values that are specified in the deployment config but not specified in the skin will be ignored.

The skin should inherit all values from the deployment config before overriding a subset, so rather than merging the overrides with the default app config, we should be merging the overrides with the app config that is a result of merging the default app config with the deployment app config – i.e. we should be merging overrides with the result of merging getDefaultAppConfig() with this.deploymentService.config.app_config (or equivalently(?), first merging overrides with this.deploymentService.config.app_config and then merging that result with getDefaultAppConfig())

[chrismclarke]

Ah good point, yeah I was a bit confused on this when working on it. I think some of the confusion was we were using an updateAppConfig variable but the update function was still pulling the default config and merging updates on top (so not so much updating the current config but setting a new one from a partial config)

So if I understand skins correctly, they are a layer on top of the compiled deployment config. In the case of applying successive skins, the 1st skin is simply a patch on the default deployment, but the 2nd would need to revert changes from the first and then apply. Or more simply each skin will want to independently set a complete value based on merge with default config.

I'll update the skin service to handle the partial merging part so that the appConfig methods can stay more straight-forward

[chrismclarke]

So it took quite a bit more work than expected but I think I've just about got there in the end. I realised one of the issues with the way app skins were currently being applied was that each time the updated skin config was merged on top of the original deployment config, meaning that if any app config had been updated from anywhere else during that time the changes would be lost.

In practice this wasn't a huge issue as the only other service that updated directly was the app header (and only during testing/debug of scroll collapse feature). Although definitely all the different levels of config changes were quite confusing and difficult to unpick.

In the end I found myself needing to write test suites to properly understand the behaviour and iterate on the code. The way the skin system now (should) work is that each time a skin is applied it also generates a revert override which can be used to undo the skin-specific changes. Then if the skin is changed again the new skin update can be merged with the previous revert to leave an overall config that would be the same had the second skin been applied originally, but allowing for any independent config changes that may have occurred in that time.

If you get a chance could you take a look at the test cases and see if they make sense to you. I've manually checked toggling between the two skins on the debug_skin_select template which works, although might be good to check against any production deployment that also uses (I'm not sure which do)

I also separately realised that various recent refactors had broken some of the service spec tests (which we don't run as part of CI (outstanding #2196), so I also have fixed these

[chrismclarke]

Unfortunately the updates are spread across a number of commits, but all the meaningful changes should be within the skin.service.ts and skin.service.spec.ts files

[jfmcquade]

See my latest review comment for a continuation of this discussion, as well as #2423 (comment)

[jfmcquade]

Approving review is a WIP, I'm trying to figure something out

[jfmcquade]

Your general changes to the skin service look good to me, and your description of skins matches my understanding. I don't think I would have thought to store the revertOverride, but I can see how that's necessary to handle skin changes while allowing the app config to be updated not via the skin service.

However an additional change was needed to handle changing skins correctly. Have a look at f72b682 and see if you approve of my changes.

In terms of production deployments that use skins, I don't believe there are any that expose skin switching to the user. For the PLH Teens app, the skins feature was effectively used to A/B test two different skins, and once the authors had settled on the skin they preferred, this one was made the default for all users. In order to catch those users who had previously installed the app and used the alternate skin, a launch action is currently used to force all users to use the intended skin on launch, see row 3 of this template (changing the default skin at the deployment config level is not enough to force a skin change for those users who have previously enabled a different skin, providing that different skin is still in the "available" skins array). Anyway, that deployment does have a skin_select, not exposed to the user, and testing this shows the desired behaviour: manually changing the skin works as expected, and the skin is forced to "default" on app relaunch (via the app launch action).

The new tests look good, I've added a few more in 27712b0

[jfmcquade]

Without this, any deployment config-level overrides to the default config that are not explicitly specified or overwritten in the skin are lost on setSkin. E.g., when testing the debug_skin_select template on the debug deployment, switching skins causes the footer to be removed.

[chrismclarke]

Ah, good catch. Diving more deeply it doesn't appear to be the skin service making changes that lose this configuration but the appConfigService that doesn't initialise with it properly. I'll add a specific test to the skin service to verify the case you've presented and fix the appConfigService accordingly (should probably add a few tests to the appConfigService too, having tests are helping massively to debug the skin service now, thanks for adding the updated tests too)

[chrismclarke]

See updates in f1f6fcb, if happy then I think this might be finally good to go! :D

Thanks again for helping review this one, can appreciate it's been a bit of a tangled mess

[chrismclarke]

I've also added a PR to update the debug config for improved testing:
IDEMSInternational/app-debug-content#84

[jfmcquade]

Looks good, the latest changes make sense and make the app-config service easier to understand. I think the complexity reflects the complexity of our system with the three different levels of overrides to the app config values, but I think these changes overall definitely make things clearer, especially with the tests.

[chrismclarke]

Looks good, the latest changes make sense and make the app-config service easier to understand. I think the complexity reflects the complexity of our system with the three different levels of overrides to the app config values, but I think these changes overall definitely make things clearer, especially with the tests.

Agreed, would definitely be good to document at some point the difference between deployment/app config and skins (and example use cases). For now at least I think I feel mostly happy with how most of the skin-specific complexity is kept within the skins service (and has test coverage of the more complex behaviours), thanks for helping get it over the line!

I've made an internal note to come back to the follow-up TODO items and will likely just move them across to the next PR as it's raised (like with previous) instead of writing individual issues for all right now