Merge lp:~elopio/u1-test-utils/in-dash-pay into lp:~canonical-isd-hackers/u1-test-utils/test-in-dash-payments

Proposed by Leo Arias
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: 78
Merged at revision: 26
Proposed branch: lp:~elopio/u1-test-utils/in-dash-pay
Merge into: lp:~canonical-isd-hackers/u1-test-utils/test-in-dash-payments
Prerequisite: lp:~elopio/u1-test-utils/in-dash-sso
Diff against target: 231 lines (+127/-20)
5 files modified
tests/pay.py (+23/-0)
tests/schema.py (+6/-0)
tests/test_purchase_good.py (+1/-1)
tests/test_users.py (+42/-9)
tests/users.py (+55/-10)
To merge this branch: bzr merge lp:~elopio/u1-test-utils/in-dash-pay
Reviewer Review Type Date Requested Status
Vincent Ladeuil (community) Approve
Review via email: mp+163267@code.launchpad.net

Description of the change

To post a comment you must log in.
lp:~elopio/u1-test-utils/in-dash-pay updated
68. By Leo Arias

Merged with the parent branch.

Revision history for this message
Vincent Ladeuil (vila) wrote :

12 + api_client = api.APIClient(conf.settings.PAY_SERVER_URL)

Same remark about the anti-pattern of forcing a config use.

Still ok with it as long as you add FIXME where relevant ;)

72 + self.assertEqual(
73 + user.user_data.full_name, 'Test user {0}'.format(unique_id)[:30])
74 + self.assertEqual(
75 + user.user_data.email, 'test+{0}@example.com'.format(unique_id))

I would switch actual, expected here while you're there (I realize you don't
introduce that).

89 + self.assertEqual(payment_type, expected_type)
90 + self.assertEqual(unattended, None)

And there too, especially given that you could have used:

    self.assertIs(None, unattended)

97 + self.assertEqual(payment_type, expected_type)
98 + self.assertEqual(unattended, True)

Same here where you could have used:

    self.assertTrue(unattended)

I.e. it's closer to:

   self.assertEqual(True, unattended)

But that's minor compared to having these tests, thanks for that ;)

114 +from tests import pay, sso

from tests import (
    pay,
    sso
)

128 - # If the user has a name longer than 30 characters, the log in will
129 - # fail. TODO is this a bug? -- elopio - 2013-05-07
130 user_data.full_name = user_data.full_name[:30]

-1

I'd rather fail if the full_name is too long. I'll be ok with a FIXME for
that as I expect we'll rarely run into the issue. But truncating this way,
especially with a test that will silently validate that is likely to lead to
confusing errors for people running into it:

72 + self.assertEqual(
73 + user.user_data.full_name, 'Test user {0}'.format(unique_id)[:30])

150 + # Make sure the user is registered in pay. Otherwise, the API calls
151 + # will give a 500 error.

Urgh, is there a bug filed for that ?

153 + if not self._user_created_in_pay:
154 + pay.log_in_to_website(self.user_data)

I could go with an explanation of why this is needed...

164 + def get_payment_method(self):

172 + return default_payment_method, allow_unattended_payments

Why not two distinct helpers there ? They could share the
pay.get_payment_preferences_with_server_api() result if needed but if we
need the allow_unattended_payments alone, the caller code will look weird
otherwise. Just wondering though, I may be over-designing here ;)

Overall, awesome work again, I'll now try to run the indash tests to better
understand how it works. Feel free to land with the tweaks mentioned above,
discuss further or address them in a followup.

review: Needs Fixing
lp:~elopio/u1-test-utils/in-dash-pay updated
69. By Leo Arias

Merged with parent branch.

70. By Leo Arias

Merged with parent branch.

71. By Leo Arias

Missing parameter from the merge.

72. By Leo Arias

Pass the server data to the pay methods.

Revision history for this message
Leo Arias (elopio) wrote :

> 12 + api_client = api.APIClient(conf.settings.PAY_SERVER_URL)
>
> Same remark about the anti-pattern of forcing a config use.
>
> Still ok with it as long as you add FIXME where relevant ;)

Just as I did with SSO, the tests pass the config values to the user object. So, we will need to change only the way the tests get those values.

