-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: user import action #2270
Feat: user import action #2270
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The core code for the action looks good. Already having the endpoint makes it reasonably straightforward – are there any implications of what Ian and Chris M were proposing about making the API server more secure (I can't find the mattermost thread atm)?
I approve of the tidying, although I haven't deeply engaged with the changes to the api package.
See demo of a slightly different user flow to that in the PR description that tests the new functionality via the debug_user_actions template. This was through running the postgres db locally, not via docker:
Screen.Recording.2024-04-04.at.16.12.26.mov
I think the deploy preview action is failing because following #2235, the relevant action is not generating a populated firebase.json
as it should. I haven't yet investigated more thoroughly.
Re the TODOs
- Yes, I can look into adding some kind of feedback (e.g. pop-up) to indicate the success/failure of the action
- It's probably sensible to discuss the other points in our next meeting and create any follow-up issues. I have added to the agenda
Do we need to think about any additional security or compliance issues? I think we already discussed this but raising again in case it needs further discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
30 character limit on inputs
Happy for the character limit on the select-text
and text-box
components to be removed. I guess it was an attempt at keeping the input within one line's width, but seems artificial.
I don't seem to have postgres db running locally (or at least yarn workspace api start
is not happy) but given that Johnny tested that I'm happy for this to be merged. I can test post merge on a deployment preview.
I've added #2277, could be good to discuss on future call |
PR Checklist
Description
Main Changes
user : import : [id]
action which looks up an existing user by id and replicated contact fields to local user.Additional Changes
@fields._app_user_id
(also available via calc(_app_user_id) property)rp-contact-field
select-text
andtext-box
components.Review Notes
Testing is a bit tricky as it requires a development server running locally as well as the code. Ideally everything would run from deployment preview using the debug repo, however it looks like the action is not currently working as intended (to review). For testing locally:
Assuming postgres db already running locally then just api can be run via:
Or if running as docker container will need to update the endpoint in
src\app\shared\services\server\interceptors.ts
to behttp://localhost/api
and runI also had a few issues with a previous docker implementation running locally (and conflicts with postgres running at same port), and ended up having to wipe local docker containers using
It also requires the debug_user_actions sheet, either running locally or synced to debug deployment.
See example in video below
Author Notes
Fields starting with
_
are omitted, such as_app_skin
,_app_theme
,_app_version
. Whilst some of these might be useful to populate (e.g. importing also sets app skin), the default behaviour is to ignore to avoid confusion with services that set these fields programmatically. It may be useful to review protected field handling in the future to make clear what can/can't be overwritten.I've added a debug_user_actions sheet, not sure whether this should be considered a debug or example sheet, feel free to update or adapt as useful
It's not totally clear why there was a 30 character limit on inputs (implemented 2 years ago by external consultants), the web spec itself limits at 2^19 (524288) for 512kb data equivalence, however the code now uses a fallback of
-1
which should have the same effect. If we do really want a hardcoded limit suggest at least 36 characters to avoid having to hardcode override whenever using a user id inputDev Notes
The changes made to server code are more the result of trying to debug why import was failing (eventually realised it was because the input id was getting cut off), I don't think any of the changes need to be deployed to the server however have retained in the PR as the changes will still be beneficial when next working on server development.
I wasn't sure where best to place the code as
server.service
db-sync.service
anduserMeta.service
all have references to the local user and server. I opted foruserMeta.service
and may consider further refactoring theserver.service
in the future to move user-specific code to auser.service
(and review what the userMeta table is actually used for... a lot of quite old code). So a little rough around the edges but hopefully should be reasonablyFuture TODOs
There is currently no visual confirmation if import successful or failed. In the short term it might be useful to have some sort of small popup/alert/prompt hardcoded into the method (happy to take on @jfmcquade ?)
longer term it might be worth considering a syntax for reading an action's response back into a template and provide updates for longer-running operations (to discuss?/follow-up issue?).
The current
emit
actions don't really fit very well with modern patterns where services register actions to a central registry. It would be good to refactoremit: server_sync
likely to aserver : sync
action along with other emit. Should create minor follow-up issue (with sensible deprecation policy to rewrite actions at parser level and not just remove)Looking through the codebase there are a lot of references to
rp-contact-field
despite the fact that deployments have been able to define their own field prefixes. Recommend either removing the option for deployments to define, or tidying up code to respect the deployment config (and migrating existing fields as required). Should create follow-up issueIt may also be worth reconsidering how contact fields are populated. Local storage is a bit inefficient for setting long lists one-by-one (particularly with the intermediate processing handled by the templateField service), so may be worth considering migration to rxjs and following the same pattern of storing data in-memory and writing to disk in the background.
Currently all requests to HTTP server endpoints are hardcoded without type-safety. This is not a huge issue as there aren't many endpoints, but given that the server produces an openAPI spec with list of available endpoints, data types etc. it might make sense to consider replacing angular HTTP server with something like https://www.npmjs.com/package/openapi-fetch - possible follow-up issue?
Git Issues
Closes #2245
Screenshots/Videos
Screenity.video.-.Apr.2.2024.webm