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

WAITP-1122: Update users import #4459

Merged
merged 3 commits into from
Apr 23, 2023
Merged

WAITP-1122: Update users import #4459

merged 3 commits into from
Apr 23, 2023

Conversation

tcaiger
Copy link
Contributor

@tcaiger tcaiger commented Apr 16, 2023

Issue #: WAITP-1122: Update users import (gender and verified)

Changes:

  • Include the ability to import with no gender OR have an 'unknown' option
  • Include the ability to import as 'verified'

Copy link
Collaborator

@rohan-bes rohan-bes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion to use @tupaia/types over re-defining the enum options for verified_email

employer: [hasContent],
position: [hasContent],
mobile_number: [],
password: [hasContent],
permission_group: [hasContent],
verified_email: [constructIsEmptyOr(constructIsOneOf(['unverified', 'verified', 'new_user']))],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
verified_email: [constructIsEmptyOr(constructIsOneOf(['unverified', 'verified', 'new_user']))],
import { UserAccountSchema } from '@tupaia/types';
...
verified_email: [constructIsEmptyOr(constructIsOneOf(UserAccountSchema.verified_email.enum))],

This is fairly new functionality, but now we have a package called @tupaia/types which we use to maintain a source of truth about our models. It's possible we could replace all these validators in central-server with ones that use ajv and @tupaia/types but for now I think we can at least rely on them for enums and the sort.

@IgorNadj I feel you've got a stronger grasp on using @tupaia/types that I do, does what I'm suggesting here seem appropriate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is great. I have added the type for verified email but the type for gender is just a string so I've left that one as is.

@tcaiger tcaiger requested a review from rohan-bes April 21, 2023 02:11
Copy link
Collaborator

@rohan-bes rohan-bes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks @tcaiger

merge in latest dev before testing
@avaek avaek merged commit 074e553 into dev Apr 23, 2023
@avaek avaek deleted the waitp-1122-users-import branch April 23, 2023 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants