u1-test-utils

Code review comment for lp:~elopio/u1-test-utils/pay_client_return_response

pay_client_return_response
Merge into trunk

Revision history for this message

Leo Arias (elopio) wrote on 2013-03-27:

<matiasb> pindonga: ok, I'll try the fix above (about create) but... if we are passing a CardRequest object as data, can we check using assert_called_with? and related to this, can you take a look at https://code.launchpad.net/~elopio/u1-test-utils/pay_client_return_response? I think we are kind of reinventing the client calls there because of this, by adding a wrapper around the methods to make them more "friendly" (all payclient methods expect a
<matiasb> elopio: ^
<elopio> sounds fair :)
<pindonga> matiasb, you can assert that the card request object was the one passed in
<pindonga> and since you just set the value on that object, it can be assumed it will have the right value
<pindonga> matiasb, you mean that the mp there is duplicating some code by adding this wrapper?
<matiasb> pindonga: ok, I'll take a look, that may be possible; about that META dict, that is required when setting up the payment to add the credit card; the payment requires some session info, and session info is pulled from the request (that's the IP address taken from the request)
<pindonga> ah, ok
<matiasb> pindonga: I mean that it looks like we need to wrap the payclient calls to use them... and it sounds weird, have an API client that we can't use easily here, I think
<pindonga> matiasb, what I don't understand is why you say we *need* to wrap them
<pindonga> surely the api client can be used as it is
<matiasb> pindonga: right, more like a philosophical question, I guess; if you check l.114 of that MP, in this case, we are getting the shop_id, billing address and card info, and then reorganizing all that to create a CardRequest to pass to the client method as data... wondering if it won't be better at some point to be able to pass data as kwargs
<matiasb> or maybe we need more friendly ways to create/initialize the *Request objects (*: Payment, Card, etc)
<nessita> matiasb, pindonga: IMHO, we should tweak the payment client api so kwargs can be passed
<matiasb> I mean, we usually get the billing address, user info, and card info from different sources, but we need to join them all to create a request object to pass as data argument for api calls
<elopio> this are the kind of objects we use in acceptance testing, because we need easy ways to get a unique object and then edit some fields from it.
<nessita> instead of needing to create a FooRequest object each time, outside the call
<elopio> I'm not sure if the same typo of objects would make sense for the real software.
<nessita> elopio, why not?
<matiasb> elopio: yes, if you use the payclient, it will validate you are passing the right kind of object as arg to the method call
<nessita> elopio, as matiasb says, the FooRequest object have to be created by any software wanting to call the payclient library
<elopio> I think I should understand better what's your proposal :)
<elopio> matiasb, nessita: so, you are against the payclient receiving Request objects?
<pindonga> matiasb, I think I understand
<nessita> elopio, the payclient already does that, no?
<pindonga> so having a single CreditCardRequest object is "bad" as it forces us to create the object
<pindonga> it would be better to just have kwargs as params
<pindonga> makes sense to me
<nessita> exactly
<pindonga> I think this was caused by the way piston handles params (or maybe just confusion on our side and we never double-checked)
<pindonga> if we don't lose anything by moving to kwargs +1
<pindonga> makes much more sense
<elopio> but it would receive card_type, card_name, ccv, and the rest?
<matiasb> right, this FooRequest thing is defined and enforced (by piston_mini_client validate decorator) specifically
<elopio> it would be like 8 params
<matiasb> well, I think we should be able to adapt it as needed; the params will be the same (the handler receiving the data won't be changing), but we could rework the client to prepare the data from the params ourselves and not depend on piston_mini_client Requests objects
<matiasb> makes sense? (something like you already did in your wrapper in your MP)
<elopio> I would agree if the arguments are grouped in data objects, as I'm doing on this wrapper.
<elopio> but then the payclient would depend on the u1testutils data objects, that sounds weird.
<elopio> or they would have to duplicate a stripped down version of the data objects.
<matiasb> pindonga: what I'm not sure is if someone is really interacting with pay through payclient (I know U1 has its own client, for example)
<nessita> elopio, I think the idea is that you improve the payclient method instead of doing any wrapper at all in the test urils
<nessita> utils*
<elopio> aghh, my internet connection is so slow today :@
<pindonga> matiasb, our test consumer uses payclient
<pindonga> and SCA also uses payclient
<elopio> nessita: yes, I got that. The part I'm not sure is what's the best way to handle that many number of arguments.
<matiasb> hmm...
<nessita> there will be as many arguments as you have now in your wrapper, no?
<nessita> elopio, ^
<elopio> nessita: on the wrapper I'm grouping the arguments on data objects. That's the clearer way to do it.
<elopio> But where would the data objects recide? in which project?
<nessita> elopio, payclient, obviously
<nessita> they are generic objects, no?
<nessita> abstracting real pieces of information
<matiasb> right
<matiasb> I didn't want to break lot of things, but wondering if this was decided like this, or there were particular reasons to prefer this approach :); I guess this can wait for a refactoring, if decided
<elopio> well, their original purpose is to be test objects. To model pieces of information as the user perceives them.
<elopio> sorry, I'm writing slow because I get your messages with a huge dealy.
<elopio> so the obvious place for them is as test helpers for pay. But we can have data objects on payclient
<elopio> and extend them on u1-test-utils to add the uuid stuff that the api client will never use.
<matiasb> I think the idea would be to have something like data objects for each logical grouping of fields, in payclient, and those should be usable from tests too; this may need more thinking though
<pindonga> matiasb, elopio we can keep the data= param for objects
<pindonga> and add support for other params additionally
<pindonga> however it's not clear what happens if you mix them
<matiasb> ack
<elopio> actually, here we have 4 parameters: shop_id, user, credit_card and billing_address
<elopio> according to the clean code book, what we should do with more than 3 arguments is to group them in an object.
<elopio> that object would be a CreditCardRequest, just a composed object instead of a dict :)
<pindonga> matiasb, elopio instead of creating a CreditCardRequest object
<pindonga> you could directly call the api like, api.new_credit_card(data=mydict)
<pindonga> and have the conversion be done internally in the payclient?
<matiasb> pindonga: that should be possible, yes; right now the validate decorator is enforcing the passed parameter is a CreditCardRequest object
<elopio> pindonga: but what I like is the other contrary:
<elopio> api.new_credit_card(shop_id='...', user=utils.create_new_user(), credit_card=utils.create_new_card(), billing_address=utils.create_new_address())
<pindonga> elopio, you'd like or not? not sure (as you just said that with more than 3 params we shoudl bundle into an obj)
<elopio> request = CreditCardRequest(shop_id='...', user=utils.create_new_user(), credit_card=utils.create_new_card(), billing_address=utils.create_new_address())
<elopio> api.new_credit_card(request)
<elopio> pindonga: that would be something like ^
<pindonga> elopio, so basically the same but without data= ?
<pindonga> elopio, most other api calls need the payment_id to be passed in too
<pindonga> elopio, and I think piston forces us to pass all params as named params
<pindonga> we can't do api.new_credit_card(request)
<pindonga> bc of that
<elopio> pindonga: no, what's different is how you create the CreditCardRequest
<pindonga> bottom line is I don't have any personal objection to changing the api, provided
<pindonga> a) we keep it backwards compatible
<pindonga> b) we improve it
<pindonga> c) it works
<pindonga> :)
<matiasb> +1, heh :)
<elopio> it could be api.new_credit_card(data=request)
<matiasb> but it sounds like it could require some discussion
<elopio> it's sad in this case that we can't add an alternate constructor for CreditCardRequest
<elopio> but we could add a constructor method.
<elopio> I can make a proposal for u1-test-utils and payclient today.
<elopio> but I'm leaving for vacations tomorrow, and this MP was intended to fix a bug I introduced in the acceptance test.
<elopio> so what should we do now?
<matiasb> elopio: meanwhile, re your MP, and assuming we keep this for now, I would change the payclient inheritance, and having a payclient instance as instance attribute of the u1testutils pay client instead
<matiasb> (to avoid potential name conflicts between payclient and custom methods, and confusions)
<matiasb> elopio: and if needed, you can leave me some instructions to get what is missing (I'll be here tomorrow and Friday)
<elopio> matiasb: ack. Thanks.
<elopio> So it would look almost like before, but we had an interesting conversation :)

