Skip to content
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

Merged
merged 7 commits into from
Apr 4, 2024
Merged

Feat: user import action #2270

merged 7 commits into from
Apr 4, 2024

Conversation

chrismclarke
Copy link
Member

@chrismclarke chrismclarke commented Apr 3, 2024

PR Checklist

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

Description

Main Changes

  • Add a new user : import : [id] action which looks up an existing user by id and replicated contact fields to local user.
  • Create debug_user_actions sheet

Additional Changes

  • Provide access to app_user_id via @fields._app_user_id (also available via calc(_app_user_id) property)
  • Tidy templateField service to remove hardcoded references to rp-contact-field
  • Remove the hardcoded 30 character max length on select-text and text-box components.
  • Server code tidying

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:

yarn workspace api start;
yarn start;

Or if running as docker container will need to update the endpoint in src\app\shared\services\server\interceptors.ts to be http://localhost/api and run

yarn workspace api build;
yarn workspace server start;

I 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

yarn workspace server dangerously_wipe

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 input

Dev 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 and userMeta.service all have references to the local user and server. I opted for userMeta.service and may consider further refactoring the server.service in the future to move user-specific code to a user.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 reasonably

Future 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 refactor emit: server_sync likely to a server : 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 issue

  • It 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

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Apr 3, 2024
@chrismclarke chrismclarke added the test - preview Create a preview deployment of the PR label Apr 3, 2024
@esmeetewinkel esmeetewinkel self-requested a review April 4, 2024 15:42
@esmeetewinkel esmeetewinkel removed their assignment Apr 4, 2024
@jfmcquade jfmcquade self-requested a review April 4, 2024 15:45
Copy link
Collaborator

@jfmcquade jfmcquade left a 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

Copy link
Collaborator

@esmeetewinkel esmeetewinkel left a 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.

@chrismclarke
Copy link
Member Author

chrismclarke commented Apr 4, 2024

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've added #2277, could be good to discuss on future call
@esmeetewinkel - could we add to next agenda?

@chrismclarke chrismclarke removed the test - preview Create a preview deployment of the PR label Apr 4, 2024
@chrismclarke chrismclarke merged commit d12a2e0 into master Apr 4, 2024
6 of 8 checks passed
@chrismclarke chrismclarke deleted the feat/user-actions branch April 4, 2024 19:13
@chrismclarke chrismclarke mentioned this pull request May 8, 2024
1 task
@jfmcquade jfmcquade mentioned this pull request May 17, 2024
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Profile restore action
3 participants