Closed (fixed)
Project:
Commerce Swiss Post
Version:
1.0.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
23 Mar 2023 at 15:08 UTC
Updated:
22 Jun 2023 at 16:44 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
primsi commentedInitial patch.
Comment #3
primsi commentedComment #5
primsi commentedAddressing coding standard issues.
Comment #7
primsi commentedAddressed test fail and added loging.
Comment #8
luksakPrimsi thank you for your contribution! I'm currently working on other projects. I'm going to have a look into this and provide feedback soon. I can't provide an ETA tough.
Comment #9
berdirNo worries, Primsi is working with this on me and I'll review it.
We've developed this before for a client project and are open-sourcing it now, basically.
Comment #10
luksakOk, great! Thank you.
One thing a bit off-topic: Could we extend this to support something similar to https://www.drupal.org/project/address_autocomplete_gmaps ?
Comment #11
berdirThe API we use does have autocomplete endpoints too, so yes. There is a street endpoint that will return suggestions for a full address.
That's not in scope of what we're doing here, but a follow-up could build on that.
The module does implement that by providing its own widget. I do wonder if that would be a more sensible approach to the magic we try to do here right now, would also give us a place to put options, like an autocomplete yes/no checkbox.
Comment #12
luksak@berdir sure, a follow-up makes more sense.
Exactly, that's why I suggested it here. But the autocomplete approach also comes with a lot of disadvantages, if solved poorly and can result in a worse UX than before.
address_autocomplete_gmapsis actually a good example. It breaks the address form's UX quite a bit. For example editing the values again is quite cumbersome.There is a typo in the service's name: `commerce_swiss_post.adress_verification` It should be: `commerce_swiss_post.address_verification`.
Comment #13
primsi commentedImplemented the idea from #11 and fixed the naming issue from #12
Comment #14
primsi commentedForgot to mention in the previous comment: also added the ability to skip validation with a double submission of the same address.
Here adding a basic webform validation handler. Not sure if we want to add he auto-correcting functionality here as well.
Comment #15
berdirI think we want the same behavior there, auto-correct and also the resubmit part. Otherwise looks good I think.
Comment #16
primsi commentedAdding that
Comment #17
luksakOne more question; Do we really need sessions for this? Can't the re-submission functionality be achieved with form state? It feels weird to store all addresses in sessions...
Comment #18
primsi commentedI did try storing it in form state, but then, this gets passed to later steps, which would need to be handled. With session there is no such issue. But I am open to try again if we think this is better.
Comment #19
primsi commentedFixing and improving stuff from internal testing.
1. Fix log levels not being correctly used
2. Rephrase debug messages.
3. Fix issue where a wrong address can be submitted if limit_validation_errors was used because the old imput was stored
Comment #21
primsi commentedRemoving the cron bit unintentionally added to the patch.
Comment #22
primsi commentedAs per discussion with @Berdir, readdig the cron bit with the correct module name (not entity type name as before).
Comment #24
berdirThis was tested and does what it's meant to. There are some possible additional feature and improvements, like configurable action on returned changes (some are a bit strange) as well as supporting autocomplete in some form as well, but we can look into that if someone is interested in sponsoring that.