Attaching a reroll of your patch with no changes, as the underlying code had changed a bit. That said, I'm still not sure it's a great UX. The modal can be closed with escape, but there's no X a user could click, nor does clicking outside the modal dismiss it. There's a bug in the behavior, too, in that you cannot then resubmit the form if you dismiss the modal, because the JS disables direct form submission.
This is the patched appearance:
I don't mind just improving what we have, but I'm also open to rethinking the UX based on what I've seen elsewhere. This from Miva is really interesting, for example:
You can easily dismiss theirs, and the side by side comparison / selection of the address is nice. Their button is weird, though, in that using what you entered shouldn't be construed as an update ... I'd likely just generalize that to a "Continue" button. That said, I'm not sure how much we'd have to rework to make the method based on a radio button selection, so my approach to start will be to just make the modal work with dismissal and checkout form resubmission.
Retaining this for now in case there's an improvement for us to make in 2.x.
Revising this ticket to point to the payment integrations section of https://docs.drupalcommerce.org/v2/user-guide/ instead.
After a few passes, all done in the 3517257-jslint branch.
Turns out the library itself being included multiple times per page was the issue. I went ahead and converted all three JS libraries to remove jQuery and add Drupal's once to prevent multiple application of the behaviors. All works great now!
I hoped this would be a simple revision of the JavaScript behavior making use of Drupal core's once library, but that doesn't seem to solve the issue. Now instead of always getting 5 iframes, sometimes I get none, and sometimes I get 5. Will keep digging.
Ultimately, it appears Affirm does not support mixed accounts. We finally got a separate account for Canadian transactions and have no ability to intermix the two.
Revised the message pattern and added log messages for all operations.
Committed.
Validated against and merged into the 2.x branch.
Committed a fix.
rszrama → created an issue. See original summary → .
Thanks so much for following up on this! I finally got a Canadian sandbox account from them to test against and confirmed that the API keys page does show the .ca subdomain. I was just tripped up last time when I first saw this patch, because their own docs page shows Canadian locale information with the .com script:
https://docs.affirm.com/developers/v1.0-Global-Integration/docs/global-a...
Craziness. Then, even worse, I actually had applied your patch locally but forgot about it, and so in one of my test environments, transactions were working, but in a new one, they weren't, and it took me an hour to realize what I had done. 🤦🏼♂️
Committing this sucker now.
Yeah, I'm just an idiot. : )
Encountered this on another client project who thought they'd sanitized a data set but didn't realize their log table was full of information. I agree, we can at least get the entity delete hook in place, but I think we could also write a more intelligent update hook that executes by default unless disabled by a setting (like we've done for expensive updates in the past), leaving it to sites that suppress the update to clean up their log table on their own time.
A single DELETE query could use a WHERE NOT EXISTS clause vs. trying to construct an array of unknowable large numbers of order IDs. Something like:
DELETE FROM {commerce_log} WHERE NOT EXISTS(
SELECT * FROM {commerce_order} co WHERE co.order_id = {commerce_log}.source_entity_id
);
I reviewed this this evening, and perhaps it didn't work before, but it works now. That said, it's a weird place to use this sort of "selecting none applies to all" behavior. There are only two countries, and it should just be treated as enabling. I'm going to convert this to a task to remove this "none" behavior, but it will require an update hook to convert any configuration with none selected to have both selected. For that, I think we can reserve the change for a new major version.
Retitling for accuracy, making a separate task for project template updating.
Yeah, let's not close this out until we're done with the 1:1 replacement of Commerce Demo with Commerce Kickstart Demo. 😄
Does this patch currently reset things properly when someone exits the Express Checkout Element and goes through normal checkout to pay via the Payment Element? We're getting reports from production usage that someone exiting one path and going to the other causes data loss.
So long as we're able to conditionally add permissions to the roles (i.e., "add if defined" ...) then sure, we can include permissions from the Centarro Certified Projects. But there's also no harm in tagging a 1.0.0 here that just incorporates Commerce Core permissions.
Right, I understood what you meant, just don't have any more information to add. I actually thought there was a current integration to TaxJar, but maybe that's going to be folded into Stripe since the acquisition? No clue. 😬
Committed a simple fix.
Revised the project page but noticed we needed to update the language in the tax types list builder as well.
Hello Mitsuhiko, very sorry to hear about your experience. I did not see this issue at the time, but looking at their website now, it appears they have changed the approach significantly to a "contact us" form without public pricing. I believe this must reflect a changing business strategy on their part that we were not privy to, but we do not have any influence over their pricing or how they present it to Drupal users.
I don't see a "free trial" mentioned anywhere in their current landing page, so we will update the language on drupal.org accordingly.
The old method was intentionally retained in that try statement for backwards compatibility. When it throws an error, then the new method is invoked via the catch. The comments tried to explain this. Thanks for your contribution, though!
I've seen integrations for TaxJar and potentially some payment gateway solutions, but I can't vouch for any of them as I don't have customers on them.
Tested and confirmed both the bug and the fix. Thanks, Tom!
Fixed the validate shipping address checkbox as part of 🐛 HTML::escape() WSOD or "profile cannot be referenced" when manually entering a shipment that popups "Confirm your shipping address" Active .
Committed an update that separates the settings. Turns out it works just fine on the admin form once the label issue is resolved. 😄
Oh, wow. I just realized in testing today that the specific error here is related to the fact that I was attempting (and I assume you were attempting) to create a shipment against a draft order that does not yet have an order number. The Shipping module creates a message with an order number placeholder that cannot by NULL, apparently, though I assume this worked before as casted to an empty string. Not sure.
$this->messenger()->addMessage($this->t('Saved shipment for order @order.', ['@order' => $order->getOrderNumber()]));
In any case, I don't see why shipping doesn't just use the order label. I've changed that in Commerce Shipping here: 🐛 Use the order label when saving a shipment instead of order number Fixed
Since this wasn't actually a bug in AvaTax, I'm reverting this to a feature request specifically to differentiate validating addresses on checkout vs. the backend.
Committed.
rszrama → created an issue. See original summary → .
The checkbox for postal code matching is very confusing, too, because it always matches on postal code, just not necessarily the 9 digit Zip. Thus:
- Match on postal code
- Postal codes are 9 digits, but most people enter the first 5 digits, do you want AvaTax to match all 9 digits?
Should just be a checkbox that reads, "Validate addresses against the full 9-digit Zip code." No description necessary.
Converting this into a meta ticket so we can create child tickets pertaining to each task.
rszrama → created an issue.
rszrama → created an issue.
rszrama → created an issue.
rszrama → created an issue.
Confirming I can reproduce the error. Interestingly enough, it actually works fine if you edit a shipment that already exists. That leads me to believe we could maybe make this work, which would be great - however, it should at least be tied to a different setting. We can't have something that says "validate addresses in checkout" also randomly apply to the admin UI. 🤣
rszrama → created an issue.
I've asked Affirm about this, but the person I was chatting with was unaware of any such requirement. Do you have a link to docs for this I can share with them, or is it something you just happened upon to make it work? (Struggling to onboard a Canadian merchant atm myself.)
Updated along with the module description.
Tagging now. Just needed a good review and the addition of the ability to delete carts from the database.
Well, I've done what I can. Not a huge deal, since this will generally be scripted and forgotten, but since we have abstract ways of truncating entity tables, I didn't think it was worth trying to untangle all that. I've simply updated the lines for deleting carts and tax numbers and tweaked the language slightly for sanitizing orders.
Committed.
Lightly rearranged the class to make this work and added a count of the number of carts deleted.
rszrama → created an issue.
Tested, works great.
Changing our approach in light of recipes.
Changing our approach in light of recipes.
cf. https://www.drupal.org/project/commerce_recipe_user_roles →
Changing our approach in light of recipes.
Changing our approach in light of recipes.
I'm not sure you need the Commerce Kickstart install profile for a new build, though we do want at least the project template to be an accelerator to new projects. Ultimately, you can just start from the core Drupal project template and add the handful of Commerce dependencies as well. There's nothing special inside this for an experienced Drupal developer.
This could be done as part of the recipe conversion: 📌 Convert the full store demo to a recipe Active
We've never had an issue installing this, doing it multiple times per week. What OS are you running?
Yeah, planning on it, just needed to review Roman's patch re: cart deletion.
rszrama → created an issue.
Tested locally and committed.
Merged.
Merged.