Fix missing card payment options #16411
Conversation
Generated by 🚫 Danger |
|
|
|
|
||
| let site: Site? | ||
| if let storageSite { | ||
| site = storageSite | ||
| } else { | ||
| /// Non - fatal fallback to `currentSite` when a storage site is missing | ||
| site = currentSite() | ||
|
|
||
| logFailedStorageSiteRead( | ||
| siteID: siteID, | ||
| currentSiteFallbackValue: site | ||
| ) | ||
| } |
There was a problem hiding this comment.
WDYT about checking currentSite() first and ensure that the current site has the same ID as siteID? I expect that it does, and we would not need to check storage unless otherwise.
| let site: Site? | |
| if let storageSite { | |
| site = storageSite | |
| } else { | |
| /// Non - fatal fallback to `currentSite` when a storage site is missing | |
| site = currentSite() | |
| logFailedStorageSiteRead( | |
| siteID: siteID, | |
| currentSiteFallbackValue: site | |
| ) | |
| } | |
| let site: Site? = { | |
| if let site = currentSite(), site.siteID == siteID { | |
| return site | |
| } | |
| if let storageSite = storage.loadSite(siteID: siteID)?.toReadOnly() { | |
| return storageSite | |
| } else { | |
| return nil | |
| } | |
| }() |
RafaelKayumov
added
the
status: do not merge
joshheald
left a comment
joshheald
left a comment
There was a problem hiding this comment.
Works as expected, a few suggestions inline.
Thanks for the quick turnaround
| return onCompletion( | ||
| .failure( | ||
| OrderIsEligibleForCardPresentPaymentError.failedToObtainSite | ||
| ) |
There was a problem hiding this comment.
I think we can continue with the eligibility check in this case, making the assumption that it's not a CIAB site and skipping those checks. It can be done later if you prefer.
| enum OrderIsEligibleForCardPresentPaymentError: Error { | ||
| case orderNotFoundInStorage | ||
| case siteNotFoundInStorage | ||
| case failedToObtainSite |
There was a problem hiding this comment.
Perhaps be slightly more descriptive here?
Not essential but may help future debugging/support
| case failedToObtainSite | |
| case failedToObtainSiteForCIABFeatureCheck |
There was a problem hiding this comment.
@joshheald Addressed in 8ebbd2e. The flow now proceeds to orders check in case if a site wasn't obtained.
|
Closing in favour of merged hotfix PR |
WOOMOB-1813
Description
The PR addresses the issue where a user couldn't see TPP and card reader options in non-CIAB site. We suspect that the culprit is a site reading from local store. Since we couldn't reproduce the issue - we're adding this as a safety measure. Additionally we're adding error logging to see if the suspected issue indeed happens.
currentSiteas a safety measure for a case where a site can't be fetched by id from storage..errorlogging for the above case.errorlogging for the case where even acurrentSitecouldn't be obtained.Test Steps
RELEASE-NOTES.txtif necessary.