<matiasb> pindonga: ok, I'll try the fix above (about create) but... if we are passing a CardRequest object as data, can we check using assert_called_with? and related to this, can you take a look at https://code.launchpad.net/~elopio/u1-test-utils/pay_client_return_response? I think we are kind of reinventing the client calls there because of this, by adding a wrapper around the methods to make them more "friendly" (all payclient methods expect a
<matiasb> elopio: ^
<elopio> sounds fair :)
<pindonga> matiasb, you can assert that the card request object was the one passed in
<pindonga> and since you just set the value on that object, it can be assumed it will have the right value
<pindonga> matiasb, you mean that the mp there is duplicating some code by adding this wrapper?
<matiasb> pindonga: ok, I'll take a look, that may be possible; about that META dict, that is required when setting up the payment to add the credit card; the payment requires some session info, and session info is pulled from the request (that's the IP address taken from the request)
<pindonga> ah, ok
<matiasb> pindonga: I mean that it looks like we need to wrap the payclient calls to use them... and it sounds weird, have an API client that we can't use easily here, I think
<pindonga> matiasb, what I don't understand is why you say we *need* to wrap them
<pindonga> surely the api client can be used as it is
<matiasb> pindonga: right, more like a philosophical question, I guess; if you check l.114 of that MP, in this case, we are getting the shop_id, billing address and card info, and then reorganizing all that to create a CardRequest to pass to the client method as data... wondering if it won't be better at some point to be able to pass data as kwargs
<matiasb> or maybe we need more friendly ways to create/initialize the *Request objects (*: Payment, Card, etc)
<nessita> matiasb, pindonga: IMHO, we should tweak the payment client api so kwargs can be passed
<matiasb> I mean, we usually get the billing address, user info, and card info from different sources, but we need to join them all to create a request object to pass as data argument for api calls
<elopio> this are the kind of objects we use in acceptance testing, because we need easy ways to get a unique object and then edit some fields from it.
<nessita> instead of needing to create a FooRequest object each time,  outside the call
<elopio> I'm not sure if the same typo of objects would make sense for the real software.
<nessita> elopio, why not?
<matiasb> elopio: yes, if you use the payclient, it will validate you are passing the right kind of object as arg to the method call
<nessita> elopio, as matiasb says, the FooRequest object have to be created by any software wanting to call the payclient library
<elopio> I think I should understand better what's your proposal :)
<elopio> matiasb, nessita: so, you are against the payclient receiving Request objects?
<pindonga> matiasb, I think I understand
<nessita> elopio, the payclient already does that, no?
<pindonga> so having a single CreditCardRequest object is "bad" as it forces us to create the object
<pindonga> it would be better to just have kwargs as params
<pindonga> makes sense to me
<nessita> exactly
<pindonga> I think this was caused by the way piston handles params (or maybe just confusion on our side and we never double-checked)
<pindonga> if we don't lose anything by moving to kwargs +1
<pindonga> makes much more sense
<elopio> but it would receive card_type, card_name, ccv, and the rest?
<matiasb> right, this FooRequest thing is defined and enforced (by piston_mini_client validate decorator) specifically
<elopio> it would be like 8 params
<matiasb> well, I think we should be able to adapt it as needed; the params will be the same (the handler receiving the data won't be changing), but we could rework the client to prepare the data from the params ourselves and not depend on piston_mini_client Requests objects
<matiasb> makes sense? (something like you already did in your wrapper in your MP)
<elopio> I would agree if the arguments are grouped in data objects, as I'm doing on this wrapper.
<elopio> but then the payclient would depend on the u1testutils data objects, that sounds weird.
<elopio> or they would have to duplicate a stripped down version of the data objects.
<matiasb> pindonga: what I'm not sure is if someone is really interacting with pay through payclient (I know U1 has its own client, for example)
<nessita> elopio, I think the idea is that you improve the payclient method instead of doing any wrapper at all in the test urils
<nessita> utils*
<elopio> aghh, my internet connection is so slow today :@
<pindonga> matiasb, our test consumer uses payclient
<pindonga> and SCA also uses payclient
<elopio> nessita: yes, I got that. The part I'm not sure is what's the best way to handle that many number of arguments.
<matiasb> hmm...
<nessita> there will be as many arguments as you have now in your wrapper, no?
<nessita> elopio, ^
<elopio> nessita: on the wrapper I'm grouping the arguments on data objects. That's the clearer way to do it.
<elopio> But where would the data objects recide? in which project?
<nessita> elopio, payclient, obviously
<nessita> they are generic objects, no?
<nessita> abstracting real pieces of information
<matiasb> right
<matiasb> I didn't want to break lot of things, but wondering if this was decided like this, or there were particular reasons to prefer this approach :); I guess this can wait for a refactoring, if decided
<elopio> well, their original purpose is to be test objects. To model pieces of information as the user perceives them.
<elopio> sorry, I'm writing slow because I get your messages with a huge dealy.
<elopio> so the obvious place for them is as test helpers for pay. But we can have data objects on payclient
<elopio> and extend them on u1-test-utils to add the uuid stuff that the api client will never use.
<matiasb> I think the idea would be to have something like data objects for each logical grouping of fields, in payclient, and those should be usable from tests too; this may need more thinking though
<pindonga> matiasb, elopio we can keep the data= param for objects
<pindonga> and add support for other params additionally
<pindonga> however it's not clear what happens if you mix them
<matiasb> ack
<elopio> actually, here we have 4 parameters: shop_id, user, credit_card and billing_address
<elopio> according to the clean code book, what we should do with more than 3 arguments is to group them in an object.
<elopio> that object would be a CreditCardRequest, just a composed object instead of a dict :)
<pindonga> matiasb, elopio instead of creating a CreditCardRequest object
<pindonga> you could directly call the api like, api.new_credit_card(data=mydict)
<pindonga> and have the conversion be done internally in the payclient?
<matiasb> pindonga: that should be possible, yes; right now the validate decorator is enforcing the passed parameter is a CreditCardRequest object
<elopio> pindonga: but what I like is the other contrary:
<elopio> api.new_credit_card(shop_id='...', user=utils.create_new_user(), credit_card=utils.create_new_card(), billing_address=utils.create_new_address())
<pindonga> elopio, you'd like or not? not sure (as you just said that with more than 3 params we shoudl bundle into an obj)
<elopio> request = CreditCardRequest(shop_id='...', user=utils.create_new_user(), credit_card=utils.create_new_card(), billing_address=utils.create_new_address())
<elopio> api.new_credit_card(request)
<elopio> pindonga: that would be something like ^
<pindonga> elopio, so basically the same but without data= ?
<pindonga> elopio, most other api calls need the payment_id to be passed in too
<pindonga> elopio, and I think piston forces us to pass all params as named params
<pindonga> we can't do api.new_credit_card(request)
<pindonga> bc of that
<elopio> pindonga: no, what's different is how you create the CreditCardRequest
<pindonga> bottom line is I don't have any personal objection to changing the api, provided
<pindonga> a) we keep it backwards compatible
<pindonga> b) we improve it
<pindonga> c) it works
<pindonga> :)
<matiasb> +1, heh :)
<elopio> it could be api.new_credit_card(data=request)
<matiasb> but it sounds like it could require some discussion
<elopio> it's sad in this case that we can't add an alternate constructor for CreditCardRequest
<elopio> but we could add a constructor method.
<elopio> I can make a proposal for u1-test-utils and payclient today.
<elopio> but I'm leaving for vacations tomorrow, and this MP was intended to fix a bug I introduced in the acceptance test.
<elopio> so what should we do now?
<matiasb> elopio: meanwhile, re your MP, and assuming we keep this for now, I would change the payclient inheritance, and having a payclient instance as instance attribute  of the u1testutils pay client instead
<matiasb> (to avoid potential name conflicts between payclient and custom methods, and confusions)
<matiasb> elopio: and if needed, you can leave me some instructions to get what is missing (I'll be here tomorrow and Friday)
<elopio> matiasb: ack. Thanks.
<elopio> So it would look almost like before, but we had an interesting conversation :)

« Back to merge proposal