belighted/active_merchant — comment on commit ee6167645d1400edd2b72f39bb2de1ca0a6930fc
File: lib/active_merchant/billing/gateways/ogone.rb line 158
By ZenCocoon:

I would rather use `void(response.authorization)` here to be more future proof. As personal choice I would also go with `response` instead of `res` but that's more of a personal choice to avoid abbreviation when it remains short.

One big part missing though, is validation. What append if the authorization is refused ? We need to check the response status before the `void`, I don't think that checking the status of the void itself matters much though, as even if an error occurs here, the authorization will be automatically waived after maximum 30 days (might depend on the bank).

I could then start with the following refactoring:

def store(payment_source, options = {})
response = authorize(1, payment_source, :store => options[:billing_id].to_s)
void(response.authorization) if response.success?
response
end

We still need to deal with blank alias, if the billing_id option is missing, we could generate a unique one, what do you think ?

`ActiveMerchant::Utils::generate_unique_id[0...30]` if an option I use.

In case we create an alias, how to return it as well as the response ? We return the alias on successful response otherwise the response object on failure ?

# Store() will return the billing_id (alias) if everything went fine, otherwise the response object will be returned.
def store(payment_source, options = {})
billing_id = (options[:billing_id] || ActiveMerchant::Utils::generate_unique_id[0...30]).to_s
response = authorize(1, payment_source, :store => billing_id)
response unless response.success?
void(response.authorization)
billing_id
end

Let me know how does that feel. ;-)