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.

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

gw-quantity-as-decimal.php: Added suppport for decimals in feed product quantity. #835

Closed
wants to merge 1 commit into from

Conversation

barthc
Copy link
Contributor

@barthc barthc commented May 24, 2024

Context

⛑️ Ticket(s): https://secure.helpscout.net/conversation/2592472572/65993?folderId=8114575

Summary

When the GW_Quantity_Decimal snippet is used to allow a decimal in the quantity input field, the incorrect price is sent to the payment gateway. This PR would fix the issue by allowing decimals for the feed product quantity.

Copy link

Warnings
⚠️ When ready, don't forget to request reviews on this pull request from your fellow wizards.

Generated by 🚫 dangerJS against 3e3ff25

Copy link
Contributor

@claygriffiths claygriffiths left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! I have one request.

@spivurno, I'm also curious what your thoughts are on this. I saw that Barth modified the snippet and sent it as a Gist, but I'm wondering if this particular addition would be better in an experimental snippet instead?

@@ -90,6 +93,114 @@ function modify_quantity_input_tag( $markup, $field, $value, $lead_id, $form_id
return $markup;
}

function modify_submission_data( $submission_data, $feed, $form, $entry ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is pretty hefty, and I'm admittedly having a hard time following what exactly we're doing here.

The appears to be a copy and paste of get_order_data with some changes. Is that correct? If so, can you call that out and note what changes you made so if something changes upstream that breaks this snippet, we'll know what to update in the future?

@spivurno
Copy link
Contributor

@claygriffiths Given the scope of the change and the burden we're taking on by incorporating so much core code here, I'd be in favor of drafting this (or closing if that is preferred) until we can validate the need.

@barthc
Copy link
Contributor Author

barthc commented May 28, 2024

@spivurno @claygriffiths I suggest we close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants