🇺🇸United States @AaronBauman

Depends on 🐛 Check for session before calling getSession(), or catch SessionNotFoundException Active

aaronbauman → changed the visibility of the branch 3532439-TEST-ONLY to hidden.

Oh, thanks for the heads up - TIL!

Is that a configuration for core only, or for all projects?

aaronbauman → created an issue.

3532360-TEST-ONLY MR !12485 - test only to demonstrate bug.

3532360-check-for-session MR !12486 - includes test and fix, ready for review

MR 13 ready for review

Neither patch nor MR are applying to D11

aaronbauman → created an issue.

aaronbauman → created an issue.

I see.
Do you have a thread open about this in Drupal core somewhere?
I'm not an expert on dependency injection, but my understanding is that this approach breaks testability and autowiring. Possibly other consequences as well. Seeing as how Commerce is, like, top 10 modules for Drupal, it's weird to implement an anti pattern.

I won't hold my breath about Commerce specifically, but I'll urge you to keep an open mind about it.

I started working on a MR, but this issue is pretty extensive.

Will wait to hear from maintainers as to whether this change would be considered before I sink more hours into it.

aaronbauman → changed the visibility of the branch 3530865-why-is-dependency to hidden.

aaronbauman → created an issue.

aaronbauman → made their first commit to this issue’s fork.

aaronbauman → created an issue.

aaronbauman → made their first commit to this issue’s fork.

Reviving this because I'd like to add shipping and tax to the orders view.

There are two "Adjustment" fields that show up in views - for Orders and Order Items - but they don't render anything.

Is Commerce supporting adjustments in views now?
And if not, is it still too painful to add support?

Yup, makes sense.

I found the module https://drupal.org/project/jquery_downgrade that I'm gonna try to limp along with for now.

I haven't thoroughly tested, but at least some of my pages appear to be rendering properly now and I'm not getting a console full of javascript errors.

Helpful! And thanks for updating the IS

Yeah, it is working fine.
I was confused somehow

aaronbauman → created an issue.

Since this issue was the top hit on Google for my query "drupal 11 bootstrap jquery" - what is the status?

Is Bootstrap going to work with Drupal 11 or not?

Opened MR144
https://git.drupalcode.org/project/radix/-/merge_requests/144

aaronbauman → created an issue.

Figured it out. The MR didn't have a composer.json

aaronbauman → made their first commit to this issue’s fork.

aaronbauman → created an issue.

aaronbauman → created an issue.

aaronbauman → created an issue.

Looks good to me.
Thanks for jumping on that feedback so quickly.
I'll set to RTBC and get it into the next release.

push params event is also called in PullBase

Probably the SalesforcePushParamsEvent in PullBase should be made into its own class.
Seems like it was shoehorned in there, and is not exactly appropriate for the use case.
That's outside the scope of this issue though.

This is good to go i think.

Also opened related issue 📌 Improve pull queue exception handling and subscriber options Active

aaronbauman → created an issue.

LGTM

Thank you for the detailed description and the MR.
This is an interesting idea, I think we can make something work here.

I don't like having these 2 different events one after another - one event should suffice. And I'd like to see it be a little less single-purpose.

Proposed changes:
1. Build this logic into existing PushParams event, rather than adding a new event
2. Allow subscriber to override push operation and choose any of update, upsert, or create

So maybe that looks like a new method on PushParams, which can be set by a subscriber to override whatever operation would have been selected in MappedObject::push

Does this make sense? I think it would solve your issue, as well as making this more useful in more scenarios.

aaronbauman → created an issue.

Opened MR448 converting patch #32 into MR, and moved the availability logic into an AvailabilityChecker

Seems like this should all be going into an AvailabilityChecker instead of jammed into the process method:

      $purchased_entity = $order_item->getPurchasedEntity();
      if (!$purchased_entity) {
        if ($order_item->hasPurchasedEntity()) {
          $order_items_to_remove[$order_item->id()] = $order_item;
        }
        continue;
      }
      // Check if the product or variation is unpublished.
      $product = $purchased_entity->getProduct();
      if (empty($product) || !$product->isPublished() || !$purchased_entity->isPublished()) {
        $order_items_to_remove[$order_item->id()] = $order_item;
        continue;
      }

MR44 is passing, back to NR

Hid the other 2 MRs

aaronbauman → changed the visibility of the branch 3107993-3x-template-not-found to hidden.

aaronbauman → changed the visibility of the branch 3107993-template-is-not to hidden.

+1 RTBC pls merge

MR27 gtg please merge.

Please commit

So it is, thanks!

aaronbauman → created an issue.

aaronbauman → created an issue.

aaronbauman → created an issue.

aaronbauman → created an issue.

Patch #2 works great for me, thanks

This doesn't have anything to do with Salesforce module.

Please create an issue against core, or whatever contrib module is responsible for this.

This is committed to 2.0.4

This patch uses the "amount_to_collect" received from Taxjar and creates a single, Adjustment on the Commerce Order for the tax amount.

Please review.

Fixed, thanks for the patch

The module does send the shipping cost to Taxjar, but I honestly don't know how that gets handled on the Taxjar end.

Do you know of a jurisdiction in which shipping is taxable, so that we could test it?

Fixed, thank you for the patch

Yes, you can implement hook_commerce_taxjar_tax_request_alter and hook_commerce_taxjar_transaction_request_alter to set $request_body to an empty array based on your business logic. This will avoid sending the request to TaxJar altogether.

Closing all 7.x issues. Please reopen if this issue still pertains to D10 version

Closing all 7.x issues. Please reopen if this issue still pertains to D10 version

Closing all 7.x issues. Please reopen if this issue still pertains to D10 version

Closing all 7.x issues. Please reopen if this issue still pertains to D10 version

Closing all 7.x issues. Please reopen if this issue still pertains to D10 version

Good idea, thanks for the patch

Fixed, thanks for the patch

Good idea, thanks for the patch

Thank you for the patch, this is now moot

Fixed in D10 version, thank you

Fixed, thank you

aaronbauman → made their first commit to this issue’s fork.

This was due to a patch I was running locally. Working as expected without that patch.

Emailed andyg5000 requesting commit and release access.

aaronbauman → created an issue.

aaronbauman → created an issue.

aaronbauman → created an issue.

aaronbauman → created an issue.

aaronbauman → created an issue.

Yeah, i'm running into all sorts of problems.

I had to switch back to the basic credit card pane, and even that is be a bit buggy.

aaronbauman → created an issue.

aaronbauman → created an issue.

https://git.drupalcode.org/project/salesforce/-/merge_requests/96

aaronbauman → created an issue.

Re: detecting config overrides, this is now a core feature: 📌 Implement new #config_target framework for config forms Active

Re: automatically invalidating token, I'm not convinced this is a good idea.
Any sort of issue may arise where we get a false positive, and invalidate the token for the wrong reason.
I'm think ✨ Provide a way to utilise multiple Salesforce auth providers Active gets us closer toward a more robust solution.

What do you think of putting the provider id(s) on the mapped object, instead of the mapping?

I'm thinking through a few scenarios here, let me know what you think:

1. Dev Sandbox vs Prod, refreshed and up to date - fields may be the same. Mapping could be applicable to both. No reason to have duplicate mappings, but we need separate Mapped Objects.
2. Full Sandbox vs Prod, refreshed and up to date - Again Mapping could be applicable to both. IDs for Mapped Objects are the same. Mapped Object could also be relevant in both orgs.
3. Dev Sandbox vs Prod, out of date - Mapping could still be applicable to both, but would be useful to be able to designate only one. Need separate Mapped Objects.
3. Full Sandbox vs Prod, out of date - Mapping could still be applicable to both, but would be useful to be able to designate only one. Could use same Mapped Objects.

aaronbauman → created an issue.

I'm talking about the situation where a subscriber might want to postpone pull.

The "disallowPull" method doesn't support this, outside of manually forcing another pull attempt.

There's no permanent disallow that i know of...

oh nice, https://www.drupal.org/project/advancedqueue → might do the trick

i'm all about maintaining less code whenever possible

One reason is to dedupe the queue entries.
This is happening already in Push Queue, but not Pull Queue.

Since the core database queue serializes all the data, there's no simple way to dedupe without loading every single queue item.
Not feasible.

For this same reason, I think Pull Queue should also be converted to a custom backend.

Another reason - not currently happening for push queue, but again not feasible with core queue - is so that queue items can be batched to minimize API usage.

Fixed, thanks

I'm not sure it makes sense to change behavior of this field plugin, which is already widely in use.

May be more appropriate to add a setting, or a new field plugin altogether.

Makes sense, merged.

FWIW, Salesforce suggests a minimum interval of 5 minutes.

aaronbauman → made their first commit to this issue’s fork.

addressed by 🐛 Call to a member function render() on string Active

Have you been using this patch in production?

The annoying thing with "disallow pull" is that it doesn't get automatically retried, ever.
So, even if the parent SFID does eventually get created, it won't trigger another pull attempt for the child record.

This is somewhat tangential, and shouldn't block this patch, but something to consider for folks that want to use this feature.