Merge lp:~elopio/u1-test-utils/pay_client_return_response into lp:u1-test-utils

[Leo Arias]

Return the response from the pay api client, instead of a boolean.

With this change, the pay API test will be able to use the helpers, reducing duplication.
This branch requires that https://code.launchpad.net/~elopio/payclient/creditcardrequest_equality/+merge/155637 is merged first.

[lp:~elopio/u1-test-utils/pay_client_return_response]

43. By Leo Arias on 2013-03-27

Do not inherit from PaymentServiceAPI, use it as an attribute instead.

[Matias Bordese (matiasb)]

+1

review: Approve

[Leo Arias (elopio)]

<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
<pindon...

=== modified file 'requirements.txt'
--- requirements.txt	2013-03-26 05:16:33 +0000
+++ requirements.txt	2013-03-27 20:56:21 +0000
@@ -5,6 +5,6 @@
 pyflakes
 Twisted
 bzr+ssh://bazaar.launchpad.net/~bloodearnest/localmail/trunk
-bzr+http://bazaar.launchpad.net/~ubuntuone-hackers/payclient/trunk@1
+bzr+http://bazaar.launchpad.net/~ubuntuone-hackers/payclient/trunk@3
 bzr+http://bazaar.launchpad.net/~canonical-isd-qa/selenium-simple-test/trunk@367
 bzr+http://bazaar.launchpad.net/~canonical-isd-hackers/canonical-identity-provider/ssoclient@3

=== modified file 'u1testutils/pay/__init__.py'
--- u1testutils/pay/__init__.py	2013-03-26 05:48:26 +0000
+++ u1testutils/pay/__init__.py	2013-03-27 20:56:21 +0000
@@ -0,0 +1,23 @@
+# Copyright 2013 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU Lesser General Public License version 3, as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+__all__ = [
+    'APIClient',
+    'data',
+    'environment',
+]
+
+import data
+import environment
+from client import APIClient

=== modified file 'u1testutils/pay/client.py'
--- u1testutils/pay/client.py	2013-03-26 06:19:46 +0000
+++ u1testutils/pay/client.py	2013-03-27 20:56:21 +0000
@@ -14,83 +14,67 @@
 
 import payclient
 from django.conf import settings
-from piston_mini_client import auth, failhandlers
 
 import u1testutils.pay.environment
