fix(payment): PI-4290 google pay shipping address required fix #3084
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| const useNewLogic = true; | ||
| // const useNewLogic = getStoreConfig()?.checkoutSettings.features['']; | ||
|
|
There was a problem hiding this comment.
Bug: The useNewLogic variable is hardcoded to true, and its feature flag is non-functional, preventing controlled rollout and rollback.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The useNewLogic variable is hardcoded to true at packages/google-pay-integration/src/gateways/google-pay-gateway.ts:452, forcing the new shipping address logic to be always enabled. The associated feature flag mechanism at line 453 is non-functional due to an empty key features['']. This prevents controlled experimentation and rollout, making it impossible to disable the new logic via a feature flag if issues arise in production, requiring a full code rollback. Consequently, the else branch (lines 460-461) becomes unreachable dead code.
💡 Suggested Fix
Define a proper key for the feature flag (e.g., 'PI-4290.googlepay_shipping_selector_fix') and use it to control useNewLogic, or initialize useNewLogic to false and enable it via the flag.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/google-pay-integration/src/gateways/google-pay-gateway.ts#L452-L454
Potential issue: The `useNewLogic` variable is hardcoded to `true` at
`packages/google-pay-integration/src/gateways/google-pay-gateway.ts:452`, forcing the
new shipping address logic to be always enabled. The associated feature flag mechanism
at line 453 is non-functional due to an empty key `features['']`. This prevents
controlled experimentation and rollout, making it impossible to disable the new logic
via a feature flag if issues arise in production, requiring a full code rollback.
Consequently, the `else` branch (lines 460-461) becomes unreachable dead code.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2775406
bc-dronov
force-pushed
the
fix/google_pay_reopen_modal
branch
3 times, most recently
from
e6b5be3 to
9bf4966
Compare
|
|
||
| async initializeWidget() { | ||
| await this._buildWidgetPayloads(); | ||
| async initializeWidget(shouldRequestShipping = true) { |
There was a problem hiding this comment.
Would be better to create a separate private method where you will set private variable based on shouldRequestShipping in this method and use it where you need to avoid prop drilling.
For example:
private setIsShippingAddressRequired(isRequired: boolean): void {
this._isShippingAddressRequired = isRequired;
}
|
|
||
| // experiment name will be added after code freeze | ||
| const useNewLogic = | ||
| getStoreConfig()?.checkoutSettings.features.experiment_shouldRequestShipping || false; |
There was a problem hiding this comment.
use isExperimentEnabled pls
| this._paymentIntegrationService.getState(); | ||
|
|
||
| // experiment name will be added after code freeze | ||
| const useNewLogic = |
There was a problem hiding this comment.
I think it would be better to rename this variable to more appropriate one
| features: { | ||
| ...storeConfig.checkoutSettings.features, | ||
| 'PI-2875.googlepay_coupons_handling': true, | ||
| // experiment name will be added after code freeze |
There was a problem hiding this comment.
It is a good practice to do not open PR if something is not ready yet and is being in testing to to avoid unnecessary extra work for reviewers at least
There was a problem hiding this comment.
@bc-nick the only thing that will be changed later is experiment name, so there is no sense to delay the review
bc-dronov
force-pushed
the
fix/google_pay_reopen_modal
branch
2 times, most recently
from
9c60db0 to
084725c
Compare
bc-dronov
force-pushed
the
fix/google_pay_reopen_modal
branch
from
084725c to
269778e
Compare
What/Why?
We have a bug with Google Pay when we open a GP payment sheet and there is a shipping selector, if we close it and re-open again the shipping selector will not persist.
That's because of the flag
getShippingAddress() === undefined- when we open a GP payment sheet, we set the shipping address, and the second opening will not require the shipping because we already have it. This behaviour is incorrect, we don't need the shipping address only on the payment step, on other steps (customer, cart / PDP) we should have the shipping address selector. So I decided to add a custom flag passing from the payment step which determine that we don't need the shipping selector on the payment step, all the rest steps will be required to render the shipping step, unless our checkout has only digital products.Rollout/Rollback
Will cover this feature with experiment after code freeze
Testing
Manual