SeedCompany / cord-api-v3

Bible translation project management API
MIT License
18 stars 4 forks source link

add financial approver #3195

Closed andrewmurraydavid closed 5 months ago

andrewmurraydavid commented 5 months ago

1100-CF Multiplication -Controller Notifications

Description

Ready for review checklist

Use [N/A] if the item is not applicable to this PR or remove the item

  • [x] Change the task url above to the actual Monday task
  • [N/A] Add/update tests if needed
  • [N/A] Add reviewers to this PR
atGit2021 commented 5 months ago

@CarsonF , I still have work to do on the edgedb repo side of things, but I have a question so pushing up what I have so far. First question is whether I should commit and push the following file, dbschema/migrations/00006-m1fbgja.edgeql, which was generated from the new migration create that we did today.
Second question, is regarding the neo4j repo side, bot the set and list are now finding the financial approver and loading the result dto, but both methods give me an error in graphql when returning, which says, message": "Cannot return null for non-nullable field ProjectTypeFinancialApprover.user.",. I'm not sure why it's saying null when I see a dto object being returned (at least from the repo).

CarsonF commented 5 months ago

First question is whether I should commit and push the following file, dbschema/migrations/00006-m1fbgja.edgeql, which was generated from the new migration create that we did today.

Yes. It'll need to be rebased. I'd like to do it to double check the workflow.

Second question, is regarding the neo4j repo side, bot the set and list are now finding the financial approver and loading the result dto, but both methods give me an error in graphql when returning, which says, message": "Cannot return null for non-nullable field ProjectTypeFinancialApprover.user.",. I'm not sure why it's saying null when I see a dto object being returned (at least from the repo).

You have

@Field(() => User)
readonly user: LinkTo<'User'> & Pick<UnsecuredDto<User>, 'email'>;

This decorator communicates to GQL that this type has a user: User! field. But the TS type here does not reflect the full User type GQL is expecting. For example, user.fullName would fail/"return null" since that object given doesn't have it. We don't want every reference to a user type to have to figure the "full user shape" which is why we use LinkTo as just a intermediate reference. These are 1️⃣ not decorated with @Field because they are just an internal reference. If we want to expose them we 2️⃣ create a field resolver. This allows exposes the field to GQL, is called lazily (only when requested in GQL operation), and is implemented with a loader for that type. https://github.com/SeedCompany/cord-api-v3/blob/0d8fe49605294d9c0258d307dd0cdf960d4ecac4/src/components/authentication/register.resolver.ts#L41-L49

atGit2021 commented 5 months ago

@CarsonF, I messed up my edgedb branch environment and lost the new edge branch called financial approver that we created yesterday so all those errors are back. Since I really don't understand or remember all the steps you walked me through yesterday to get the new FinancialApprover type working, would you be able to fix and rebase that into this PR so I can pull it down?

CarsonF commented 5 months ago

We need to get this moving. Closing in favor of #3206