Harmonize package deletion and claiming

- Move upsert & delete onto Package
- Make verification results easier to debug
- Smooth out a slight very wrinkle in return value for paypal_updated
- Implement and test desired behavior

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)
+                   )

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 "<VerificationResult: %r>" % 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)

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'])

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)

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):
 

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):

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('[email protected]').nonce
+        return alice.finish_email_verification('[email protected]', nonce)
+
+
     def test_appears_to_work(self):
         self.make_package()
         assert self.check() == '[email protected]'
@@ -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('[email protected]').nonce
-        alice.finish_email_verification('[email protected]', 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 == '[email protected]'
         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.'