> 72 + self.assertEqual(
> 73 + user.user_data.full_name, 'Test user {0}'.format(unique_id)[:30])
> 74 + self.assertEqual(
> 75 + user.user_data.email, 'test+{0}@example.com'.format(unique_id))
>
> I would switch actual, expected here while you're there (I realize you don't
> introduce that).

As we discussed, I currently prefer it this way.

> 89 + self.assertEqual(payment_type, expected_type)
> 90 + self.assertEqual(unattended, None)
>
> And there too, especially given that you could have used:
>
> self.assertIs(None, unattended)
>
> 97 + self.assertEqual(payment_type, expected_type)
> 98 + self.assertEqual(unattended, True)
>
> Same here where you could have used:
>
> self.assertTrue(unattended)
>
> I.e. it's closer to:
>
> self.assertEqual(True, unattended)
>
> But that's minor compared to having these tests, thanks for that ;)

I changed the asserts but not the order of the parameters.

> 114 +from tests import pay, sso
>
> from tests import (
> pay,
> sso
> )
>
> 128 - # If the user has a name longer than 30 characters, the log in will
> 129 - # fail. TODO is this a bug? -- elopio - 2013-05-07
> 130 user_data.full_name = user_data.full_name[:30]
>
> -1
>
> I'd rather fail if the full_name is too long. I'll be ok with a FIXME for
> that as I expect we'll rarely run into the issue. But truncating this way,
> especially with a test that will silently validate that is likely to lead to
> confusing errors for people running into it:

I think you are misunderstanding it here, as we are creating this user from scratch. So, I'm just making sure it will not have a name longer than 30 characters. On my TODO list, ask about this because it looks like a bug. I just don't know in what server or client.
Or maybe I misunderstood your comment.

> 72 + self.assertEqual(
> 73 + user.user_data.full_name, 'Test user {0}'.format(unique_id)[:30])
>
> 150 + # Make sure the user is registered in pay. Otherwise, the API calls
> 151 + # will give a 500 error.
>
> Urgh, is there a bug filed for that ?

Yes, bug 1144523. I added that on the comment.
>
> 153 + if not self._user_created_in_pay:
> 154 + pay.log_in_to_website(self.user_data)
>
> I could go with an explanation of why this is needed...

I think that's what I'm explaining in the comment above.
>
> 164 + def get_payment_method(self):
>
> 172 + return default_payment_method, allow_unattended_payments
>
> Why not two distinct helpers there ? They could share the
> pay.get_payment_preferences_with_server_api() result if needed but if we
> need the allow_unattended_payments alone, the caller code will look weird
> otherwise. Just wondering though, I may be over-designing here ;)

done.

lp:~elopio/u1-test-utils/in-dash-pay updated
73. By Leo Arias

Changed the assertEquals.

74. By Leo Arias

Added a link to the pay bug that requires login before using the api.

75. By Leo Arias

Split the get_payment_method helper.

76. By Leo Arias

Wrong method name.

77. By Leo Arias

Wrong merge, missed the uuid.

78. By Leo Arias

Typo.

Revision history for this message
Vincent Ladeuil (vila) wrote :

