diff --git a/gratipay/models/package/__init__.py b/gratipay/models/package/__init__.py index 7b1a906035..d39b6999af 100644 --- a/gratipay/models/package/__init__.py +++ b/gratipay/models/package/__init__.py @@ -65,14 +65,58 @@ def remote_api_url(self): # ============ @classmethod - def from_id(cls, id): + def from_id(cls, id, cursor=None): """Return an existing package based on id. """ - return cls.db.one("SELECT packages.*::packages FROM packages WHERE id=%s", (id,)) + cursor = cursor or cls.db + return cursor.one("SELECT packages.*::packages FROM packages WHERE id=%s", (id,)) + @classmethod - def from_names(cls, package_manager, name): + def from_names(cls, package_manager, name, cursor=None): """Return an existing package based on package manager and package names. """ - return cls.db.one("SELECT packages.*::packages FROM packages " - "WHERE package_manager=%s and name=%s", (package_manager, name)) + cursor = cursor or cls.db + return cursor.one( "SELECT packages.*::packages FROM packages " + "WHERE package_manager=%s and name=%s" + , (package_manager, name) + ) + + + @classmethod + def upsert(cls, package_manager, **kw): + """Upsert a package. Required keyword arguments: + + - ``name`` (string) + - ``description`` (string) + - ``emails`` (list of strings) + + Optional keyword argument: + + - ``cursor`` + + :return None: + + """ + cursor = kw.pop('cursor', cls.db) + cursor.run(''' + INSERT INTO packages + (package_manager, name, description, emails) + VALUES ('npm', %(name)s, %(description)s, %(emails)s) + + ON CONFLICT (package_manager, name) DO UPDATE + SET description=%(description)s, emails=%(emails)s + ''', kw) + + + def delete(self, cursor=None): + """Delete the package, unlinking any team (the team itself lives on) + and clearing any claim. + """ + cursor = cursor or self.db + if self.load_team(cursor): + self.unlink_team(cursor) + cursor.run("DELETE FROM claims WHERE package_id=%s", (self.id,)) + cursor.run( "DELETE FROM packages WHERE package_manager=%s AND name=%s" + , (self.package_manager, self.name) + ) diff --git a/gratipay/models/participant/email.py b/gratipay/models/participant/email.py index acd7e01b41..dbc18cb9bc 100644 --- a/gratipay/models/participant/email.py +++ b/gratipay/models/participant/email.py @@ -17,17 +17,25 @@ EMAIL_HASH_TIMEOUT = timedelta(hours=24) +class VerificationResult(object): + def __init__(self, name): + self.name = name + def __repr__(self): + return "" % self.name + __str__ = __repr__ + + #: Signal that verifying an email address failed. -VERIFICATION_FAILED = object() +VERIFICATION_FAILED = VerificationResult('Failed') #: Signal that verifying an email address was redundant. -VERIFICATION_REDUNDANT = object() +VERIFICATION_REDUNDANT = VerificationResult('Redundant') #: Signal that an email address is already verified for a different :py:class:`Participant`. -VERIFICATION_STYMIED = object() +VERIFICATION_STYMIED = VerificationResult('Stymied') #: Signal that email verification succeeded. -VERIFICATION_SUCCEEDED = object() +VERIFICATION_SUCCEEDED = VerificationResult('Succeeded') class Email(object): @@ -257,8 +265,9 @@ def finish_email_verification(self, email, nonce): if (utcnow() - record.verification_start) > EMAIL_HASH_TIMEOUT: return VERIFICATION_FAILED, None, None try: - paypal_updated = False + paypal_updated = None if packages: + paypal_updated = False self.finish_package_claims(cursor, nonce, *packages) self.save_email_address(cursor, email) has_no_paypal = not self.get_payout_routes(good_only=True) diff --git a/gratipay/sync_npm.py b/gratipay/sync_npm.py index 5eb9a027a1..b2c572e22d 100644 --- a/gratipay/sync_npm.py +++ b/gratipay/sync_npm.py @@ -4,6 +4,8 @@ import requests from couchdb import Database +from gratipay.models.package import NPM, Package + REGISTRY_URL = 'https://replicate.npmjs.com/' @@ -49,35 +51,24 @@ def consume_change_stream(stream, db): # Decide what to do. if change.get('deleted'): - op, arg = delete, change['id'] + package = Package.from_names(NPM, change['id']) + assert package is not None # right? + op, kw = package.delete, {} else: - processed_doc = process_doc(change['doc']) - if not processed_doc: + op = Package.upsert + kw = process_doc(change['doc']) + if not kw: continue - op, arg = upsert, processed_doc + kw['package_manager'] = NPM # Do it. cursor = connection.cursor() - op(cursor, arg) + kw['cursor'] = cursor + op(**kw) cursor.run('UPDATE worker_coordination SET npm_last_seq=%(seq)s', change) connection.commit() -def delete(cursor, name): - cursor.run("DELETE FROM packages WHERE package_manager='npm' AND name=%s", (name,)) - - -def upsert(cursor, processed): - cursor.run(''' - INSERT INTO packages - (package_manager, name, description, emails) - VALUES ('npm', %(name)s, %(description)s, %(emails)s) - - ON CONFLICT (package_manager, name) DO UPDATE - SET description=%(description)s, emails=%(emails)s - ''', processed) - - def check(db, _print=print): ours = db.one('SELECT npm_last_seq FROM worker_coordination') theirs = int(requests.get(REGISTRY_URL).json()['update_seq']) diff --git a/tests/py/test_email.py b/tests/py/test_email.py index 91c796f8ff..b1bc6b8865 100644 --- a/tests/py/test_email.py +++ b/tests/py/test_email.py @@ -30,7 +30,7 @@ def add(self, participant, address, _flush=False): participant.start_email_verification(address) nonce = participant.get_email(address).nonce result = participant.finish_email_verification(address, nonce) - assert result == (_email.VERIFICATION_SUCCEEDED, [], False) + assert result == (_email.VERIFICATION_SUCCEEDED, [], None) if _flush: self.app.email_queue.flush() @@ -767,7 +767,7 @@ def check(self, *package_names): # email? packages = [Package.from_names(NPM, name) for name in package_names] - assert result == (_email.VERIFICATION_SUCCEEDED, packages, bool(packages)) + assert result == (_email.VERIFICATION_SUCCEEDED, packages, True if packages else None) assert self.alice.email_address == P('alice').email_address == self.address # database? @@ -859,6 +859,16 @@ def test_finishing_email_verification_with_preexisting_paypal_doesnt_update_payp assert result == (_email.VERIFICATION_SUCCEEDED, [foo], False) + def test_deleting_package_removes_open_claims(self): + self.add_and_verify_email(self.alice, self.address) + self.alice.set_paypal_address(self.address) + self.start(self.address, 'foo') + load = lambda: self.db.one('select * from claims') + assert load() is not None + Package.from_names('npm', 'foo').delete() + assert load() is None + + def test_finishing_email_verification_is_thread_safe(self): foo = self.make_package() self.alice.start_email_verification(self.address, foo) diff --git a/tests/py/test_packages.py b/tests/py/test_packages.py index c980096725..4f7ebca8d9 100644 --- a/tests/py/test_packages.py +++ b/tests/py/test_packages.py @@ -9,16 +9,58 @@ from pytest import raises -class TestPackage(Harness): +Foo = lambda cursor=None: Package.from_names(NPM, 'foo', cursor) + + +class Basics(Harness): def test_can_be_instantiated_from_id(self): - p = self.make_package() - assert Package.from_id(p.id).id == p.id + package = self.make_package() + assert Package.from_id(package.id).id == package.id + + def test_from_id_can_use_a_cursor(self): + package = self.make_package() + with self.db.get_cursor() as cursor: + assert Package.from_id(package.id, cursor).id == package.id def test_can_be_instantiated_from_names(self): self.make_package() assert Package.from_names(NPM, 'foo').name == 'foo' + def test_from_names_can_use_a_cursor(self): + self.make_package() + with self.db.get_cursor() as cursor: + assert Package.from_names(NPM, 'foo', cursor).name == 'foo' + + + def test_can_be_inserted_via_upsert(self): + Package.upsert(NPM, name='foo', description='Foo!', emails=[]) + assert Foo().name == 'foo' + + def test_can_be_updated_via_upsert(self): + self.make_package() + Package.upsert(NPM, name='foo', description='Bar!', emails=[]) + assert Foo().description == 'Bar!' + + def test_can_be_upserted_in_a_transaction(self): + self.make_package(description='Before') + with self.db.get_cursor() as cursor: + Package.upsert(NPM, name='foo', description='After', emails=[], cursor=cursor) + assert Foo().description == 'Before' + assert Foo().description == 'After' + + + def test_can_be_deleted(self): + self.make_package().delete() + assert Foo() is None + + def test_can_be_deleted_in_a_transaction(self): + package = self.make_package() + with self.db.get_cursor() as cursor: + package.delete(cursor) + assert Foo() == package + assert Foo() is None + class Linking(Harness): @@ -99,6 +141,12 @@ def test_closing_team_unlinks_package(self): team.close() assert Package.from_names('npm', 'foo').team is None + def test_deleting_package_leaves_team_unlinked(self): + _, package, team = self.link() + assert team.package is not None + package.delete() + assert team.package is None + class GroupEmailsForParticipant(Harness): diff --git a/tests/py/test_sync_npm.py b/tests/py/test_sync_npm.py index 528a5fead7..4ac354b2bc 100644 --- a/tests/py/test_sync_npm.py +++ b/tests/py/test_sync_npm.py @@ -71,7 +71,19 @@ def test_not_afraid_to_delete_docs(self): assert self.db.one('select * from packages') is None - # TODO Test for packages linked to teams when we get there. + def test_even_deletes_package_with_linked_team(self): + + # Set up package linked to team. + foo = self.make_package() + alice = self.make_participant('alice') + with self.db.get_cursor() as cursor: + team = foo.get_or_create_linked_team(cursor, alice) + assert self.db.one('select p.*::packages from packages p').team == team + + # Delete package. + docs = [{'deleted': True, 'id': 'foo'}] + sync_npm.consume_change_stream(self.change_stream(docs), self.db) + assert self.db.one('select * from packages') is None def test_picks_up_with_last_seq(self): diff --git a/tests/ttw/test_package_claiming.py b/tests/ttw/test_package_claiming.py index 5f8a1549d4..287ff96823 100644 --- a/tests/ttw/test_package_claiming.py +++ b/tests/ttw/test_package_claiming.py @@ -3,6 +3,8 @@ import pickle from gratipay.testing import BrowserHarness, P +from gratipay.models.package import NPM, Package +from gratipay.models.participant import email class Test(BrowserHarness): @@ -14,10 +16,15 @@ def check(self, choice=0): self.css('label')[0].click() # activate select self.css('label')[choice].click() self.css('button')[0].click() - assert self.has_element('.notification.notification-success', 1) - assert self.has_text('Check your inbox for a verification link.') + assert self.wait_for_success() == 'Check your inbox for a verification link.' return self.db.one('select address from claims c join emails e on c.nonce = e.nonce') + def finish_claiming(self): + alice = P('alice') + nonce = alice.get_email('alice@example.com').nonce + return alice.finish_email_verification('alice@example.com', nonce) + + def test_appears_to_work(self): self.make_package() assert self.check() == 'alice@example.com' @@ -44,13 +51,10 @@ def test_disabled_items_are_disabled(self): assert self.db.all('select * from claims') == [] - def test_that_claimed_packages_can_be_given_to(self): + def test_claimed_packages_can_be_given_to(self): package = self.make_package() self.check() - - alice = P('alice') - nonce = alice.get_email('alice@example.com').nonce - alice.finish_email_verification('alice@example.com', nonce) + self.finish_claiming() self.make_participant('admin', claimed_time='now', is_admin=True) self.sign_in('admin') @@ -80,3 +84,28 @@ def test_visiting_verify_link_shows_helpful_information(self): assert self.css('.withdrawal-notice a').text == 'update' assert self.css('.withdrawal-notice b').text == 'alice@example.com' assert self.css('.listing-name').text == 'foo' + + + def test_deleted_packages_are_404(self): + self.make_package() + Package.from_names(NPM, 'foo').delete() + self.visit('/on/npm/foo/') + assert self.css('#content h1').text == '404' + + + def test_claiming_deleted_packages_is_a_noop(self): + self.make_package() + self.check() + Package.from_names(NPM, 'foo').delete() + assert self.finish_claiming() == (email.VERIFICATION_SUCCEEDED, [], None) + + + def test_but_once_claimed_then_team_persists(self): + self.make_package() + self.check() + foo = Package.from_names(NPM, 'foo') + assert self.finish_claiming() == (email.VERIFICATION_SUCCEEDED, [foo], True) + foo.delete() + self.visit('/foo/') + foo = self.css('#content .statement p') + assert foo.text == 'Foo fooingly.'