fix(core): Resolve customer user via relation instead of email lookup#4468
fix(core): Resolve customer user via relation instead of email lookup#4468grolmus wants to merge 3 commits intovendurehq:masterfrom
Conversation
The customer entity resolver was falling back to looking up the User by email address when the user relation was not eagerly loaded. This caused guest customers to incorrectly appear as "verified" when a different registered customer shared the same email address. Now loads the customer's actual user relation via CustomerService.findOne() instead. Fixes vendurehq#4026 Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes fix an issue where a guest customer could incorrectly appear verified in the Admin Dashboard due to email-based user lookup returning an unrelated user account. The 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/e2e/customer.e2e-spec.ts`:
- Around line 706-718: The test is not verifying that the GraphQL mutation
succeeded before comparing customers; update the block that calls
shopClient.query for SET_CUSTOMER (and the returned SetCustomerForOrderMutation)
to first assert the mutation succeeded (e.g.,
expect(data.setCustomerForOrder).toBeDefined() or
expect(result.errors).toBeUndefined()), then read the customer id from the
strongly-typed response instead of using the cast (remove the "as any" and use
the SetCustomerForOrderMutation shape to access
data.setCustomerForOrder.customer.id), and only then compare guestCustomerId to
firstCustomer.id.
In `@packages/core/src/api/resolvers/entity/customer-entity.resolver.ts`:
- Around line 52-59: The fallback path in user(`@Ctx`() ctx: RequestContext,
`@Parent`() customer: Customer) calls this.customerService.findOne(ctx,
customer.id) without requesting relations, so loaded?.user stays undefined;
update the call to explicitly request the user relation (pass ['user'] as the
third parameter to this.customerService.findOne) so the returned Customer
includes the user relation and the method can return loaded.user correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1105030-3ebf-4d43-a49b-68d20a6d5475
📒 Files selected for processing (2)
packages/core/e2e/customer.e2e-spec.tspackages/core/src/api/resolvers/entity/customer-entity.resolver.ts
| const { setCustomerForOrder } = await shopClient.query< | ||
| SetCustomerForOrderMutation, | ||
| SetCustomerForOrderMutationVariables | ||
| >(SET_CUSTOMER, { | ||
| input: { | ||
| firstName: 'Guest', | ||
| lastName: 'Shared Email', | ||
| emailAddress: firstCustomer.emailAddress, | ||
| }, | ||
| }); | ||
|
|
||
| const guestCustomerId = (setCustomerForOrder as any).customer?.id; | ||
| if (guestCustomerId && guestCustomerId !== firstCustomer.id) { |
There was a problem hiding this comment.
Guard setCustomerForOrder success before asserting customer linkage.
This test can produce a false positive: if setCustomerForOrder returns an error result, guestCustomerId is undefined and the else branch still passes against firstCustomer. Assert success first and remove the as any cast.
Proposed fix
+ const orderErrorGuard: ErrorResultGuard<ActiveOrderCustomerFragment> = createErrorResultGuard(
+ input => !!input.lines,
+ );
const { setCustomerForOrder } = await shopClient.query<
SetCustomerForOrderMutation,
SetCustomerForOrderMutationVariables
>(SET_CUSTOMER, {
input: {
firstName: 'Guest',
lastName: 'Shared Email',
emailAddress: firstCustomer.emailAddress,
},
});
- const guestCustomerId = (setCustomerForOrder as any).customer?.id;
+ orderErrorGuard.assertSuccess(setCustomerForOrder);
+ const guestCustomerId = setCustomerForOrder.customer?.id;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { setCustomerForOrder } = await shopClient.query< | |
| SetCustomerForOrderMutation, | |
| SetCustomerForOrderMutationVariables | |
| >(SET_CUSTOMER, { | |
| input: { | |
| firstName: 'Guest', | |
| lastName: 'Shared Email', | |
| emailAddress: firstCustomer.emailAddress, | |
| }, | |
| }); | |
| const guestCustomerId = (setCustomerForOrder as any).customer?.id; | |
| if (guestCustomerId && guestCustomerId !== firstCustomer.id) { | |
| const orderErrorGuard: ErrorResultGuard<ActiveOrderCustomerFragment> = createErrorResultGuard( | |
| input => !!input.lines, | |
| ); | |
| const { setCustomerForOrder } = await shopClient.query< | |
| SetCustomerForOrderMutation, | |
| SetCustomerForOrderMutationVariables | |
| >(SET_CUSTOMER, { | |
| input: { | |
| firstName: 'Guest', | |
| lastName: 'Shared Email', | |
| emailAddress: firstCustomer.emailAddress, | |
| }, | |
| }); | |
| orderErrorGuard.assertSuccess(setCustomerForOrder); | |
| const guestCustomerId = setCustomerForOrder.customer?.id; | |
| if (guestCustomerId && guestCustomerId !== firstCustomer.id) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/e2e/customer.e2e-spec.ts` around lines 706 - 718, The test is
not verifying that the GraphQL mutation succeeded before comparing customers;
update the block that calls shopClient.query for SET_CUSTOMER (and the returned
SetCustomerForOrderMutation) to first assert the mutation succeeded (e.g.,
expect(data.setCustomerForOrder).toBeDefined() or
expect(result.errors).toBeUndefined()), then read the customer id from the
strongly-typed response instead of using the cast (remove the "as any" and use
the SetCustomerForOrderMutation shape to access
data.setCustomerForOrder.customer.id), and only then compare guestCustomerId to
firstCustomer.id.
gabriellbui
left a comment
gabriellbui
left a comment
There was a problem hiding this comment.
Review
The fix is conceptually correct — replacing email-based user lookup with relation-based lookup properly isolates the guest customer's user field from a registered customer sharing the same email. However there's a blocking issue in the implementation.
🔴 MUST CHANGE
packages/core/src/api/resolvers/entity/customer-entity.resolver.ts:52 — Missing ['user'] relation
// Current
const loaded = await this.customerService.findOne(ctx, customer.id);
return loaded?.user ?? null;
// Fix
const loaded = await this.customerService.findOne(ctx, customer.id, ['user']);
return loaded?.user ?? null;findOne signature is findOne(ctx, id, relations: RelationPaths<Customer> = []). Without explicitly passing ['user'], the relation is never loaded and loaded?.user is always undefined. The fallback path would return null for all customers whose user isn't eagerly pre-loaded on the parent entity — including registered customers. This could be a silent regression worse than the original bug. (CodeRabbit flagged this same issue.)
🟡 SHOULD CHANGE
packages/core/e2e/customer.e2e-spec.ts:718 — Test conditional logic allows the bug scenario to go untested
const guestCustomerId = (setCustomerForOrder as any).customer?.id;
if (guestCustomerId && guestCustomerId !== firstCustomer.id) {
// actual assertion
} else {
// fallback that doesn't test the bug
}If guestCustomerId is undefined (e.g. mutation returned an error type) or equals firstCustomer.id, the test silently passes without exercising the regression scenario. The as any cast should use the typed SetCustomerForOrderMutation shape, and the test should be structured to guarantee the guest customer is created and assert unconditionally.
🔵 CONSIDER
packages/core/e2e/customer.e2e-spec.ts:706 — Validate mutation result before use
Add a guard like expect(setCustomerForOrder.__typename).toBe('Order') before accessing .customer so an unexpected error type doesn't silently short-circuit the test through the else branch.
Once the ['user'] relation arg is added, this is a clean and well-scoped fix for a real bug. Good find on the root cause.
michaelbromley
left a comment
michaelbromley
left a comment
There was a problem hiding this comment.
Gabirel and CodeRabbit are spot on. This PR is dangerous because it introduces a new bug and has a test that does not test what it should be.
- Add missing ['user'] relation to customerService.findOne() call so the
user relation is actually loaded in the fallback path
- Move regression test to guest-checkout-strategy.e2e-spec.ts where the
TestGuestCheckoutStrategy can create the conflict scenario (default
strategy rejects guest checkout with registered emails)
- Rewrite test to use assertSuccess and unconditional assertions
- Rename misleading describe('Order taxes') to describe('GuestCheckoutStrategy')
Fixes vendurehq#4026
Addressed review feedbackThanks @gabriellbui and @michaelbromley for the thorough review. Both issues are now fixed: 1. Missing
|
3 participants
Summary
userresolver onCustomerEntityResolverwhich incorrectly fell back togetUserByEmailAddress()— this caused guest customers to appear "verified" when a different registered customer shared the same email addresscustomerService.findOne()which loads the actual user relation via the foreign keyUserServicedependency from the resolverFixes #4026
Test plan