Merge lp:~elopio/u1-test-utils/in-dash-pay into lp:~canonical-isd-hackers/u1-test-utils/test-in-dash-payments
- in-dash-pay
- Merge into test-in-dash-payments
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 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Vincent Ladeuil (community) | Approve | ||
Review via email: mp+163267@code.launchpad.net |
Commit message
Description of the change
This branch needs:
https:/
Leo Arias (elopio) wrote : | # |
> 12 + api_client = api.APIClient(
>
> 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_
> 74 + self.assertEqual(
> 75 + user.user_
>
> 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.assertEqua
> 90 + self.assertEqua
>
> And there too, especially given that you could have used:
>
> self.assertIs(None, unattended)
>
> 97 + self.assertEqua
> 98 + self.assertEqua
>
> Same here where you could have used:
>
> self.assertTrue
>
> I.e. it's closer to:
>
> self.assertEqua
>
> 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.
>
> -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_
>
> 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_
> 154 + pay.log_
>
> 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_
>
> 172 + return default_
>
> Why not two distinct helpers there ? They could share the
> pay.get_
> need the allow_unattende
> otherwise. Just wondering though, I may be over-designing here ;)
done.
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 !
Preview Diff
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'] |
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( data.full_ name, 'Test user {0}'.format( unique_ id)[:30] ) data.email, 'test+{ 0}@example. com'.format( unique_ id))
73 + user.user_
74 + self.assertEqual(
75 + user.user_
I would switch actual, expected here while you're there (I realize you don't
introduce that).
89 + self.assertEqua l(payment_ type, expected_type) l(unattended, None)
90 + self.assertEqua
And there too, especially given that you could have used:
self. assertIs( None, unattended)
97 + self.assertEqua l(payment_ type, expected_type) l(unattended, True)
98 + self.assertEqua
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 full_name[ :30]
129 - # fail. TODO is this a bug? -- elopio - 2013-05-07
130 user_data.full_name = user_data.
-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( data.full_ name, 'Test user {0}'.format( unique_ id)[:30] )
73 + user.user_
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: in_to_website( self.user_ data)
154 + pay.log_
I could go with an explanation of why this is needed...
164 + def get_payment_ method( self):
172 + return default_ payment_ method, allow_unattende d_payments
Why not two distinct helpers there ? They could share the payment_ preferences_ with_server_ api() result if needed but if we d_payments alone, the caller code will look weird
pay.get_
need the allow_unattende
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.