Refactor!: app field settings

[chrismclarke]

PR Checklist

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

Breaking Changes

• Fields starting _ are now treated more strictly as protected. Templates using values of set_field starting with an underscore, or task_group completed field tracking will show warnings when setting such fields (later will throw error)

• App config can no longer overwrite default fields for _feedback_selected_text, _feedback_sidebar_open, or any other fields defined in APP_FIELDS (e.g. server_sync_latest). Most of these fields don't make sense to change at runtime, and often are handled inconsistently (e.g. initial startup may populate default value and not update on change) so instead they have been hardcoded as the default values.

Description

• Create new list of PROTECTED_FIELDS with consistent naming across all deployments

• Remove direct access to localStorage by services which may inadvertently bypass storage naming prefixes, so that all storage writes go through localStorageService instead

• Remove FIELD_PREFIX as a runtime app config option. This again hasn't been changed by deployments, doesn't make sense to configure at runtime, and is implemented inconsistently (some services lookup the value, others keep the hardcoded rp-contact-field prefix). This value is instead just hardcoded within the localstorage service.

• Add methods setProtected and getProtected to localStorageService to allow writing fields that start with an underscore in a type-safe way and only explicitly (rejected when using a standard setString operation)

• Code tidying to remove various localStorageService methods not in use (e.g. setBoolean) and commented out code

• Add basic localstorage spec tests

• Apply changes from feat: store first app launch value in field #2301 to set APP_FIRST_LAUNCH field. This builds on top of, and therefore closes feat: store first app launch value in field #2301

Review Notes

The main change to check is that protected field values are preserved, e.g. app theme, language, skin etc. The localstorage values on a deployment using this branch could be compared with the values on main. Ideally this should be done using a profile that has a reasonable amount of contact fields populated (e.g. one that has also set task group values and has an older app first launch date to verify)

Additionally should confirm db sync and user import still working as expected to populate fields from one user to another (requires running db and api locally)

Dev Notes

The main goal of the PR is to make all interaction with localStorage pass through the localStorageService, which in turn handles things like the field name prefix and protected field names. This cleans up quite a few messier areas of the codebase, particularly multiple subscriptions for appFields which don't ever change and inconsistent use of the rp-contact-field prefix.

I had originally planned to make the process of writing to protected fields more strict (block unless explicit), however realised in particular task.service writes to arbitrary fields as defined from templates, and so could break (now a deprecation). This should still be migrated in the future as user profile restore would automatically ignore these fields anyway, but should probably include a QC check in parser and service to ensure protected fields not used

Scope for breaking changes should be minimal, as (somewhat inefficiently) most protected fields are repeatedly set every app launch (e.g. deployment_name, app_version, content_version), or during service subscriptions (e.g. app_theme, app_language).

The only case where changes will be lost would be for fields that were previously accidentally written without prefix (e.g. app_events_summary), however in this case both the value is repopulated from other indexeddb entries and the original would never have been synced to the server (checks for prefix) - in fact now it will be synced and will include the first_launch child property within rp-contact-field.app_events_summary.

Future TODOs

• Replace hardcoded FIELD_PREFIX for dev mode only better support testing multiple apps on localhost. Production shoudl remove prefix altogether as doesn't serve any real purpose. In order to do this we would need a migration system to allow a one-time write of all existing contact fields to new namespace, at which point it might make more sense to reconsider implementing as an in-memory system with delayed persistence to indexeddb.

• Consider refactoring localStorage service to store json objects that include data type

• Consider refactoring rest of appConfig fields to protected (e.g. selected_text_field, app_update_available_field)

• Consider simplifying templateFieldService to be more of a loose wrapper around localstorage service - if implemented with rxdb could expose value subscription methods

• Consider removing setJson and getJson methods

• Remove final references to rp-contact-field from test suites

• Consider refactoring the app-events.service to not persist a combined value to localstorage and instead just track a few protected field entries

• Refactor task.service to use protected fields instead of arbitrary setField operations and highlightedTaskField variable

Git Issues

Closes #

Screenshots/Videos

app_first_launch field now populated

[chrismclarke]

Error when calling getField() without a key

An error was being thrown on launch because the task service calls getField() with an undefined value for the key.

I've pushed a commit which addresses this commit: 35d6674 Alternatively, perhaps the check should sit at the level of the LocalStorageService.get() method.

Interesting, what was the error. When I just try in my local browser LocalStorage.getItem(undefined) it returns null without any error. No objection to the catch in any case, more just curious.

Protected field values

A spot check on the protected field values suggests that they are preserved between master and this feature branch, and can be updated as expected (e.g. setting theme and skin).

The _app_first_launch field appears to be set correctly

Thanks for checking

User sync/import

As stated in the "Author Notes" section of #2270, protected fields are currently excluded from the user import action, so we wouldn't expect to see these values populated from the server when performing the user sync action. This is potentially something to consider as a follow up.

Yeah I think probably worth making explicit somehow which fields are/aren't synced (and not sure whether better as opt-in or opt-out), but best handled as follow-up

Syncing other fields and dynamic data works as expected, with one caveat: when importing user data from the server, the value of the field _server_sync_latest is saved to a field called server_sync_latest, without the _ prefix. See demo below:

Nice catch, I can see the server.service manually populates the contact_field property before sending to the server but doesn't add the prefix like the storage service does. This is the only place in the code I can see PROTECTED_FIELDS accessed directly, although noticed I had also forgotten to include the _ prefix when removing a protected field so have instead added a protected field name accessor to avoid similar issues in the future 847c980

Field prefix

I have never understood the prefix "rp-contact-field", I assume this is inherited from RapidPro? Is now the time to rethink this prefix? EDIT: I see this addressed in the first item of the Future TODOs checklist

Yes all inherited from early focus on rapidpro which doesn't make a huge amount of sense anymore (although useful mostly as a prefix to disambiguate multiple apps on localhost). Would be nice to remove but we would first need a migration system to copy old values to new system

[chrismclarke]

Thanks for looking over @jfmcquade - I've addressed the 1 issue you picked up on, let me know if you think any other changes are required. I've moved follow-up issues to #2322, feel free to update anything else you think of

[esmeetewinkel]

I can confirm rp-contact-field._app_first_launch is populated correctly, and also skin / theme survive when changing from master to the PR branch.

I noticed that there's a new contact field app_events_summary. Should that start with _?

Could someone resolve the conflicts between this PR branch and master?

[chrismclarke]

I can confirm rp-contact-field._app_first_launch is populated correctly, and also skin / theme survive when changing from master to the PR branch.

I noticed that there's a new contact field app_events_summary. Should that start with _?

Mentioned briefly in the dev notes, this has previously been stored without the rp-contact-field prefix and so wasn't getting synced. Now it has the prefix but the legacy version will remain unless browser cache cleared. As for making it a protected _ variable - that's not possible at this time as the protected variables don't yet support nested json (mentioned as possible follow-up). So for now I think probably just leave as-is and deal with in the future

Could someone resolve the conflicts between this PR branch and master?
Done.

[jfmcquade]

Error when calling getField() without a key

An error was being thrown on launch because the task service calls getField() with an undefined value for the key.
I've pushed a commit which addresses this commit: 35d6674 Alternatively, perhaps the check should sit at the level of the LocalStorageService.get() method.

Interesting, what was the error. When I just try in my local browser LocalStorage.getItem(undefined) it returns null without any error. No objection to the catch in any case, more just curious.

Woops, forgot to attach the screenshot. The error is caused by passing undefined to the LocalStorageService.get() method.

Other than that, all looking good. Nice to have the follow-ups tracked in #2322