From 13b33a67b7523fea1f12c0f6ff3013e120052132 Mon Sep 17 00:00:00 2001 From: Chad Whitacre Date: Wed, 21 Sep 2016 16:50:23 -0400 Subject: [PATCH 1/4] Remove unused method I think the plan was to use this in closing teams? We can bring it back when we get to #3602, I guess. --- gratipay/models/team/mixins/membership.py | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/gratipay/models/team/mixins/membership.py b/gratipay/models/team/mixins/membership.py index 8194b80331..8166d7854c 100644 --- a/gratipay/models/team/mixins/membership.py +++ b/gratipay/models/team/mixins/membership.py @@ -27,17 +27,6 @@ def remove_member(self, participant, recorder): self.set_take_for(participant, ZERO, recorder) - def remove_all_members(self, cursor=None): - (cursor or self.db).run(""" - INSERT INTO takes (ctime, member, team, amount, recorder) ( - SELECT ctime, member, %(username)s, 0.00, %(username)s - FROM current_takes - WHERE team=%(username)s - AND amount > 0 - ); - """, dict(username=self.username)) - - @property def nmembers(self): """The number of members. Read-only and computed (not in the db); equal to From f1bafcd832b320b16c0d3781c363422a67bbc624 Mon Sep 17 00:00:00 2001 From: Chad Whitacre Date: Wed, 21 Sep 2016 17:19:53 -0400 Subject: [PATCH 2/4] Round out tests for membership mixin This surfaced a bug in get_memberships (yay!), a holdover from when Teams were a special kind of Participant rather than a separate entity. --- gratipay/models/team/mixins/membership.py | 5 +---- tests/py/test_team_membership.py | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/gratipay/models/team/mixins/membership.py b/gratipay/models/team/mixins/membership.py index 8166d7854c..25795cada4 100644 --- a/gratipay/models/team/mixins/membership.py +++ b/gratipay/models/team/mixins/membership.py @@ -53,10 +53,7 @@ def get_memberships(self, current_participant=None): if current_participant: member['removal_allowed'] = current_participant.username == self.owner if member['username'] == current_participant.username: - member['is_current_user'] = True - if take['ctime'] is not None: - # current user, but not the team itself - member['editing_allowed']= True + member['editing_allowed']= True member['last_week'] = self.get_take_last_week_for(member['participant_id']) members.append(member) diff --git a/tests/py/test_team_membership.py b/tests/py/test_team_membership.py index d2c74810f9..ca2523ec68 100644 --- a/tests/py/test_team_membership.py +++ b/tests/py/test_team_membership.py @@ -32,6 +32,26 @@ def test_gm_returns_more_memberships_when_there_are_more_members(self): self.enterprise.add_member(self.bruiser, self.picard) assert len(self.enterprise.get_memberships()) == 2 + def test_gm_sets_removal_allowed_to_true_when_removal_allowed(self): + self.enterprise.add_member(self.bruiser, self.picard) + memberships = self.enterprise.get_memberships(self.picard) + assert memberships[0]['removal_allowed'] + + def test_gm_sets_removal_allowed_to_false_when_removal_not_allowed(self): + self.enterprise.add_member(self.bruiser, self.picard) + memberships = self.enterprise.get_memberships(self.bruiser) + assert not memberships[0]['removal_allowed'] + + def test_gm_sets_editing_allowed_to_true_when_editing_allowed(self): + self.enterprise.add_member(self.bruiser, self.picard) + memberships = self.enterprise.get_memberships(self.bruiser) + assert memberships[0]['editing_allowed'] + + def test_gm_sets_editing_allowed_to_false_when_editing_not_allowed(self): + self.enterprise.add_member(self.bruiser, self.picard) + memberships = self.enterprise.get_memberships(self.picard) + assert not memberships[0]['editing_allowed'] + # am - add_member From df5c9be57b7244c98ef87deba12208f7554611c5 Mon Sep 17 00:00:00 2001 From: Chad Whitacre Date: Wed, 21 Sep 2016 17:56:47 -0400 Subject: [PATCH 3/4] Update get_take_last_week_for Brings the API into alignment with get_take_for and adds a test suite. --- gratipay/models/team/mixins/membership.py | 2 +- gratipay/models/team/mixins/takes.py | 43 +++++++++++++---------- gratipay/testing/billing.py | 15 +++++++- tests/py/test_billing_exchanges.py | 6 ---- tests/py/test_team_membership.py | 8 ++++- tests/py/test_team_takes.py | 30 +++++++++++++++- 6 files changed, 75 insertions(+), 29 deletions(-) diff --git a/gratipay/models/team/mixins/membership.py b/gratipay/models/team/mixins/membership.py index 25795cada4..30b22631a7 100644 --- a/gratipay/models/team/mixins/membership.py +++ b/gratipay/models/team/mixins/membership.py @@ -55,6 +55,6 @@ def get_memberships(self, current_participant=None): if member['username'] == current_participant.username: member['editing_allowed']= True - member['last_week'] = self.get_take_last_week_for(member['participant_id']) + member['last_week'] = self.get_take_last_week_for(take['participant']) members.append(member) return members diff --git a/gratipay/models/team/mixins/takes.py b/gratipay/models/team/mixins/takes.py index 7357ea8e2e..53b8789297 100644 --- a/gratipay/models/team/mixins/takes.py +++ b/gratipay/models/team/mixins/takes.py @@ -31,25 +31,6 @@ class TakesMixin(object): ndistributing_to = 0 - def get_take_last_week_for(self, participant_id): - """Get the participant's nominal take last week. - """ - return self.db.one(""" - - SELECT amount - FROM takes - WHERE team_id=%s AND participant_id=%s - AND mtime < ( - SELECT ts_start - FROM paydays - WHERE ts_end > ts_start - ORDER BY ts_start DESC LIMIT 1 - ) - ORDER BY mtime DESC LIMIT 1 - - """, (self.id, participant_id), default=ZERO) - - def set_take_for(self, participant, take, recorder, cursor=None): """Set the amount a participant wants to take from this team during payday. @@ -148,6 +129,30 @@ def get_take_for(self, participant, cursor=None): """, (self.id, participant.id), default=ZERO) + def get_take_last_week_for(self, participant, cursor=None): + """ + :param Participant participant: the participant to get the take for + :param GratipayDB cursor: a database cursor; if ``None``, a new cursor + will be used + :return: a :py:class:`~decimal.Decimal`: the ``participant``'s take + from this team at the beginning of the last completed payday, or 0. + """ + return (cursor or self.db).one(""" + + SELECT amount + FROM takes + WHERE team_id=%s AND participant_id=%s + AND mtime < ( + SELECT ts_start + FROM paydays + WHERE ts_end > ts_start + ORDER BY ts_start DESC LIMIT 1 + ) + ORDER BY mtime DESC LIMIT 1 + + """, (self.id, participant.id), default=ZERO) + + def update_taking(self, old_takes, new_takes, cursor=None, member=None): """Update `taking` amounts based on the difference between `old_takes` and `new_takes`. diff --git a/gratipay/testing/billing.py b/gratipay/testing/billing.py index e4e9ea65d8..62b5b29c1e 100644 --- a/gratipay/testing/billing.py +++ b/gratipay/testing/billing.py @@ -3,8 +3,10 @@ from __future__ import absolute_import, division, print_function, unicode_literals import braintree +import mock from braintree.test.nonces import Nonces +from gratipay.billing.payday import Payday from gratipay.billing.exchanges import cancel_card_hold from gratipay.models.exchange_route import ExchangeRoute @@ -12,7 +14,18 @@ from .vcr import use_cassette -class BillingHarness(Harness): +class PaydayMixin(object): + + @mock.patch.object(Payday, 'fetch_card_holds') + def run_payday(self, fch): + fch.return_value = {} + Payday.start().run() + + def start_payday(self): + Payday.start() + + +class BillingHarness(Harness, PaydayMixin): """This is a harness for billing-related tests. """ diff --git a/tests/py/test_billing_exchanges.py b/tests/py/test_billing_exchanges.py index d23b6519bc..00e919e0db 100644 --- a/tests/py/test_billing_exchanges.py +++ b/tests/py/test_billing_exchanges.py @@ -15,7 +15,6 @@ record_exchange_result, get_ready_payout_routes_by_network ) -from gratipay.billing.payday import Payday from gratipay.exceptions import NegativeBalance, NotWhitelisted from gratipay.models.exchange_route import ExchangeRoute from gratipay.testing import Foobar, Harness, D,P @@ -149,11 +148,6 @@ def test_capch_amount_under_minimum(self): # grprbn - get_ready_payout_routes_by_network - @mock.patch.object(Payday, 'fetch_card_holds') - def run_payday(self, fch): - fch.return_value = {} - Payday.start().run() - def test_grprbn_that_its_empty_to_start_with(self): assert get_ready_payout_routes_by_network(self.db, 'paypal') == [] diff --git a/tests/py/test_team_membership.py b/tests/py/test_team_membership.py index ca2523ec68..cd48c8abbd 100644 --- a/tests/py/test_team_membership.py +++ b/tests/py/test_team_membership.py @@ -1,6 +1,6 @@ from __future__ import absolute_import, division, print_function, unicode_literals -from test_team_takes import TeamTakesHarness +from test_team_takes import TeamTakesHarness, PENNY from gratipay.models.team import mixins @@ -52,6 +52,12 @@ def test_gm_sets_editing_allowed_to_false_when_editing_not_allowed(self): memberships = self.enterprise.get_memberships(self.picard) assert not memberships[0]['editing_allowed'] + def test_gm_sets_last_week(self): + self.enterprise.add_member(self.bruiser, self.picard) + self.run_payday() + memberships = self.enterprise.get_memberships(self.picard) + assert memberships[0]['last_week'] == PENNY + # am - add_member diff --git a/tests/py/test_team_takes.py b/tests/py/test_team_takes.py index d440d54448..ce7cb3b3f6 100644 --- a/tests/py/test_team_takes.py +++ b/tests/py/test_team_takes.py @@ -3,9 +3,10 @@ from pytest import raises from gratipay.models.team.mixins.takes import NotAllowed, PENNY, ZERO from gratipay.testing import Harness, D,P,T +from gratipay.testing.billing import PaydayMixin -class TeamTakesHarness(Harness): +class TeamTakesHarness(Harness, PaydayMixin): # Factored out to share with membership tests ... def setUp(self): @@ -122,6 +123,33 @@ def test_stf_doesnt_let_anyone_set_a_take_who_is_not_already_on_the_team_even_to assert actual == 'can only set take if already a member of the team' + # gtlwf - get_take_last_week_for + + def test_gtlwf_gets_take_last_week_for_someone(self): + self.enterprise.set_take_for(self.crusher, PENNY*1, self.picard) + self.enterprise.set_take_for(self.crusher, PENNY*24, self.crusher) + self.run_payday() + self.enterprise.set_take_for(self.crusher, PENNY*48, self.crusher) + assert self.enterprise.get_take_for(self.crusher) == PENNY*48 # sanity check + assert self.enterprise.get_take_last_week_for(self.crusher) == PENNY*24 + + def test_gtlwf_returns_zero_when_they_werent_taking(self): + self.run_payday() + self.enterprise.set_take_for(self.crusher, PENNY*1, self.picard) + assert self.enterprise.get_take_for(self.crusher) == PENNY*1 # sanity check + assert self.enterprise.get_take_last_week_for(self.crusher) == ZERO + + def test_gtlwf_ignores_a_currently_running_payday(self): + self.enterprise.set_take_for(self.crusher, PENNY*1, self.picard) + self.enterprise.set_take_for(self.crusher, PENNY*24, self.crusher) + self.run_payday() + self.enterprise.set_take_for(self.crusher, PENNY*48, self.crusher) + self.start_payday() + self.enterprise.set_take_for(self.crusher, PENNY*96, self.crusher) + assert self.enterprise.get_take_for(self.crusher) == PENNY*96 # sanity check + assert self.enterprise.get_take_last_week_for(self.crusher) == PENNY*24 + + # ut - update_taking def test_ut_updates_taking(self): From 346dac7daeff9fefe2d56eb95761891bc943cb42 Mon Sep 17 00:00:00 2001 From: Chad Whitacre Date: Thu, 22 Sep 2016 12:10:18 -0400 Subject: [PATCH 4/4] Add tests for vetting during set_take_for --- gratipay/models/team/mixins/takes.py | 4 +-- tests/py/test_team_takes.py | 42 ++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/gratipay/models/team/mixins/takes.py b/gratipay/models/team/mixins/takes.py index 53b8789297..16a7bbcc43 100644 --- a/gratipay/models/team/mixins/takes.py +++ b/gratipay/models/team/mixins/takes.py @@ -53,10 +53,10 @@ def set_take_for(self, participant, take, recorder, cursor=None): def vet(p): if p.is_suspicious: raise NotAllowed("user must not be flagged as suspicious") - elif not p.has_verified_identity: - raise NotAllowed("user must have a verified identity") elif not p.email_address: raise NotAllowed("user must have added at least one email address") + elif not p.has_verified_identity: + raise NotAllowed("user must have a verified identity") elif not p.is_claimed: raise NotAllowed("user must have claimed the account") diff --git a/tests/py/test_team_takes.py b/tests/py/test_team_takes.py index ce7cb3b3f6..6025ff85b9 100644 --- a/tests/py/test_team_takes.py +++ b/tests/py/test_team_takes.py @@ -123,6 +123,48 @@ def test_stf_doesnt_let_anyone_set_a_take_who_is_not_already_on_the_team_even_to assert actual == 'can only set take if already a member of the team' + def test_stf_vets_participant_for_suspiciousness(self): + mallory = self.make_participant('mallory', is_suspicious=True) + actual = self.err(mallory, 0, self.picard) + assert actual == 'user must not be flagged as suspicious' + + def test_stf_vets_participant_for_email(self): + mallory = self.make_participant('mallory') + actual = self.err(mallory, 0, self.picard) + assert actual == 'user must have added at least one email address' + + def test_stf_vets_participant_for_verified_identity(self): + mallory = self.make_participant('mallory', email_address='m@example.com') + actual = self.err(mallory, 0, self.picard) + assert actual == 'user must have a verified identity' + + def test_stf_vets_participant_for_claimed(self): + mallory = self.make_participant('mallory', email_address='m@example.com', verified_in='TT') + actual = self.err(mallory, 0, self.picard) + assert actual == 'user must have claimed the account' + + + def test_stf_vets_recorder_for_suspiciousness(self): + mallory = self.make_participant('mallory', is_suspicious=True) + actual = self.err(self.crusher, 0, mallory) + assert actual == 'user must not be flagged as suspicious' + + def test_stf_vets_recorder_for_email(self): + mallory = self.make_participant('mallory') + actual = self.err(self.crusher, 0, mallory) + assert actual == 'user must have added at least one email address' + + def test_stf_vets_recorder_for_verified_identity(self): + mallory = self.make_participant('mallory', email_address='m@example.com') + actual = self.err(self.crusher, 0, mallory) + assert actual == 'user must have a verified identity' + + def test_stf_vets_recorder_for_claimed(self): + mallory = self.make_participant('mallory', email_address='m@example.com', verified_in='TT') + actual = self.err(self.crusher, 0, mallory) + assert actual == 'user must have claimed the account' + + # gtlwf - get_take_last_week_for def test_gtlwf_gets_take_last_week_for_someone(self):