Review: pr 2351 proposed changes

[chrismclarke]

PR Checklist

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

Description

Targets PR #2351 branch

• Add override hierarchy to appConfig service to make it easier to track overrides coming from different levels (defaults, deployment, skin and template)

• Remove override revert logic from skin service (now handled by app config)

• Remove additional template-app-config service to instead include effects within existing template-metadata service

• Update tests

Review Notes

I checked the feature_app_layout_footer and appeared to be working as demonstrated, but would be good to double check still functions as expected. I also checked the debug skin select and looks to be functioning as I would expect, but would be good to double check reverting defaults after skin change still works as expected (skin revert code removed from service but should still be handled by app config service)

Dev Notes

I purposefully haven't included a level for config changes triggered by actions, as I'm not sure this would be handled in the same way as other overrides or not. Deployment, skin and template-level overrides are all quite similar in the way that they have clearly defined lifespans (persistent, until skin change, until template change). It's not quite clear how setting config from an action would work (would it persist until next action changes? or until template/skin change?). In any case I think we only need to deal with that once we have a clear use-case.

Git Issues

Closes #

Screenshots/Videos

If useful, provide screenshot or capture to highlight main changes

[jfmcquade]

Great, thanks @chrismclarke. I think I was anticipating a solution along these lines to be much more complicated, but it was good to discuss it on the call and this implementation is actually much easier to understand than the current handling in my opinion – the AppConfigService.configOverrides array is a reasonably intuitive representation of the current overrides (and it can be logged out for debugging). And nice that the app config service handles the override order in a way that those services which set the respective overrides do not need to be aware of.

The functionality does seem to be as expected when testing with the feature example sheets (in combination with different skins), and the tests are passing, nice to have some more (I added one more). I will merge this and continue to make your suggested changes on the targeted PR.

[chrismclarke]

Great, thanks @chrismclarke. I think I was anticipating a solution along these lines to be much more complicated, but it was good to discuss it on the call and this implementation is actually much easier to understand than the current handling in my opinion – the AppConfigService.configOverrides array is a reasonably intuitive representation of the current overrides (and it can be logged out for debugging). And nice that the app config service handles the override order in a way that those services which set the respective overrides do not need to be aware of.

The functionality does seem to be as expected when testing with the feature example sheets (in combination with different skins), and the tests are passing, nice to have some more (I added one more). I will merge this and continue to make your suggested changes on the targeted PR.

Great, yeah I was pleasantly surprised too, the discussion on the call really helped a lot to zoom out for a sec and think about doing things in a different way. In hindsight it makes a lot more sense when trying to keep track of multiple layers of overrides to just store each of those layers separately. Instead of having to worry about rolling back a specific layer and tracking partial states the focus now can just be on recreating from scratch as needed.

Also always a good sign when you can remove a net 30 lines of code whilst still adding 20 lines of tests... now just need to do that with the rest of the codebase 😅