chrismclarke
commented
2 months ago Initially, I wrote the logic to use the template field service to set the field variable, see https://github.com/IDEMSInternational/parenting-app-ui/commit/ef7220f19a7e56816cebd5b0bf7835f18147fed2. However that approach hardcodes the field name ("_app_first_launch") into the app-events service, without having it declared in the app config (i.e. as appConfig.APP_FIELDS.APP_FIRST_LAUNCH). Therefore it seemed sensible to use local storage service to set the field string directly, matching the pattern used to set most of the other protected variables throughout the app. However I don't think this pattern is ideal, as we're setting a field without using the field service and it requires an additional subscription to the app config service in order to get the field name.
This system of setting protected fields could probably be rationalised across the code at some point. It may be that it's sensible to store the names of these protected fields a level up, in the deployment config defined in deployment.model.ts, but not in the app_config object, as it doesn't seem necessary to update these values at runtime. Then the values could be consumed by services without needing a subscription to the app config service. If these values were declared without the FIELD_PREFIX prefix, then the fields could be set via the templateFieldService.setField() method, which appends the prefix itself. It may be that the current pattern of bypassing the template field service is intentional in the case of protected fields in order to differentiate them from user-set fields, in which case perhaps there should be some protections on templateFieldService.setField() to prevent setting a protected field directly with this method.
I think given that some protected fields may to be accessed before any other services init (e.g. knowing if it's the first launch to handle data sync/migrate or similar ops) we have pretty good reason to move all of these outside of runtime config. Not to mention how suddenly changing at runtime does nothing to migrate the legacy values. The same can be said for the general localstorage prefix, which again really shouldn't be swapped at runtime but also probably should be more explicitly used in core app config to at least be able to distinguish app variables set by different deployments when running locally.
As a first step in tidying I think I would recommend:
- move the
APP_FIELDSout ofappConfig.tsand instead just hardcode into the localstorage service (as this is where they are used). Likely rename toPROTECTED_FIELDSand do not export - move the
FIELD_PREFIXto a deploymentConfig variable that can be read from environment - add a method to the localstorageservice to
setProtectedField, with support just for those listed. This should also include the prefix as required. - update pr to set app first open field from the main
app.component.tsto run only on first launch and after all other services initialised. Can include a check in case does not exist to just pull fromapp-event.service- in the future we can hopefully set this independently of service - check codebase for instances of
localstorage.set(and try to refactor to use localstorageservice (this is a sync service so no need to worry to much about initialisation at least) - Consider moving all prefix handling from the templateField service just to the localstorage service (I can't think of a good reason why templating needs to be prefixed but not fields set outside of templating system, maybe that was our way of preserving protected field names but I don't think it's a very useful distinction to make). This might need a quick audit to check for any fields that are being set without prefix
Let me know if you think this approach sounds sensible, and whether you want me to give a pass at a PR or if you're happy to attempt a quick refactor.
PR Checklist
Description
Whilst we already track the timestamp of a user's first launch of the app, this value is not synced tot he database. This PR stores the value of the app's first launch in a field, enabling it to be synced to the user database for analysis.
The field name is
_app_first_launch.Dev notes
Initially, I wrote the logic to use the template field service to set the field variable, see https://github.com/IDEMSInternational/parenting-app-ui/pull/2301/commits/ef7220f19a7e56816cebd5b0bf7835f18147fed2. However that approach hardcodes the field name ("_app_first_launch") into the app-events service, without having it declared in the app config (i.e. as
appConfig.APP_FIELDS.APP_FIRST_LAUNCH). Therefore it seemed sensible to use local storage service to set the field string directly, matching the pattern used to set most of the other protected variables throughout the app. However I don't think this pattern is ideal, as we're setting a field without using the field service and it requires an additional subscription to the app config service in order to get the field name.This system of setting protected fields could probably be rationalised across the code at some point. It may be that it's sensible to store the names of these protected fields a level up, in the deployment config defined in
deployment.model.ts, but not in theapp_configobject, as it doesn't seem necessary to update these values at runtime. Then the values could be consumed by services without needing a subscription to the app config service. If these values were declared without theFIELD_PREFIXprefix, then the fields could be set via thetemplateFieldService.setField()method, which appends the prefix itself. It may be that the current pattern of bypassing the template field service is intentional in the case of protected fields in order to differentiate them from user-set fields, in which case perhaps there should be some protections ontemplateFieldService.setField()to prevent setting a protected field directly with this method.Git Issues
Closes #2295, assuming that having access to this value in the postgres database is deemed to be a satisfactory resolution of that issue
Screenshots/Videos