Skip to content

add financial approver #3195

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

Closed
wants to merge 6 commits into from

Conversation

andrewmurraydavid
Copy link
Contributor

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

  • 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 atGit2021 force-pushed the 1100-multiplication-notification branch from 05163f3 to 58766f5 Compare May 8, 2024 01:20
@atGit2021 atGit2021 self-assigned this May 8, 2024
@atGit2021
Copy link
Contributor

@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
Copy link
Member

CarsonF commented May 8, 2024

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.

@ResolveField(() => User, {
description: 'The newly created, logged-in user',
})
async user(
@Parent() { user }: RegisterOutput,
@Loader(UserLoader) users: LoaderOf<UserLoader>,
): Promise<User> {
return await users.load(user);
}

@atGit2021
Copy link
Contributor

@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
Copy link
Member

CarsonF commented May 9, 2024

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

@CarsonF CarsonF closed this May 9, 2024
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.

3 participants