Account created on 8 October 2010, over 15 years ago
#

Merge Requests

More

Recent comments

🇮🇱Israel jsacksick

Support requests are generally not answered from this queue:

With the abundance of support available in Drupal Answers, the Drupal focused Stack Exchange site, we are no longer answering support requests in the issue queue.

Support requests opened here will be closed (won't fix), so please search for existing answers or post new questions at:

https://drupal.stackexchange.com (using the drupal-commerce tag)

🇮🇱Israel jsacksick

No, you can't NEST conditions from Commerce core UI, so I don't think there is anything we can do here?

🇮🇱Israel jsacksick

I agree on the pricinciple, but this is a broader issue affecting Commerce in general where references are stored rather than a full record.
The same apply to a product variation being removed after it was purchased, or a promotion for example.

🇮🇱Israel jsacksick

You're not checking the right thing... Go to admin/commerce/config/shipment-types/default/edit/form-display.

🇮🇱Israel jsacksick

This is by design yes, to allow for a shorter checkout experience.
You can skip collecting any of the information that's already collected by PayPal.

I am also no longer able to select the module provided PayPal Checkout flow as an option in my order type's checkout settings.

Yes, because this checkout flow is meant to be used for the "shortcut" flow (i.e when the payment is initiated from the cart page), not for the regular flow.

🇮🇱Israel jsacksick

Updating the status to "Needs work" because of the failing tests.

🇮🇱Israel jsacksick

I'm wondering if we should introduce a new major branch here as well.

🇮🇱Israel jsacksick

We've decided internally to skip this change for now.

🇮🇱Israel jsacksick

Weird that the tests are failing.

Let's rename the trait to PaymentOrderSummaryFormTrait, like the following:

/**
 * Provides a trait for rendering an order summary in payment-related forms.
 */
trait PaymentOrderSummaryFormTrait {

Also, let's add a weight to the "order_summary" form element:

$form['order_summary'] = [
      '#type' => 'container',
      '#attributes' => ['class' => ['order-summary', 'payment-order-summary']],
      '#weight' => -10,
    ];
🇮🇱Israel jsacksick

Can we tweak the naming a little bit?

Event object: TransactionFailureEvent.
Constant: TRANSACTION_FAILURE.
Value: commerce_braintree.transaction_failure

This seems to be more consistent with what we're doing elsewhere.

🇮🇱Israel jsacksick

I just wanted to report that Commerce is suffering from the exact same issue: See https://git.drupalcode.org/project/commerce/-/jobs/8222384#L345.

🇮🇱Israel jsacksick

jsacksick created an issue.

🇮🇱Israel jsacksick

jsacksick created an issue.

🇮🇱Israel jsacksick

jsacksick created an issue.

🇮🇱Israel jsacksick

jsacksick created an issue.

🇮🇱Israel jsacksick

jsacksick created an issue.

🇮🇱Israel jsacksick

jsacksick created an issue.

🇮🇱Israel jsacksick

Committed to 3.x dev! Thanks!

🇮🇱Israel jsacksick

jsacksick changed the visibility of the branch 3569178-use-links-instead to active.

🇮🇱Israel jsacksick

jsacksick changed the visibility of the branch 3569178-use-links-instead to hidden.

🇮🇱Israel jsacksick

What you're highlighting in the screenshot is the "Default step" of the pane. by default the shipping information pane is assigned to the disabled step.

So the screenshot isn't relevant. If shipping rates aren't recalculated, the problem lies somewhere else.

🇮🇱Israel jsacksick

jsacksick created an issue.

🇮🇱Israel jsacksick

jsacksick made their first commit to this issue’s fork.

🇮🇱Israel jsacksick

jsacksick made their first commit to this issue’s fork.

🇮🇱Israel jsacksick

Still not fully fixed, but at least it doesn't look terribly broken. Perhaps the remaining issues are due to Commerce.

🇮🇱Israel jsacksick

Still not functional, but better... Also, this likely doesn't work for the user add payment method form, but this likely requires way more work.

🇮🇱Israel jsacksick

jsacksick created an issue.

🇮🇱Israel jsacksick

Merged, thank you!

🇮🇱Israel jsacksick

There is substiantal rework happening in dev. To the order view page. The order edit form is meant to be "abandoned" in favor of inline edit within the order view page.

🇮🇱Israel jsacksick

Tests are failing, several calls to dispatchFailedPaymentEvent() weren't updated. So this needs additional work.

🇮🇱Israel jsacksick

I know, and it was merged. I wanted to tag a Commerce release before EOY but we're still working on a few things that aren't finalized yet.

🇮🇱Israel jsacksick

Let's not depend on JQuery if possible. Core is moving away from JQuery, so as our recent JS.

🇮🇱Israel jsacksick

jsacksick created an issue.

🇮🇱Israel jsacksick

jsacksick made their first commit to this issue’s fork.

🇮🇱Israel jsacksick

jsacksick made their first commit to this issue’s fork.

🇮🇱Israel jsacksick

Current UI with the changes from the MR:

🇮🇱Israel jsacksick

jsacksick created an issue.

🇮🇱Israel jsacksick

Likely Commerce core as it provides a CustomerProfile inline form plugin.

And do you know when the change might be worked on?

No, not yet :(

🇮🇱Israel jsacksick

We'll likely write an update hook in a followup issue to update the default view display.

🇮🇱Israel jsacksick

jsacksick made their first commit to this issue’s fork.

🇮🇱Israel jsacksick

Did you check the configuration? I mean, there is not a lot we can do on our side if fields are magically exposed / added by a 3rd party module. A form display is used, so you really have to check that.

🇮🇱Israel jsacksick

@damienmckenna: The recalculation is triggered only in the case a required field is updated.
Also, fwiw, we'll likely not proceed with this after all.

We're contemplating a potential UX change that would introduce a Save button for adresses, which should help with the rates recalculation as we'd have a more efficient way of detecting that an address has changed.

🇮🇱Israel jsacksick

We're close (I think) though I'm not sure if we kept coverage for the existing form? Or are we just testing the new form?

Let's rename the setting flag to:
commerce_payment_use_legacy_add_payment_form

Also commerce_payment_update_10303 has the following message:

  $message = t('Your payment add form has been updated to a new single page format. If you depend on the old two step method, you will need to set a variable in settings.php to preserve it. See the release notes for Commerce Core x.y.z for more information.');

So let's change x.y.z to 3.3.0. and let's actually update "you will need to set a variable in settings.php" to 'you will need to set the "commerce_payment_use_legacy_add_payment_form" variable in settings.php'.

🇮🇱Israel jsacksick

The Commerce icon changed, we now override the cart icon that ships with GIN, so we likely need update our icon.

🇮🇱Israel jsacksick

jsacksick created an issue.

🇮🇱Israel jsacksick

Did you check your shipment type configuration? Check the form display, you might have extra fields showing up there. I don't believe there is any "bug" here.

🇮🇱Israel jsacksick

jsacksick created an issue.

🇮🇱Israel jsacksick

Will a stable release be available for installation before 25 January? If not, can someone provide a detailed description of what needs to be done to install the needed updates?

I'm thinking of tagging a beta1.

🇮🇱Israel jsacksick

Changes are merged. Followup improvements should be made in separate issues.

🇮🇱Israel jsacksick

After fixing the selectors:

🇮🇱Israel jsacksick

jsacksick created an issue.

🇮🇱Israel jsacksick

Discuss this with @berdir today, apparently this can be safely skipped thanks to core committing a workaround. See 🐛 Workaround PHP bug with fibers and __get() Active .

🇮🇱Israel jsacksick

That looks terrible:

 $customer = $this->get('uid')->first()?->get('entity')->getTarget()?->getValue();

And we also use the magic getter literally everwhere...

🇮🇱Israel jsacksick

#12: definitely.

🇮🇱Israel jsacksick

then in processPaymentCallback

what is "processPaymentCallback"? Is that your custom code?
if it's custom code, can't you catch the LogicException?

🇮🇱Israel jsacksick

Tests are failing, putting this to needs work.

🇮🇱Israel jsacksick

Can we try using the FocusFirst Drupal core command? Left additional minor feedbacks. haven't manually tested this yet but will after we confirm whether the core command can be used.

🇮🇱Israel jsacksick

hm... In theory yes, but feels weird for this new interface to have a setter...
Maybe we should go with your first commit... I feel more comfortable shipping with this workaround for now rather than trying to figure out where this interface should live, figuring the naming, the setter etc...

So let's just stick to the first version.

🇮🇱Israel jsacksick

I'm just not sure I understand what's fixed in practice by your change? Could you please expand on this? Do you have custom code embedding the payment gateway form?

🇮🇱Israel jsacksick

Merged, thanks!

🇮🇱Israel jsacksick

So for your use case, what exactly happens? There is no crash but the payment isn't skipped, right?

🇮🇱Israel jsacksick

Experiencing #8 as well on PHP 8.3 and 11..

🇮🇱Israel jsacksick

Ok tests are failing on D10 due to the use of getOriginal() which was introduced in D11.
Perhaps Commerce core could introduce a helper so it can be used accross Commerce.

There are phpcs failures.

And ideally we'd add a test to confirm the following:

  1. A shipment is created by clicking the "Add shipping information" link, when the order has no items
  2. The shipment is marked as owned_by_packer after the modal is closed / the form is saved.
  3. Adding order items result in the shipment being repacked / refreshed (i.e items not empty).
  4. We should probably check that the shipment is rated too.
Production build 0.71.5 2024