Hmpf, you did it twice, congrats, thanks, so much clearer, what else can I say than: OMG, let's land THAT !

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'tests/pay.py'
2--- tests/pay.py 1970-01-01 00:00:00 +0000
3+++ tests/pay.py 2013-05-15 22:33:26 +0000
4@@ -0,0 +1,23 @@
5+from sst import actions as sst_actions
6+from u1testutils.pay import api, data
7+from u1testutils.sso import sst as sso_website
8+
9+
10+def get_payment_preferences_with_server_api(user, pay_server_url):
11+ api_client = api.APIClient(pay_server_url)
12+ return api_client.client.account_preferences(open_id=user.openid)
13+
14+
15+def add_new_credit_card_with_server_api(
16+ user, unattended, pay_server_url, consumer_id):
17+ credit_card = data.CreditCard.make_test_visa_card()
18+ billing_address = data.Address.make_unique()
19+ api_client = api.APIClient(pay_server_url)
20+ api_client.add_new_credit_card(
21+ consumer_id, user, credit_card, billing_address, unattended)
22+
23+
24+def log_in_to_website(user, pay_server_url):
25+ sst_actions.go_to(pay_server_url)
26+ sst_actions.click_link('login-link')
27+ sso_website.sign_in(user, is_site_recognized=False)
28
29=== modified file 'tests/schema.py'
30--- tests/schema.py 2013-05-15 22:33:26 +0000
31+++ tests/schema.py 2013-05-15 22:33:26 +0000
32@@ -6,6 +6,12 @@
33 class openid(schema.Section):
34 openid_sso_server_url = schema.StringOption()
35
36+ class upay(schema.Section):
37+ pay_server_url = schema.StringOption()
38+ consumer_id = schema.StringOption()
39+ pay_api_username = schema.StringOption()
40+ pay_api_password = schema.StringOption()
41+
42 class ubuntuone(schema.Section):
43 ubuntuone_server_url = schema.StringOption()
44
45
46=== modified file 'tests/test_purchase_good.py'
47--- tests/test_purchase_good.py 2013-05-15 22:33:26 +0000
48+++ tests/test_purchase_good.py 2013-05-15 22:33:26 +0000
49@@ -26,7 +26,7 @@
50 # A user will purchase a good
51 self.user = users.User.make_unique()
52 # Set the urls of the servers that will be used by the user.
53- self.user.set_urls(
54+ self.user.set_servers_data(
55 sso_server_url=conf.settings.OPENID_SSO_SERVER_URL,
56 ubuntuone_server_url=conf.settings.UBUNTUONE_SERVER_URL)
57 # Arbitrarily using a good that is known to work
58
59=== modified file 'tests/test_users.py'
60--- tests/test_users.py 2013-05-15 22:33:26 +0000
61+++ tests/test_users.py 2013-05-15 22:33:26 +0000
62@@ -1,28 +1,61 @@
63+import uuid
64+
65 # FIXME do not use django config to get the server settings.
66 # -- elopio - 2013-05-15
67 from django import conf
68+from sst import runtests
69+
70
71 import tests
72 from tests import users
73
74
75-class UsersTest(tests.TestCase):
76+class UsersTest(runtests.SSTTestCase):
77+
78+ xserver_headless = True
79
80 def test_make_unique(self):
81- user = users.User.make_unique(unique_id='test_uuid')
82- user.set_urls(sso_server_url=conf.settings.OPENID_SSO_SERVER_URL)
83-
84- self.assertEqual(user.user_data.full_name, 'Test user test_uuid')
85- self.assertEqual(
86- user.user_data.email, 'test+test_uuid@example.com')
87+ unique_id = str(uuid.uuid1())
88+ user = users.User.make_unique(unique_id)
89+ user.set_servers_data(
90+ sso_server_url=conf.settings.OPENID_SSO_SERVER_URL,
91+ pay_server_url=conf.settings.PAY_SERVER_URL)
92+ self.assertEqual(
93+ user.user_data.full_name, 'Test user {0}'.format(unique_id)[:30])
94+ self.assertEqual(
95+ user.user_data.email, 'test+{0}@example.com'.format(unique_id))
96 self.assertEqual(
97 user.user_data.password, 'Hola123*')
98 self.assertFalse(user.is_logged_in())
99- self.assertEqual(user.payment_method, None)
100+ self.assertIs(user.get_payment_type(), None)
101+ self.assertIs(user.get_allow_unattended_payments(), None)
102+
103+ def test_set_credit_card_payment_method(self):
104+ expected_type = 'Credit Card'
105+ user = users.User.make_unique()
106+ user.set_servers_data(
107+ sso_server_url=conf.settings.OPENID_SSO_SERVER_URL,
108+ pay_server_url=conf.settings.PAY_SERVER_URL, consumer_id = 'TEST')
109+ user.set_payment_method(expected_type)
110+ self.assertEqual(user.get_payment_type(), expected_type)
111+ self.assertIs(user.get_allow_unattended_payments(), None)
112+
113+ def test_set_unattended_credit_card_payment_method(self):
114+ expected_type = 'Credit Card'
115+ user = users.User.make_unique()
116+ user.set_servers_data(
117+ sso_server_url=conf.settings.OPENID_SSO_SERVER_URL,
118+ pay_server_url=conf.settings.PAY_SERVER_URL, consumer_id = 'TEST')
119+ user.set_payment_method(expected_type, unattended=True)
120+ self.assertEqual(user.get_payment_type(), expected_type)
121+ self.assertTrue(user.get_allow_unattended_payments())
122+
123+
124+class SessionTest(tests.TestCase):
125
126 def test_log_in_and_out(self):
127 user = users.User.make_unique()
128- user.set_urls(
129+ user.set_servers_data(
130 sso_server_url=conf.settings.OPENID_SSO_SERVER_URL,
131 ubuntuone_server_url=conf.settings.UBUNTUONE_SERVER_URL)
132 user.login(self)
133
134=== modified file 'tests/users.py'
135--- tests/users.py 2013-05-15 22:33:26 +0000
136+++ tests/users.py 2013-05-15 22:33:26 +0000
137@@ -1,39 +1,84 @@
138 from u1testutils.sso import data
139
140-from tests import sso
141+from tests import pay, sso
142
143
144 class User(object):
145
146 def __init__(self, user_data):
147 self.user_data = user_data
148+ self._user_created_in_sso = False
149+ self._user_created_in_pay = False
150 self.payment_method = None
151
152- def set_urls(self, sso_server_url=None, ubuntuone_server_url=None):
153+ def set_servers_data(
154+ self, sso_server_url=None, pay_server_url=None, consumer_id=None,
155+ ubuntuone_server_url=None):
156 self.sso_server_url = sso_server_url
157+ self.pay_server_url = pay_server_url
158+ self.consumer_id = consumer_id
159 self.ubuntuone_server_url = ubuntuone_server_url
160
161 @classmethod
162 def make_unique(cls, unique_id=None):
163 user_data = data.User.make_unique(unique_id)
164- # If the user has a name longer than 30 characters, the log in will
165- # fail. TODO is this a bug? -- elopio - 2013-05-07
166 user_data.full_name = user_data.full_name[:30]
167 return cls(user_data)
168
169 def is_logged_in(self):
170 return sso.is_logged_in_on_desktop(self.sso_server_url)
171
172+
173 def login(self, test):
174- # Currently we are using a local server, so we don't need to delete
175- # the user after the test. The same applies to the staging server.
176- # When we start testing against the production server, we should either
177- # use a stock user, or delete it after the test. -- elopio - 2013-05-15
178- sso.create_new_account_with_server_api(
179- self.user_data, self.sso_server_url)
180+ self._ensure_user_created_in_sso()
181 test.addCleanup(self.logout)
182 sso.log_in_with_desktop_client(
183 self.user_data, self.sso_server_url, self.ubuntuone_server_url)
184
185+ def _ensure_user_created_in_sso(self):
186+ if not self._user_created_in_sso:
187+ # Currently we are using a local server, so we don't need to delete
188+ # the user after the test. The same applies to the staging server.
189+ # When we start testing against the production server, we should
190+ # either use a stock user, or delete it after the test.
191+ # -- elopio - 2013-05-15
192+ sso.create_new_account_with_server_api(
193+ self.user_data, self.sso_server_url)
194+ self._user_created_in_sso = True
195+
196 def logout(self):
197 sso.log_out_with_dbus_api(self.sso_server_url)
198+
199+ def _ensure_user_created_in_pay(self):
200+ self._ensure_user_created_in_sso()
201+ # Make sure the user is registered in pay. Otherwise, the API calls
202+ # will give a 500 error.
203+ # TODO update this when bug http://pad.lv/1144523 is fixed.
204+ if not self._user_created_in_pay:
205+ pay.log_in_to_website(self.user_data, self.pay_server_url)
206+ self._user_created_in_pay = True
207+
208+ def set_payment_method(self, payment_type, unattended=False):
209+ self._ensure_user_created_in_pay()
210+ if payment_type == 'Credit Card':
211+ pay.add_new_credit_card_with_server_api(
212+ self.user_data, unattended, self.pay_server_url,
213+ self.consumer_id)
214+ else:
215+ raise NotImplementedError
216+
217+ def get_payment_type(self):
218+ self._ensure_user_created_in_pay()
219+ preferences = pay.get_payment_preferences_with_server_api(
220+ self.user_data, self.pay_server_url)
221+ default_payment_method = preferences['default_payment_method']
222+ if default_payment_method is not None:
223+ return default_payment_method['type']
224+ else:
225+ return None
226+
227+ def get_allow_unattended_payments(self):
228+ self._ensure_user_created_in_pay()
229+ preferences = pay.get_payment_preferences_with_server_api(
230+ self.user_data, self.pay_server_url)
231+ return preferences['allow_unattended_payments']

Subscribers

People subscribed via source and target branches