Merge pull request #4448 from gratipay/project/claim-packages-deletion

Harmonize package deletion and claiming

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('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.'