Authorize.Net: Get customer profile with merchant_customer_id or email #2690
Conversation
| def get_customer_profile(options) | ||
| requires!(options, :customer_profile_id) | ||
| requires!(options, :customer_profile_id) unless options[:email] || options[:merchant_customer_id] |
There was a problem hiding this comment.
I think this would be better as a straight conditional, rather than three guarded requires! calls. Best as I can figure, what you're trying to do is say you want one of customer_profile_id, email, or merchant_customer_id. In that case, I'd skip requires! entirely (which is just a function that throws ArgumentError in certain conditions) and throw the exception yourself, or do simpler logic. As an example, something like (didn't think through this, not tested, may be wrong, but points in the direction I'm thinking)
raise ArgumentError("Requires one of customer_profile_id, email, or merchant_customer_id") unless options[:customer_profile_id] || options[:email] || options[:merchant_customer_id]
might be a better fit.
There was a problem hiding this comment.
Sounds good, I was just following a pattern I found in create_customer_profile. But that would be a lot cleaner.
| @@ -74,8 +74,17 @@ def test_successful_profile_create_get_update_and_delete | |||
| assert_nil response.authorization | |||
| assert response = @gateway.get_customer_profile(:customer_profile_id => @customer_profile_id) | |||
| assert_nil response.params['profile']['merchant_customer_id'] | |||
| assert_nil response.params['profile']['description'] | |||
| assert_equal 'Up to 255 Characters', response.params['profile']['description'] | |||
There was a problem hiding this comment.
Was this a change in the API?
There was a problem hiding this comment.
I don't know how this ever passed before. The description was being set, so it shouldn't have been asserted as nil.
| assert response.test? | ||
| assert_success response | ||
| assert_nil response.authorization | ||
| assert response = @gateway.get_customer_profile(:email => 'johndoe@test.com') |
There was a problem hiding this comment.
That's not the response; that's the profile. Use a new variable.
| assert response = @gateway.get_customer_profile(:email => 'johndoe@test.com') | ||
| assert_nil response.params['profile']['merchant_customer_id'] | ||
| assert_equal 'Up to 255 Characters', response.params['profile']['description'] | ||
| #assert_equal 'new email address', response.params['profile']['email'] |
| @@ -12,15 +12,17 @@ def setup | |||
| @credit_card = credit_card | |||
| @address = address | |||
| @customer_profile_id = '3187' | |||
| @email = 'johndoe@test.com' | |||
There was a problem hiding this comment.
Do you truly want these in all tests? If not, I'd modify the tests themselves where you want this. The fact you've got optional modifications at https://github.com/activemerchant/active_merchant/pull/2690/files#diff-068964b7f2d4b4c8f35e220595c2ee62R25 makes me think this shouldn't be a global change.
| assert_success response | ||
| assert_equal @email, response.params['email'] | ||
|
|
||
| @gateway.expects(:ssl_post).returns(successful_get_customer_profile_response) |
There was a problem hiding this comment.
I'd prefer to guard these with .when() calls, rather than just adding new .returns() calls at specific junctions in tests, but that's a personal preference.
| @gateway.expects(:ssl_post).returns(successful_get_customer_profile_response) | ||
|
|
||
| assert response = @gateway.get_customer_profile(:email => @email) | ||
| assert_instance_of Response, response |
There was a problem hiding this comment.
This line (and its twin on 358) don't seem to give us anything useful.
|
@bpollack Thank you for all the feedback; I know my style needs improvement and I love that open source projects allow learning like that :) I pushed another commit a few days ago that I think addresses the change requests; if you could take another look and let me know if you see anything else, that would be great! |
|
@nfarve Because you're in authorize.net, hit me you'd be a really good person to tag. This seems like it might be in line with the general work you're doing right now, and the patch looks clean; any thoughts on merging? |
I wanted to use the
AuthorizeNetCim#get_customer_profilemethod in a project and noticed the API supports getting a profile with themerchantCustomerIdoremailin addition to thecustomerProfileId, so I added support for that.