Allow to specify if billing address is required for orders

[softr8]

With the implementation of default bill address, we have found that some legacy users are going thought the checkout process without billing address failing at the confirm step, this change makes sure that billing address is present.

I am not totally sure if this should be controlled via config variable but I'm assuming all payment methods require billing address

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• I have added tests to cover this change (if needed)

[houndci-bot]

Style/SafeNavigation: Use safe navigation (&.) instead of checking if an object exists before calling the method.

[softr8]

Should I fix this? it is barely used in the project

[jarednorman]

I think it's worth fixing as it's the preferred way to do that now that we don't need to support any old Rubies from before that syntax was added.

[jarednorman]

All payment methods don't necessarily require a billing address. I work with some stores that don't even track billing addresses, so this definitely qualifies as a breaking change that would need to be controlled via configuration.

[softr8]

All payment methods don't necessarily require a billing address. I work with some stores that don't even track billing addresses, so this definitely qualifies as a breaking change that would need to be controlled via configuration.

Right, giving a second thought, it might be worth delegating this decision to the payment methods, if at least one of them require the address, then make it required before payment step

what about something like

def billing_address_required
 payment_methods.detect {|pm|| pm.require_billing_address }
end

def ensure_billing_address
  return unless billing_address_required
  return unless bill_address&.vaild?

  errors.add(:base, I18n.t('spree.bill_address_required'))
  false
end

Then each payment method is responsible for it

[aldesantis]

@softr8 I like the idea on a theoretical level, but I think that may cause problems with stores doing weird stuff (e.g. offering different payment methods depending on some conditions on the order, which may or may not change after the address step).

I'd feel more comfortable with having an app-level configuration toggle.

If you want to make it super-flexible, you could then also add a method to the order class:

def billing_address_required?
  Spree::Config.billing_address_required
end

That way, it's easy to override the behavior per-order, if needed.

@jarednorman what do you think?

[softr8]

@softr8 I like the idea on a theoretical level, but I think that may cause problems with stores doing weird stuff (e.g. offering different payment methods depending on some conditions on the order, which may or may not change after the address step).

That was exactly my point, get the available payment methods for the order and delegate the validation to them, will try another attempt to see how it looks

[jarednorman]

I think @aldesantis's suggestion was to avoid adding the payment method check to core here, but instead have that method delegate to the configuration option so that if you want finer grained control (like if you want to delegate to the payment methods like you have) you can override Spree::Order#billing_address_required?.

I like that solution.

[aldesantis]

Correct: I think we should avoid doing anything with the payment methods on the order, because it seems too brittle and prone to breaking as a new behavior to introduce in existing stores. Instead, we can use global flag and allow people to override it on Spree::Order if they want to implement any custom logic, such as the one originally implemented by @softr8.

[softr8]

Alright! I guess I tried to port our implementation for everyone but it is not that common, we have payment methods per zone (pay on delivery for example) and it does not require billing address.

[jarednorman]

Yeah, I'm happy with this.

[kennyadsl]

Thanks!