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.

Sign up for GitHub

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

Make mergeBasket Conditional More Robust #1048

Merged
merged 7 commits into from
Mar 13, 2023
Merged

Make mergeBasket Conditional More Robust #1048

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
97b589e
Update merge logic
bendvc Mar 9, 2023
56b61c3
Update CHANGELOG.md
bendvc Mar 9, 2023
d2c11a0
Lint
bendvc Mar 9, 2023
57958a8
PR feedback
bendvc Mar 10, 2023
d6c475f
Rename isNewlyRegisters to isNew for simplicity
bendvc Mar 10, 2023
f6e73f4
Lint
bendvc Mar 10, 2023
0b1e37a
Merge branch 'develop' into merge-basket-logic
bendvc Mar 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/template-retail-react-app/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
## v2.8.0-dev (Mar 03, 2023)
- Make `mergeBasket` conditional more robust [#1048](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/1048)

## v2.7.0 (Mar 03, 2023)
- Add Page Designer ImageTile component [#967](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/967)
- Add Page Designer ImageWithText component [#991](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/991)
Expand Down
12 changes: 12 additions & 0 deletions packages/template-retail-react-app/app/commerce-api/hooks/useCustomer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import {useCommerceAPI, CustomerContext} from '../contexts'

const AuthTypes = Object.freeze({GUEST: 'guest', REGISTERED: 'registered'})

const REGISTRATION_GRACE_PERIOD = 2 * 1000 // 2 seconds in milliseconds
Copy link
Contributor

Choose a reason for hiding this comment

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

"Grace period" is confusing - is this just a workaround for "just registered and logged in"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this is a work around for determining if a user has just registered. Unfortunately there isn't a property on the customer object that tells you that directly. There is a host of data type values, but they unfortunately cant be used to derive whether or not a user is new.

I was struggling with that name, I'm open to suggestions on other names, or solutions.


export default function useCustomer() {
const api = useCommerceAPI()
const {customer, setCustomer} = useContext(CustomerContext)
Expand Down Expand Up @@ -39,6 +41,16 @@ export default function useCustomer() {
return customer?.authType === AuthTypes.GUEST
},

/**
* Returns if this customer is newly registered.
*/
get isNewlyRegistered() {
if (!customer || customer.authType !== 'registered') return false
const lastLoginTimeDate = new Date(Date.parse(customer.lastLoginTime))
const creationDate = new Date(Date.parse(customer.creationDate))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const lastLoginTimeDate = new Date(Date.parse(customer.lastLoginTime))
const creationDate = new Date(Date.parse(customer.creationDate))
const lastLoginTimeDate = Date.parse(customer.lastLoginTime)
const creationDate = Date.parse(customer.creationDate)

Don't convert from number to Date if all you're doing is calculating a number. 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True. I'll fix this outside of this suggestion as my OCD is telling me to rename those variables as they would no longer be Date but rather TimeStamps.

return lastLoginTimeDate - creationDate < REGISTRATION_GRACE_PERIOD
},

/** Returns the customer's saved addresses with the 'preferred' address in the first index */
get addresses() {
if (!customer?.addresses) {
Expand Down
11 changes: 9 additions & 2 deletions packages/template-retail-react-app/app/commerce-api/hooks/useShopper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,16 @@ const useShopper = (opts = {}) => {
}
}, [customer.authType, basket.loaded])

// Call merge basket whenever user type changes from guest to registered
// Call merge basket.
useEffect(() => {
if (customer.authType === 'registered') {
// Only call merge when there are items in the guest basket and you are
// a returning customer.
const shouldMerge =
customer.authType === 'registered' &&
!customer.isNewlyRegistered &&
basket.itemCount > 0

if (shouldMerge) {
basket.mergeBasket()
}
}, [customer.authType])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}, [customer.authType])
}, [customer.authType, customer.isNewlyRegistered, basket.itemCount])

Are these needed? (React hooks are still confusing!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. If we added those values it just means that the hooks didUpdate logic would be run when those addition values change, which isn't necessarily what we want to be doing. I know the hook with the guard in it is ugly, there are alternatives but they are equally unsightly.

Expand Down