Ignore #49. Updated the former related issue, just a heads-up.
Still very eager to get this merged! 🤩
The Paypal integration was already implemented through 📌 Add PayPal support to Payment Element Gateway Active this issue only targets the Link implementation now.
grevil → made their first commit to this issue’s fork.
We have new information on this! See the comment in the parent issue here ✨ Print details from error messages returned by paypal Active .
Yes, we just tested the patch yesterday.
We get the "PAYMENT ACTION REQUIRED" as expected and following the attached link will bring us to PayPal, where we are seeing the correctly updated price. Unfortunately, as @anybody already stated, confirming the purchase will bring us into an endless PayPal loop.
https://developer.paypal.com/docs/checkout/standard/customize/overcharge... suggests, we should simply do a redirect to the provided link, but we somehow also need to provide a "return_url" (https://developer.paypal.com/docs/checkout/standard/customize/overcharge...). Otherwise, we won't get redirected to the shop after confirming the purchase.
But we are still unsure how to provide this "return_url". Using it as a query parameter on the redirect link didn't work properly, so we might need to make another sdk call. It might also be enough to simply provide a "return_url" and "cancel_url" on the "Create Order API" call (if that isn't already done).
Unfortunately, it isn't well documented on how to do this... @jsacksick any ideas?
All done! Please review!
Alright, that's it! Please review!
It is not worth changing the plugin manager "createInstance" method to additionally use a group to create a plugin instance.
Instead, simply prefixing the id will do the trick.
Current MR as patch.
Merged!
Done! Please review!
grevil → created an issue.
All done.
Now that we switched to the plugin manager, we might already have "proper" caching for the most part.
No, I meant a Oembed Widget Example. The widget url IS the endpoint.
Basically just a widget provider, that returns a JSON in the oembed format. No submodule.
All green! Merging.
Hey @tomtech, just wanted to ask how soon we can expect to get this reviewed / merged?
Also, I couldn't find an issue regarding the missing PayPal / Link PaymentMethodTypes, so I created one here: 🐛 Implement PayPal and Link "PaymentMethodType"s Active . Not sure if you guys plan to implement that yourself? Otherwise, we could also chime in to help implement the PayPal integration, as the "commerce_paypal" Express Checkout Buttons are currently no option for us (see 🐛 Anonymous user can't finish checkout using PayPal Express Checkout, when shipping rates exist Active ).
Thanks for the feedback! 🙂👍
grevil → created an issue.
Ok we could safely reproduce #10 now.
It definitely happens because of the missing shipping rate price in the total. Note, that this is NOT reproducible using sandbox mode.
Interestingly enough, the commerce stripe express checkout handles this correctly (see ✨ Stripe Express Checkout Element Integration Active ). But this might be a stripe thing. Stripe (through AmazonPay for example) communicates back and forth with the Server, so once AmazonPay has the address, they pass the address back to the server and the server can calculate the shipping rates based off the address given and send the available rates to stripe / amazon pay.
Please finish this @LRWebks
grevil → created an issue.
LGTM! Great work! 🎉🌈
grevil → created an issue.
Ok, back to needs work. Newest developments in 🐛 Needs to redirect users on UNPROCESSABLE_ENTITY > PAYER_ACTION_REQUIRED - No checkout possible Active , show, that we need to properly handle the details and especially the links in a loop instead of resetting to the first issue / link available.
Ok, I gathered a few more information on this problem and I think I found the problem.
The problem is, that our shop overcharges the user above the amount he agreed to pay for, which is not allowed and against the "Overcapture requirements (PSD2)", on which PayPal implemented a way to reconfirm the new updated price through the "payer-action" link returned from the "PAYER_ACTION_REQUIRED" error response.
For more information, please take a look at:
https://www.paypal.com/ae/cshelp/article/overcapture-requirements-psd2-h...
https://developer.paypal.com/docs/multiparty/checkout/standard/customize...
This is easily reproducible using the express checkout buttons:
- An anonymous user buys a product through the express checkout cart button (we have no information on their address yet)
- He approves to pay the product costs via PayPal
- The user gets redirected to the shop. The shop now knows the address of the user (through by PayPal)
- The shop can now calculate the shipping rate based on the address given and patch the PayPal auction.
- When trying to finalize the purchase (capture the payment), PayPal returns a "PAYER_ACTION_REQUIRED" response containing a "payer-action" link, because the user approved the costs of the product only, not the costs of the product + shipping cost.
- The "payer-action" link redirects the user to PayPal, where he needs to reconfirm the new updated amount.
Unfortunately, there seems to be no error in sandbox mode for whatever reason. It seems like overcharging in sandbox mode is just fine, maybe there is a setting to tweak this behavior.
The remaining question is, whether this is something, that should be handled by the service provided through ✨ Print details from error messages returned by paypal Active . Or something else entirely. @tomtech @jsacksick what do you think about this?
Now, that we enabled the express checkout buttons, there are a LOT of messages like these in our shop. We temporarily disabled the express buttons, until this is properly handled.
When I apply the patch, update the db and add a new link, I get the following error:
TypeError: _menu_link_attributes_add_attributes(): Argument #2 ($options) must be of type array, null given, called in /var/www/html/web/modules/custom/menu_link_attributes/menu_link_attributes.module on line 36 in _menu_link_attributes_add_attributes() (line 47 of modules/custom/menu_link_attributes/menu_link_attributes.module).
@grevil looks like this needs an update hook to set this TRUE by default
Nope, it is good to go! "ViewsExposedFilterBlocksBlock" is a plugin, so the "defaultConfiguration()" method will do the job for us! LGTM!
Drupal 11 is stable for a while now, there won't be any more new patches from the bot! Setting this "Fixed".
clear() is public now! This is ready for review!
Great stuff! Thank you, @tomtech! 🎉
Something along these lines should suffice. I am unsure, whether "commerce_order.order.paid" is the correct event to use. But I couldn't find a better.
grevil → created an issue.
Magically fixed! 🌈
Sorry, we thought it might relate to 🐛 Orders are not completed Active .
@jsacksick, what do you think about the fix? It's pretty unclear, to us, how this fixes the issue.
Sadly, @alex.bukach did not reply.
We should indeed try to finish this (cleanly) as this could impact conversions and shop owners might not notice this issue at all.
Hmmm... Actually, I think we did it this way so that the alteration is done runtime rather than affecting the adjustment that is stored in DB. This way the adjustment label would always be on the current site language...
Can't remember exactly anymore. But we could still add a new setting (off by default) which prevents the subscriber to do anything, until activated!
EDIT: Or maybe there is something like an conditional subscriber in drupal / commerce? Unsure about that.
Alright! Seems to work as expected! We just had 2 test purchases inside a live environment using amazonPay and googlePay. Both worked as expected (Klarna was tested before inside a sandbox environment, which also worked great).
Setting this to "Needs review" for the Centarro people to take a final look at the changes!
New patch. Old patch had a bug I introduced, sorry for that.
@tomtech thanks for the feedback! And great to hear, that you guys are on it! Really appreciate the work of your team (and especially by @vmarchuk for working on this issue!).
I disabled support for link and PayPal. We will test it live now and come back to you guys again!
Static patch of the current MR attached.
Sorry, back to "Needs work" regarding PayPal.
Alright, this is pretty far now.
Regarding the payment methods:
Klarna
Works great and as expected.
AmazonPay
Works great and as expected.
GooglePay
Works great and as expected.
ApplePay
Not tested. I do not have Safari.
Link
Could not test link, since I couldn't log into my account for some reason
PayPal
After confirming the payment I get an error and get sent to the "checkout/{order_id}/order_information" page with the error:
Payment failed at the payment server. Please review your information and try again.
There are still a couple of console errors and I feel like a few parts of code should use existing commerce services instead of "reinventing the wheel" (e.g. the shipping address validation). But generally speaking, this is fairly close!
We will test it in a live environment tommorow and give further feedback.
Ignore #40 ✨ Stripe Express Checkout Element Integration Active we now skip the adjustment if it is of type "tax" and already included in the line item price. Meaning, the original implementation of "commerce_stripe_get_express_checkout_order_line_items" by @vmarchuk is restored again (with the mentioned check included).
But I am still unsure if is the correct approach, to pass the tax as a line item. But according to the Stripe AI assistan it is:
Certainly! Here's how you can add tax to a line item using Stripe.js without automatic tax calculation:
example.js
const stripe = Stripe('your_publishable_key');
const lineItems = [
{
price_data: {
currency: 'usd',
unit_amount: 2000,
product_data: {
name: 'T-shirt',
},
},
quantity: 1,
},
{
price_data: {
currency: 'usd',
unit_amount: 200,
product_data: {
name: 'Sales Tax',
},
},
quantity: 1,
}
];
stripe.checkout.sessions.create({
mode: 'payment',
line_items: lineItems,
success_url: 'https://example.com/success',
cancel_url: 'https://example.com/cancel',
}).then(function(session) {
// Handle the session
});
In this example:
We define two line items in the lineItems array:
The T-shirt priced at $20.00 (2000 cents)
The sales tax as a separate line item, priced at $2.00 (200 cents)
We don't use the automatic_tax parameter, as we're manually adding the tax as a separate line item.
But on docs provided as resources by the AI I couldn't find this approach. But yea seems good enough.
as mentioned, waiting for other community members to chime in here, especially the ones that contributed patches previously. But from a pure code point of view, this looks good now
I don't think anyone will chime in here anymore. Can we get this fixed?
Ok, I found the issue. Adjustments were added as "extra_line_items", leading to this error.
Although this code seems intentional, I could not yet find the reason it was coded this way. @vmarchuk could you give feedback, on why the adjustments were added as line items? It might be needed for "onShippingRateChange()" and "onShippingAddressChange()" but not passed to the js eventually? Or we need to revert https://git.drupalcode.org/project/commerce_stripe/-/merge_requests/129/... again and just add a check?
Awesome! Thanks, @dydave and @ressa for your quick back 2 back finalization of this! 😍
Ok, the problem is the VAT that is part of the line items, even if it should be already included in the price of the products:
I'll have a closer look at this tommorow.
Added a few comments on the MR. Furthermore, when pressing e.g. the amazonPay checkout button, I get the following error in console:
Uncaught (in promise) IntegrationError: The amount 1986 is less than the total amount of the line items provided.
And the Amazon popup just loads infinitely.
I do not get this error when e.g. using the express checkout buttons from commerce_paypal.
@vmarchuk the testing environment is currently broken, could you revive it?
Already told Jonathan about that via Slack, he is already at it / forwarded it.
I added a general hint in 🐛 Documentation improvements Active , that based on that setting, some active payment methods will be unavailable. Would that suffice already? If so, this issue can be closed as duplicate.
Adjusted accordingly. Please review now.
I'd prefer "PaymentExceptionHandler". None of the other classes have a "PayPal" prefix.
Also, with what we're doing, wouldn't that mean that we get boh the generic Drupal Commerce error + this one?
Yea, we would get the error + a message with the same error. I agree, that this wouldn't be very nice UX wise. I'd say, we throw a generic Error Text and let the message be more specific.
Let's wait for the error handler service from ✨ Print details from error messages returned by paypal Active , after which we continue here.
POSTPONED on https://www.drupal.org/project/commerce_paypal/issues/3490120 ✨ Print details from error messages returned by paypal Active .
Ok, that should be it! Please review!
Note that this service is currently pretty "barebones", but can be extended in follow-up issues like 🐛 Needs to redirect users on UNPROCESSABLE_ENTITY > PAYER_ACTION_REQUIRED - No checkout possible Active . We think it only makes sense to do the error handling in one place, which we can further refine in the future.
@rakesh.regar provided @tomtech's patch as an MR.
Changes LGTM and makes the JS a bit more safe. Unsure if it will fix the original issue, as there aren't clear steps to reproduce (other the ones from @tomtech).
The only thing is, that in the case "data.status" is "403", the added message will quickly disappear through location.reload(). Other than that, it looks fine!
Let's hear feedback from others.
This is meant as a general solution for all cases where a "BadResponseException" is being processed. Sorry if the MR confused you. I just picked out one place where this is the case.
A service would be preferable if we want to hide information about specific errors or specific roles.
List of the available errors and their messages: https://github.com/paypal/paypal-js/blob/a2e716e0a0982fe79c8c51e8c04e187...
But what's shown in the screenshot looks kinda buggy (as we see the tags etc)
Yea just quick concept code, I wanted to hear your feedback first.
Here a first concept how this could look like. Of course, we should streamline this approach through moving this code into its own service and using the service, whenever we catch a "BadResponseException".
Inside the service we could also filter out, for which error we want to display further information and for which not (if needed). We can also differentiate between what error should be thrown for an admin user vs. what should be shown for a normal user (using a dedicated permission).
What are your thoughts on this @jsacksick? Would this be worth pursuing? Here is a screenshot of the current MR (although the Markup is escaped):
Thank you!
@jsacksick you forgot to initiate the UUID service! Which currently leads to the following error when refunding:
Error: Call to a member function generate() on null in Drupal\commerce_paypal\Plugin\Commerce\PaymentGateway\Checkout->refundPayment() (line 669 of modules/contrib/commerce_paypal/src/Plugin/Commerce/PaymentGateway/Checkout.php).
Interestingly, I can't even refund a payment on latest dev release.
I get the following error:
Error: Call to a member function generate() on null in Drupal\commerce_paypal\Plugin\Commerce\PaymentGateway\Checkout->refundPayment() (line 669 of modules/contrib/commerce_paypal/src/Plugin/Commerce/PaymentGateway/Checkout.php).
Here is a list of all available issue descriptions:
https://github.com/paypal/paypal-js/blob/a2e716e0a0982fe79c8c51e8c04e187... (note, that 422 is UNPROCESSABLE_ENTITY).
IMO, the error message descriptions are fairly safe in an admin context.
Of course we have to be careful to not publish sensitive information and should have a look at PayPals own implementations, in which cases they espose these messages.
There are no user information in these error descriptions.
Alright, I fixed the remaining issues noted in #28. That should be it now!
Ok, this is it! Done!
Thanks for the feedback @larowlan! We'll discuss this internally and come back to you!
Talked with @anybody internally. Not really worth taking a deeper look at this.
This shouldn't happen. "ignore_tags" has their default setting defined in the Doctrine Annotation above each filter plugin. So "ignore_tags" should always resolve at least to an empty string, not NULL. Maybe a simple clear all caches will fix the issue?
There is no need for an update hook, since this is a plugin. Filters never implement "defaultConfiguration()" for some reason. Not here nor in core. They are always defined in the doctrine annotation / PHP Attributes.
Done, please review.
grevil → created an issue.
Alright, that should be it!
All done, please review!
Ok, I will prepare everything to declare "theme" deprecated. Thanks for your feedback!
EDIT: I created a CR draft: https://www.drupal.org/node/3523553 →
But I am generally confused, why we are doing so much fuss about this? IMO, we should just adjust the label of "controls" from "Show playback controls" to "Use native controls" and overwrite the default from TRUE to FALSE and be done with it.
I can't seem to find any code unsetting "controls", or am I missing something here? As already stated before in #8 "controls" is not defined by us, but comes from "FileMediaFormatterBase", meaning it needs to be unset explicitly in the "VidstackPlayerVideoFormatter".