-
-
-def get_pay_client(pay_server_url=None):
-    """Return the Payment Service API client.
-
-    pay_server_url -- The URL of the Pay server that provides the API. Default
-        is None, which means that the URL will be taken from value of the
-        Django settings PAY_SERVER_URL option.
+from piston_mini_client import auth
+
+
+class APIClient(object):
+    """API client helper for payment tests.
+
+    It extends the PaymentServiceAPI to take the credentials from the Django
+    configuration file and to make it easier for the tests to call the API
+    methods.
 
     """
-    user = settings.PAY_API_USERNAME
-    password = settings.PAY_API_PASSWORD
-    basic_auth = auth.BasicAuthorizer(user, password)
-    pay_server_url = _get_pay_server_url(pay_server_url)
-    pay_api_url = '{0}/api/2.0/'.format(pay_server_url)
-    return payclient.PaymentServiceAPI(
-        auth=basic_auth, service_root=pay_api_url)
-
-
-def _get_pay_server_url(pay_server_url=None):
-    if pay_server_url is None:
-        pay_server_url = \
-            u1testutils.pay.environment.get_pay_base_url()
-    else:
-        pay_server_url = pay_server_url.rstrip('/')
-    return pay_server_url
-
-
-def add_new_credit_card(shop_id, user, credit_card, billing_address,
-                        pay_server_url=None):
-    """Add a new credit card through the Pay API.
-
-    Keyword arguments:
-    shop_id -- The identifier of the shop that will add the credit card.
-    user -- The user that will have the new credit card. It must have the
-        openid attribute.
-    credit_card -- The credit card information. It must have the attributes
+
+    def __init__(self, pay_server_url=None):
+        pay_server_url = self._get_pay_server_url(pay_server_url)
+        user = settings.PAY_API_USERNAME
+        password = settings.PAY_API_PASSWORD
+        basic_auth = auth.BasicAuthorizer(user, password)
+        pay_api_url = '{0}/api/2.0/'.format(pay_server_url)
+        self.client = payclient.PaymentServiceAPI(
+            auth=basic_auth, service_root=pay_api_url)
+
+    def _get_pay_server_url(self, pay_server_url=None):
+        if pay_server_url is None:
+            pay_server_url = u1testutils.pay.environment.get_pay_base_url()
+        else:
+            pay_server_url = pay_server_url.rstrip('/')
+        return pay_server_url
+
+    def add_new_credit_card(self, shop_id, user, credit_card, billing_address):
+        """Add a new credit card through the Pay API.
+
+        Keyword arguments:
+        shop_id -- The identifier of the shop that will add the credit card.
+        user -- The user that will have the new credit card. It must have the
+            openid attribute.
+        credit_card -- The credit card information. It must have the attributes
         card_type,  name_on_cad, card_number, ccv_number, expiration_month and
-        expiration_year.
-    billing_address -- The billing address for the card. It must have the
-        attributes street_line1, state, country and postal_code. The country
-        is the name of the country in english.
-    pay_server_url -- The URL of the Pay server that provides the API. Default
-        is None.
-
-    """
-    billing_address_dict = {
-        'street': billing_address.street_line1,
-        'state': billing_address.state,
-        'country': _get_country_code(billing_address.country),
-        'postal': billing_address.postal_code
-    }
-    kwargs = dict(
-        shop_id=shop_id,
-        open_id=user.openid,
-        card_type=credit_card.card_type,
-        card_holder=credit_card.name_on_card,
-        card_number=credit_card.card_number,
-        card_ccv=credit_card.ccv_number,
-        card_expiration_month=credit_card.expiration_month,
-        card_expiration_year=credit_card.expiration_year,
-        billing_address=billing_address_dict
-    )
-    credit_card_request = payclient.CreditCardRequest(**kwargs)
-    pay_client = get_pay_client(pay_server_url)
-    try:
-        response = pay_client.new_credit_card(data=credit_card_request)
-        return isinstance(response, payclient.CreditCardResponse)
-    except failhandlers.APIError:
-        # TODO log the error.
-        return False
-
-
-def _get_country_code(country_name):
-    # TODO add more?
-    country_data = {'United States': 'US'}
-    return country_data.get(country_name)
+            expiration_year.
+        billing_address -- The billing address for the card. It must have the
+            attributes street_line1, state, country and postal_code. The
+            country is the name of the country in english.
+
+        """
+        billing_address_dict = {
+            'street': billing_address.street_line1,
+            'state': billing_address.state,
+            'country': billing_address.get_country_code(),
+            'postal': billing_address.postal_code
+        }
+        kwargs = dict(
+            shop_id=shop_id,
+            open_id=user.openid,
+            card_type=credit_card.card_type,
+            card_holder=credit_card.name_on_card,
+            card_number=credit_card.card_number,
+            card_ccv=credit_card.ccv_number,
+            card_expiration_month=credit_card.expiration_month,
+            card_expiration_year=credit_card.expiration_year,
+            billing_address=billing_address_dict
+        )
+        credit_card_request = payclient.CreditCardRequest(**kwargs)
+        return self.client.new_credit_card(data=credit_card_request)

=== modified file 'u1testutils/pay/data.py'
--- u1testutils/pay/data.py	2013-03-26 05:48:26 +0000
+++ u1testutils/pay/data.py	2013-03-27 20:56:21 +0000
@@ -69,3 +69,8 @@
         country = "United States"
         return cls(
             street_line1, street_line2, city, state, postal_code, country)
+
+    def get_country_code(self):
+        # TODO add more?
+        country_data = {'United States': 'US'}
+        return country_data.get(self.country)

=== modified file 'u1testutils/pay/selftests/unit/test_pay_api_client.py'
--- u1testutils/pay/selftests/unit/test_pay_api_client.py	2013-03-26 15:11:44 +0000
+++ u1testutils/pay/selftests/unit/test_pay_api_client.py	2013-03-27 20:56:21 +0000
@@ -16,7 +16,6 @@
 
 import mock
 import payclient
-from piston_mini_client import failhandlers
 
 import u1testutils.django
 from u1testutils.pay import client
@@ -27,34 +26,35 @@
 class APIClientTestCase(unittest.TestCase):
 
     def test_get_api_client(self):
-        api_client = client.get_pay_client()
-        self.assertTrue(isinstance(api_client, payclient.PaymentServiceAPI))
+        api_client = client.APIClient()
+        self.assertTrue(
+            isinstance(api_client.client, payclient.PaymentServiceAPI))
 
     def test_get_api_client_with_default_url(self):
-        api_client = client.get_pay_client()
+        api_client = client.APIClient()
         self.assertEquals(
-            api_client._service_root, 'http://localhost:8000/api/2.0/')
+            api_client.client._service_root, 'http://localhost:8000/api/2.0/')
 
     def test_get_api_client_with_settings_url(self):
         with u1testutils.django.patch_settings(
                 PAY_SERVER_URL='http://settings.pay.local:8000'):
-            api_client = client.get_pay_client()
+            api_client = client.APIClient()
         self.assertEquals(
-            api_client._service_root,
+            api_client.client._service_root,
             'http://settings.pay.local:8000/api/2.0/')
 
     def test_get_api_client_with_argument_url(self):
         pay_server_url = 'http://argument.pay.local:8000'
-        api_client = client.get_pay_client(pay_server_url)
+        api_client = client.APIClient(pay_server_url)
         self.assertEquals(
-            api_client._service_root,
+            api_client.client._service_root,
             'http://argument.pay.local:8000/api/2.0/')
 
     def test_get_api_client_with_argument_url_and_trailing_slash(self):
         pay_server_url = 'http://argument.pay.local:8000/'
-        api_client = client.get_pay_client(pay_server_url)
+        api_client = client.APIClient(pay_server_url)
         self.assertEquals(
-            api_client._service_root,
+            api_client.client._service_root,
             'http://argument.pay.local:8000/api/2.0/')
 
 
@@ -67,15 +67,27 @@
         self.billing_address = pay_data.Address.make_unique()
 
     def test_new_credit_card_added(self):
-        with mock.patch('payclient.PaymentServiceAPI.new_credit_card',
-                        return_value=payclient.CreditCardResponse()):
-            added = client.add_new_credit_card(
-                'TEST', self.user, self.credit_card, self.billing_address)
-        self.assertTrue(added)
-
-    def test_add_new_credit_card_failed(self):
-        with mock.patch('payclient.PaymentServiceAPI.new_credit_card',
-                        side_effect=failhandlers.APIError('test')):
-            added = client.add_new_credit_card(
-                'TEST', self.user, self.credit_card, self.billing_address)
-        self.assertFalse(added)
+        shop_id = 'TEST'
+        api_client = client.APIClient()
+        with mock.patch('payclient.PaymentServiceAPI.new_credit_card') \
+                as mock_api_call:
+            api_client.add_new_credit_card(
+                shop_id, self.user, self.credit_card, self.billing_address)
+        expected_params = {
+            'shop_id': shop_id,
+            'open_id': self.user.openid,
+            'card_type': self.credit_card.card_type,
+            'card_holder': self.credit_card.name_on_card,
+            'card_number': self.credit_card.card_number,
+            'card_ccv': self.credit_card.ccv_number,
+            'card_expiration_month': self.credit_card.expiration_month,
+            'card_expiration_year': self.credit_card.expiration_year,
+            'billing_address': {
+                'street': self.billing_address.street_line1,
+                'state': self.billing_address.state,
+                'postal': self.billing_address.postal_code,
+                'country': 'US',
+            }
+        }
+        mock_api_call.assert_called_with(
+            data=payclient.CreditCardRequest(**expected_params))