Pattern help for factory - review this idea for changing core.cfc

[Brian G]

Shawn's GoEmerchant gateway has a customized creditcard object that has an additional method for getCardType().

At the moment, the core.cfc factory has a hardcoded path to generating the creditcard object from cfpayment.api.model.creditcard.  I'd like to make it such that each gateway can extend the base model objects as needed to provide helper methods.   What I am thinking of doing is changing the factory methods in core.cfc from something like:

  <cfreturn createObject("component", "model.creditcard").init(argumentCollection = arguments) />

to:

  <cfif fileExists("gateway.#cfg.path#.model.creditcard")>

      <cfreturn createObject("component", "gateway.#cfg.path#.model.creditcard").init(argumentCollection = arguments) /> 

  <cfelse>

      <cfreturn createObject("component", "model.creditcard").init(argumentCollection = arguments) /> 

  </cfif>

It could also be part of the gateway configuration instead of a fileExists() call, but I like the idea of doing it by convention rather than configuration.

Thoughts?   Any alternative approaches?

Brian

[Phil Cruz]

Another option is to have the factory methods in the base.cfc, then the gateways could override that to return gateway specific model object as needed. Not that it's a big deal, but it would be slightly cleaner for the application to only have to deal with a gateway instance as opposed to having to deal with the gateway and the core object.

-Phil

--
You received this message because you are subscribed to the Google Groups "cfpayment" group.
To view this discussion on the web visit https://groups.google.com/d/msg/cfpayment/-/ZiyVXb9Og24J.
To post to this group, send email to cfpa...@googlegroups.com.
To unsubscribe from this group, send email to cfpayment+...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/cfpayment?hl=en.

[Brian G]

Phil, thanks!  Each gateway has the core service injected, what if we added methods like the following to base.cfc?

<cffunction name="createResponse" output="false" access="public" returntype="any">

<cfreturn getService().createResponse(argumentCollection = arguments) />

</cffunction>

And then each gateway could override that as necessary?  There's no reason we couldn't move the calls down to the base.cfc except that people already have implemented with using the core so this way both would work but also provide a clean path for per-gateway overrides?

Brian

[Phil Cruz]

I understand it's good to provide some compatibility for existing implementations but in the long run I think it would be slightly cleaner to deprecate the methods in the core and encourage people to use the gateway methods going forward. and the base.createResponse() like

<cffunction name="createResponse" output="false" access="public" returntype="any">

<cfreturn createObject("component", "cfpayment.model.Response").init(argumentCollection = arguments) />

</cffunction>

But it's not a big deal, so either way is fine.

-Phil

Brian

--

You received this message because you are subscribed to the Google Groups "cfpayment" group.

To view this discussion on the web visit https://groups.google.com/d/msg/cfpayment/-/7H76KVzzJC0J.

[Brian G]

Back from the holidays and thinking about this a little bit... is it
valuable to be able to create creditcards and other objects without
having a gateway? For example, maybe you don't intend to actually
process any transactions but you want to verify a card number is valid
(populate the card object and run validate())? Probably a slightly
esoteric example (although perhaps one that would be useful for unit
testing) but if that's good, then just beyond backwards compatibility,
letting the core continue to generate them might have some use?

I agree with your cleaner comments... but am loathe to make people
change code. Of course, now's the time if there is one as we prep
1.0. I'll have to look in my code to see how I create the creditcards
and whether or not I could S&R it to use the gateway instead.

Brian

[Phil Cruz]

I think that scenario is a bit esoteric. In terms of testing, ideally,
I would prefer there was only one way to get a credit card object (via
the gateway) and not have to test/support multiple methods.

But I understand wanting backwards compatibility, that's why I
suggested we keep them in there for 1.0 but mark as deprecated. No one
has to change code now, but going forward the "official" method would
be to get objects via the gateway. Sometime down the road, after there
has been plenty of time to change code, we can remove the core
methods.

-Phil

[Shawn Mckee]

Back to "questions from the clueless." 

Are we talking about moving the "creator" functions out of the core and into the gateways or completely eliminating the core code? The former I understand the latter I don't. With the former though the getGateway and getVersion functions would need some tweaks so the core knows which instantiation we are talking about. Moving the getStatus functions out worries me in that it makes it easier for people to decide to change the values and suddenly there is no longer a cohesive system. Now I'm not big on protecting people from themselves but...

Shawn Mckee
clinicApps.com
http://www.clinicapps.com

[Phil Cruz]

We're not talking about eliminating core code. We're talking about moving the "create" functions 

<cffunction name="createCreditCard" >

<cffunction name="createEFT" >

<cffunction name="createToken" >

<cffunction name="createResponse" >

<cffunction name="createMoney" >

from the core.cfc to the base.cfc where they can be overridden as needed. There would be no changes to getStatus or any "get" functions.

-Phil 

[Shawn Mckee]

OK, cool, that I understand and agree with.

Shawn Mckee
clinicApps.com
http://www.clinicapps.com

[Brian G]

On Jan 3, 8:45 am, Phil Cruz <p...@philcruz.com> wrote:
> We're not talking about eliminating core code. We're talking about moving
> the "create" functions

> from the core.cfc to the base.cfc where they can be overridden as needed.
> There would be no changes to getStatus or any "get" functions.

I did some research this morning to see how ActiveMerchant (Ruby)
handles this. I based cfpayment largely on their system so we would
have something equally useful. They have about 80 gateways it seems so
I figured they must have run into this before too. They have one kind
of key difference which impacts how this per-gateway override needs to
work.

