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

fi: support uuid -> username mapping #290

Merged
merged 16 commits into from
May 20, 2023
Merged

fi: support uuid -> username mapping #290

merged 16 commits into from
May 20, 2023

Conversation

vnugent
Copy link
Contributor

@vnugent vnugent commented May 11, 2023

Issue #202

Incrementally move user profile data to our own db. The goal of this PR is to support syncing username from Auth0 to our db to enable mapping from user uuid to username.

  • support user profile create/update
  • support uuid -> username mapping
  • restrict setting email field on profile creation
  • introduce "builder service account" with elevated permissions to read user profile
  • retire get username hack
  • fix unit tests
  • run job to create users collection in prod

@vnugent vnugent removed the WIP in progress label May 19, 2023
@vnugent vnugent marked this pull request as ready for review May 19, 2023 20:50
@vnugent vnugent requested a review from zichongkao May 19, 2023 20:57
@vnugent vnugent changed the title fix: wip support uuid -> username mapping fi: support uuid -> username mapping May 19, 2023
@vnugent
Copy link
Contributor Author

vnugent commented May 19, 2023

@zichongkao any idea why end-to-end tests are failing?

@zichongkao
Copy link
Contributor

@zichongkao any idea why end-to-end tests are failing?

Basically in

const jwtSpy = jest.spyOn(jwt, 'verify')
we mock jwt.verify's return value and the return value has no scope field. I made the middleware robust to not having scope. But in future, I'm sure we'll have to mock scope in the mocked response so that we can give isBuilder status for testing out user profile stuff.

@zichongkao
Copy link
Contributor

I'll review the PR later this evening!

@vnugent
Copy link
Contributor Author

vnugent commented May 20, 2023

I pulled your fix down. API tests are still failing inside queryAPI()

  ● areas API › queries › retrieves an area and lists associated organizations

    expect(received).toBe(expected) // Object.is equality

    Expected: 200
    Received: 500

      80 |         userUuid
      81 |       })
    > 82 |       expect(response.statusCode).toBe(200)
         |                                   ^

The response object:

{
        status: 500,
        text: '{"errors":[{"message":"Context creation failed: connect ECONNREFUSED 127.0.0.1:80","extensions":{"code":"INTERNAL_SERVER_ERROR"}}]}\n',
        method: 'POST',
        path: '/'
      }

      at Object.<anonymous> (src/__tests__/areas.ts:83:15)

  ● are

@zichongkao
Copy link
Contributor

That's a bit surprising. They pass locally and on the automated checks. Is your .env in a good state? On my end, I don't see anything that is trying to connect to port 80.

@vnugent
Copy link
Contributor Author

vnugent commented May 20, 2023

That's a bit surprising. They pass locally and on the automated checks. Is your .env in a good state? On my end, I don't see anything that is trying to connect to port 80.

You're right. I have a .env.local that overrides some of the DB values. All green now.

Copy link
Contributor

@zichongkao zichongkao left a comment

Choose a reason for hiding this comment

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

Looks good in general! I can see how adding username resolvers will help reduce frontend API calls in many places.

Many of the questions I raised are for my own learning.

The one product decision that might be worth discussing is whether to make usernames mandatory during sign-up. I'm leaning toward yes, especially since they are mutable so users can revisit them later if they don't like them. Otherwise the power of defaults is strong and users may just leave the auto-generated ones.

expect(response.statusCode).toBe(200)
const areaResult = response.body.data.area
expect(areaResult.uuid).toBe(muuidToString(wa.metadata.area_id))
expect(areaResult.organizations.length).toBe(1)
expect(areaResult.organizations).toHaveLength(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this!

src/db/UserSchema.ts Show resolved Hide resolved
}, {
_id: false,
timestamps: {
updatedAt: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly confused too, since below we type this as a "NotUpdatableField", so how come we allow an updatedAt time?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I understand now why we allow updatedAt. But still wondering why we list as a "NotUpdatableField".

Copy link
Contributor Author

@vnugent vnugent May 20, 2023

Choose a reason for hiding this comment

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

updatedAt: true: Mongo will set the value for you whenever there's an update
*Edit: in this case I simply want Mongo to create the updatedAt field for me. We want to set username update timestamp ourselves, but don't want API users to do it.

src/db/UserSchema.ts Show resolved Hide resolved
src/model/UserDataSource.ts Show resolved Hide resolved
src/model/UserDataSource.ts Show resolved Hide resolved
src/model/UserDataSource.ts Outdated Show resolved Hide resolved
@vnugent vnugent merged commit c8d8b0b into develop May 20, 2023
1 check passed
@vnugent vnugent deleted the fix-202 branch May 20, 2023 19:57
@OpenBeta OpenBeta deleted a comment from asythlos Oct 27, 2023
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.

None yet

2 participants