launchpad-reviewers team mailing list archive

[Thomi Richards <thomi.richards@xxxxxxxxxxxxx>]

Thomi Richards has proposed merging lp:~thomir/launchpad/devel-fix-delete-ssh-key-bug into lp:launchpad.

Commit message:
Don't send notification when deleting SSH keys from the API.

Requested reviews:
  William Grant (wgrant): code

For more details, see:
https://code.launchpad.net/~thomir/launchpad/devel-fix-delete-ssh-key-bug/+merge/296280

Don't send notification when deleting SSH keys from the API.
-- 
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2016-05-25 16:41:05 +0000
+++ lib/lp/registry/model/person.py	2016-06-02 04:51:53 +0000
@@ -3486,7 +3486,7 @@
             IPerson(account), key_text, False, dry_run=dry_run)
 
     def deleteSSHKeyFromSSO(self, user, openid_identifier, key_text,
-                                     dry_run=False):
+                            dry_run=False):
         """See `IPersonSet`."""
         if user != getUtility(ILaunchpadCelebrities).ubuntu_sso:
             raise Unauthorized()
@@ -3503,7 +3503,7 @@
             # ISSHKeySet does not restrict the same SSH key being added
             # multiple times, so make sure we delte them all:
             for key in keys:
-                key.destroySelf()
+                key.destroySelf(send_notification=False)
 
     def newTeam(self, teamowner, name, display_name, teamdescription=None,
                 membership_policy=TeamMembershipPolicy.MODERATED,
@@ -4078,12 +4078,13 @@
     keytext = StringCol(dbName='keytext', notNull=True)
     comment = StringCol(dbName='comment', notNull=True)
 
-    def destroySelf(self):
-        # For security reasons we want to notify the preferred email address
-        # that this sshkey has been removed.
-        self.person.security_field_changed(
-            "SSH Key removed from your Launchpad account.",
-            "The SSH Key %s was removed from your account." % self.comment)
+    def destroySelf(self, send_notification=True):
+        if send_notification:
+            # For security reasons we want to notify the preferred email
+            # address that this sshkey has been removed.
+            self.person.security_field_changed(
+                "SSH Key removed from your Launchpad account.",
+                "The SSH Key %s was removed from your account." % self.comment)
         super(SSHKey, self).destroySelf()
 
     def getFullKeyText(self):

=== modified file 'lib/lp/registry/tests/test_ssh.py'
--- lib/lp/registry/tests/test_ssh.py	2016-05-27 01:23:52 +0000
+++ lib/lp/registry/tests/test_ssh.py	2016-06-02 04:51:53 +0000
@@ -66,6 +66,29 @@
         expected = "ssh-dss %s %s" % (key.keytext, key.comment)
         self.assertEqual(expected, key.getFullKeyText())
 
+    def test_destroySelf_sends_notification_by_default(self):
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            key = self.factory.makeSSHKey(person, send_notification=False)
+            key.destroySelf()
+            [email] = pop_notifications()
+            self.assertEqual(
+                email['Subject'],
+                "SSH Key removed from your Launchpad account.")
+            self.assertThat(
+                email.get_payload(),
+                StartsWith(
+                    "The SSH Key %s was removed from your "
+                    % key.comment)
+            )
+
+    def test_destroySelf_notications_can_be_supressed(self):
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            key = self.factory.makeSSHKey(person, send_notification=False)
+            key.destroySelf(False)
+            self.assertEqual([], pop_notifications())
+
 
 class TestSSHKeySet(TestCaseWithFactory):
 

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2016-06-01 11:42:24 +0000
+++ lib/lp/testing/factory.py	2016-06-02 04:51:53 +0000
@@ -4286,7 +4286,8 @@
         return getUtility(IHWSubmissionDeviceSet).create(
             device_driver_link, submission, parent, hal_device_id)
 
-    def makeSSHKey(self, person=None, key_type=SSHKeyType.RSA):
+    def makeSSHKey(self, person=None, key_type=SSHKeyType.RSA,
+                   send_notification=True):
         """Create a new SSHKey.
 
         :param person: If specified, the person to attach the key to. If
@@ -4305,7 +4306,8 @@
             self.getUniqueString(),
             self.getUniqueString(),
             )
-        return getUtility(ISSHKeySet).new(person, public_key)
+        return getUtility(ISSHKeySet).new(
+            person, public_key, send_notification=send_notification)
 
     def makeBlob(self, blob=None, expires=None, blob_file=None):
         """Create a new TemporaryFileStorage BLOB."""