First, CreditCard objects are defined by ActiveMerchant::Billing which
is like getting it from the cfpayment core (and not the gateway). I
think this makes sense as we don't want to extend the credit card
object itself if we can avoid it so it can be passed to any gateway.
The gateway should handle any gateway-specific enhancements to the
base data. The example that prompted this was Shawn needing a
getCardType() routine that would return a specific formatting of a
long or short card brand name ("MasterCard" or "Master Card" for
example).

This however can be handled by folding in the getCardType() method to
geomerchant.cfc and have it take the account as an argument. It's a
helper method specific to sending data to the gateway so it belongs in
the gateway (IMHO). I have a handful in my Braintree gateway too,
like dateToBraintree() and braintreeToDate(). I think the idea of
keeping the generic objects clean and "top level" is important to the
overall goal of making it as easy to switch consuming code between
gateways with minimal changes so that your application code doesn't
need to know anything more than purchase(account, money, options).

Second, I looked at how ActiveMerchant handles the response because
the need to override the response looks to be far more likely to
handle output from different kinds of processing systems. AM has a
subtle difference to cfpayment that I think bears evaluation. So,
let's walk through how their gateways post and parse results.

ActiveMerchant also has the idea of a super.process() called
commit(). In that commit(), they also submit the HTTP request. It
looks something like this (in typical Ruby-ish minimalism):

response = parse(action, ssl_post(endpoint_url, request, options))

That response is not actually a Response object; the result of
ssl_post() is run through parse(), which takes whatever response the
gateway sends back (name-value pairs, XML, JSON, etc) and converts it
into a structure/hash. Each gateway is responsible instantiating the
Response object and populating it:

Response.new(:authorization => response["transactionid"],
:test => test,
:cvv_result => response["cvvresponse"],
:avs_result => response["avsresponse"] )

This is what our gateways do where we analyze the result from the
gateway, pick out the various parameters and normalize them in the
response object. The difference is that our super.process() is
creating the Response object in base.cfc where it can't be overridden
and passing it back. This was part of the problem with Phil's stripe
implementation which needed to act upon the HTTP status code; in this
approach, he would have no problem making adjustments based on the
HTTP status code. If we used the ActiveMerchant approach, it would
also be trivial to override the response object if it's really needed
(we ultimately decided that it wasn't in Shawn's GEM case).
ActiveMerchant seems to provide extended response objects primarily
for integration-style gateways like Paypal so it's not common in
practice.

In my own vacuum, where I'm leaning right now would be to follow
ActiveMerchant and refactor the base gateway so that an individual
gateway implementation (braintree, gem, stripe, etc) would look like
this in process():

<cffunction name="process">
// either use the newly inherited createResponse() from base.cfc or
createObject() to use a custom one
<cfset Response = createResponse() />
...
// return a structure of values including the gateway response, the
result status, and original request
<cfset res = super.process(payload = p) />

// normalize values inline or sub out gateway parsing to a helper
function
<cfset normalized_values = parse(res) />

<cfset Response.init(response = res
,TransactionID = normalized_values.TransactionID
,Authorization = normalized_values.Authorization
,AVSCode = normalized_values.AVSCode
...);

// OR
<cfset Response.setTransactionID(normalized_values.TransactionID) />
<cfset Response.setAuthorization(normalized_values.Authorization) />
<cfset Response.setAVSCode(normalized_values.AVSCode) />

<cfreturn Response />

</cffunction>

The base.cfc would be refactored so that process() returns a structure
rather than a Response object. The Response object would be made
smarter to know how to take the keys in that structure and populate
the majority of the parameters. The normalization would continue to
be handled on a per-gateway basis like it is now.

Net result... Inputs come from core.cfc and Outputs come from the
gateways:

createCreditCard, createEFT, createToken and createMoney methods stay
in Core.cfc (mirrors ActiveMerchant::Billing).
createResponse method is moved to base.cfc (the output of a gateway).
base.cfc's process() would return a structure of request/response data
gateway CFCs would only need to add one line of code basically to
createResponse() inline and init using the results of the
super.process().
response.cfc would know how to take that structure in init() and
populate the core values (status, requestdata, test mode, etc),
leaving the gateway implementation to populate the rest of the values
like they do today.

It would be optional to sub out the normalization to a parse() method
(although I like the idea of that as a recommendation).

If were making code changes, I think this sounds like a better long-
term strategy for 1.0. Thoughts?


Brian

[Phil Cruz]

That all sounds good and makes sense. 

While we are making changes, I noticed that AM moved away from using a Money object

https://groups.google.com/forum/#!searchin/activemerchant/money$20as$20integer/activemerchant/-9jO7FUjpNg/CsxzsGmBVhYJ 

We're getting rid of the Money object in ActiveMerchant because it is
an additional moving part that you don't gain very much benefit from.
We use our own custom Money object in Shopify and then just pass the
integer value to ActiveMerchant for payment processing. We find the
abstraction of Money great in Shopify, but cumbersome in
ActiveMerchant. ActiveMerchant doesn't need to add, subtract, divide,
or operate on Money at all, so it doesn't provide any benefit other
than complexity and an additional dependency. 

Perhaps we should follow their lead here as well?

-Phil

Brian

--
You received this message because you are subscribed to the Google Groups "cfpayment" group.

[Brian G]

On Jan 3, 1:37 pm, Phil Cruz <p...@philcruz.com> wrote:
> While we are making changes, I noticed that AM moved away from using a
> Money object
>

> https://groups.google.com/forum/#!searchin/activemerchant/money$20as$...

I don't feel it's really a pain to use... although I can see why just
a straight amount might be easier. One less moving piece?

That would require end-user code change (vs. gateway implementation
change) though as it changes the public API, and quite a bit of it.

Brian