Closed chrismclarke closed 1 month ago
Visit the preview URL for this PR (updated for commit bfcfebf):
https://plh-teens-app1--pr2320-refactor-app-fields-9l1c2a8v.web.app
(expires Thu, 06 Jun 2024 13:14:26 GMT)
🔥 via Firebase Hosting GitHub Action 🌎
Sign: e4c0bab6b08dd290fbf002fd6e07987fa4b5fce1
Error when calling
getField()
without a keyAn error was being thrown on launch because the task service calls
getField()
with anundefined
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 calledserver_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 847c980b2dd382125d4900fa5863d270e36ef1ea
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
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 https://github.com/IDEMSInternational/parenting-app-ui/issues/2322, feel free to update anything else you think of
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.
PR Checklist
Breaking Changes
Fields starting
_
are now treated more strictly as protected. Templates using values ofset_field
starting with an underscore, ortask_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 inAPP_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 deploymentsRemove direct access to
localStorage
by services which may inadvertently bypass storage naming prefixes, so that all storage writes go throughlocalStorageService
insteadRemove
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 hardcodedrp-contact-field
prefix). This value is instead just hardcoded within the localstorage service.Add methods
setProtected
andgetProtected
to localStorageService to allow writing fields that start with an underscore in a type-safe way and only explicitly (rejected when using a standardsetString
operation)Code tidying to remove various
localStorageService
methods not in use (e.g.setBoolean
) and commented out codeAdd basic localstorage spec tests
Apply changes from #2301 to set
APP_FIRST_LAUNCH
field. This builds on top of, and therefore closes #2301Review 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 therp-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 usedScope 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 withinrp-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
andgetJson
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 arbitrarysetField
operations andhighlightedTaskField
variableGit Issues
Closes #
Screenshots/Videos
app_first_launch field now populated![image](https://github.com/IDEMSInternational/parenting-app-ui/assets/10515065/5d22da56-65c1-4702-9a20-85f3789f2d9a)