Merge lp:~stevenk/launchpad/notify-announce-list into lp:launchpad

[Steve Kowalik]

[r=wgrant][no-qa] Drop announce_list from notify() and PackageUpload.acceptFromQueue(), leaving it up to notify() internals.

Drop announce_list from the arguments for notify() and PackageUpload.acceptFromQueue().

This now leaves announce notification up to the IDistroSeries.changeslist property, and the notification code itself if it will send it. There was a little test fallout, but nothing too horrid.

I have discussed this change with Julian Edwards before implementing, and he agreed it was a good change.

[William Grant (wgrant)]

133 + if (action == 'accepted' and distroseries.changeslist and
134 + not archive.is_ppa and pocket != PackagePublishingPocket.BACKPORTS
135 + and not (pocket == PackagePublishingPocket.SECURITY and spr is None)
136 + and not is_auto_sync_upload(
137 + spr, bprs, pocket, changes['Changed-By'])):

The ands were consistently at the end of each line. Either is OK, but not both at once please.

review: Approve

=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py	2011-05-20 08:04:19 +0000
+++ lib/lp/archiveuploader/nascentupload.py	2011-06-02 09:53:33 +0000
@@ -855,7 +855,6 @@
                 changes_file_object = open(self.changes.filepath, "r")
                 self.queue_root.notify(
                     summary_text=self.warning_message,
-                    announce_list=self.policy.announcelist,
                     changes_file_object=changes_file_object,
                     logger=self.logger)
                 changes_file_object.close()

=== modified file 'lib/lp/archiveuploader/tests/nascentupload-announcements.txt'
--- lib/lp/archiveuploader/tests/nascentupload-announcements.txt	2011-05-27 06:17:14 +0000
+++ lib/lp/archiveuploader/tests/nascentupload-announcements.txt	2011-06-02 09:53:33 +0000
@@ -655,11 +655,17 @@
     DEBUG   Body:
     DEBUG bar (1.0-6) breezy; urgency=low
     ...
-    DEBUG No announcement sent
+    DEBUG Announcing to [e-mail]
     ...
     DEBUG You are receiving this email because you are the uploader, maintainer
     or
     DEBUG signer of the above package.
+    DEBUG Would have sent a mail:
+    DEBUG   Subject: [ubuntu/hoary] bar 1.0-6 (Accepted)
+    DEBUG   Sender: Celso Providelo <[e-mail]>
+    DEBUG   Recipients: [e-mail]
+    DEBUG   Bcc: Root <root@localhost>, [e-mail]
+    ...
 
 No emails generated:
 

=== modified file 'lib/lp/archiveuploader/tests/nascentupload-ddebs.txt'
--- lib/lp/archiveuploader/tests/nascentupload-ddebs.txt	2011-03-03 00:43:44 +0000
+++ lib/lp/archiveuploader/tests/nascentupload-ddebs.txt	2011-06-02 09:53:33 +0000
@@ -39,7 +39,7 @@
 We don't really care where the source ends up, so we just accept the
 default overrides. It is now pending publication.
 
-    >>> src.queue_root.acceptFromQueue("[e-mail]")
+    >>> src.queue_root.acceptFromQueue()
     >>> print src.queue_root.status.name
     DONE
 
@@ -91,7 +91,7 @@
 
     >>> bin.queue_root.overrideBinaries(main, devel, None, [main, universe])
     True
-    >>> bin.queue_root.acceptFromQueue('[e-mail]')
+    >>> bin.queue_root.acceptFromQueue()
 
     >>> print bin.queue_root.status.name
     ACCEPTED

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2011-05-20 03:28:24 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2011-06-02 09:53:33 +0000
@@ -1438,7 +1438,7 @@
         # just-uploaded changesfile from librarian.
         self.layer.txn.commit()
 
-        upload.queue_root.acceptFromQueue('[e-mail]')
+        upload.queue_root.acceptFromQueue()
 
         # 'biscuit_1.0-2' building on i386 get accepted and published.
         packager.buildVersion('1.0-2', suite=self.breezy.name, arch="i386")

=== modified file 'lib/lp/archiveuploader/tests/upload-karma.txt'
--- lib/lp/archiveuploader/tests/upload-karma.txt	2010-12-02 16:13:51 +0000
+++ lib/lp/archiveuploader/tests/upload-karma.txt	2011-06-02 09:53:33 +0000
@@ -33,7 +33,7 @@
 
     >>> from canonical.database.sqlbase import commit
     >>> commit()
-    >>> bar_src.queue_root.acceptFromQueue("[e-mail]")
+    >>> bar_src.queue_root.acceptFromQueue()
     Karma added: action=distributionuploadaccepted, distribution=ubuntu
 
 As can be seen, karma was added for the package creator.
@@ -55,7 +55,7 @@
     >>> key = getUtility(IGPGKeySet).getGPGKeysForPeople([name16])[0]
     >>> removeSecurityProxy(foo_src.queue_root).signing_key = key
     >>> commit()
-    >>> foo_src.queue_root.acceptFromQueue("[e-mail]")
+    >>> foo_src.queue_root.acceptFromQueue()
     Karma added: action=distributionuploadaccepted, distribution=ubuntu
     Karma added: action=sponsoruploadaccepted, distribution=ubuntu
 

=== modified file 'lib/lp/archiveuploader/tests/uploadpolicy.txt'
--- lib/lp/archiveuploader/tests/uploadpolicy.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/archiveuploader/tests/uploadpolicy.txt	2011-06-02 09:53:33 +0000
@@ -71,8 +71,6 @@
   >>> buildd_policy.setDistroSeriesAndPocket("hoary")
   >>> print buildd_policy.distroseries.name
   hoary
-  >>> print buildd_policy.announcelist
-  [e-mail]
 
 Policies can make decisions based on whether or not they want to
 approve an upload automatically (I.E. move it straight to ACCEPTED

=== modified file 'lib/lp/archiveuploader/uploadpolicy.py'
--- lib/lp/archiveuploader/uploadpolicy.py	2011-05-27 19:53:20 +0000
+++ lib/lp/archiveuploader/uploadpolicy.py	2011-06-02 09:53:33 +0000
@@ -150,15 +150,6 @@
         if self.archive is None:
             self.archive = self.distroseries.main_archive
 
-    @property
-    def announcelist(self):
-        """Return the announcement list address."""
-        announce_list = getattr(self.options, 'announcelist', None)
-        if (announce_list is None and
-            getattr(self, 'distroseries', None) is not None):
-            announce_list = self.distroseries.changeslist
-        return announce_list
-
     def checkUpload(self, upload):
         """Mandatory policy checks on NascentUploads."""
         if self.archive.is_copy:

=== modified file 'lib/lp/soyuz/adapters/notification.py'
--- lib/lp/soyuz/adapters/notification.py	2011-05-27 21:12:25 +0000
+++ lib/lp/soyuz/adapters/notification.py	2011-06-02 09:53:33 +0000
@@ -115,9 +115,8 @@
 
 
 def notify(blamer, spr, bprs, customfiles, archive, distroseries, pocket,
-           announce_list=None, summary_text=None, changes=None,
-           changesfile_content=None, changesfile_object=None, action=None,
-           dry_run=False, logger=None):
+           summary_text=None, changes=None, changesfile_content=None,
+           changesfile_object=None, action=None, dry_run=False, logger=None):
     """Notify about 
 
     :param blamer: The `IPerson` who is to blame for this notification.
@@ -127,7 +126,6 @@
     :param archive: The target `IArchive`.
     :param distroseries: The target `IDistroSeries`.
     :param pocket: The target `PackagePublishingPocket`.
-    :param announce_list: Where to announce the upload.
     :param summary_text: The summary of the notification.
     :param changes: A dictionary of the parsed changes file.
     :param changesfile_content: The raw content of the changes file, so it
@@ -203,7 +201,7 @@
             spr, bprs, customfiles, archive, distroseries, pocket, action)
         body = assemble_body(
             blamer, spr, archive, distroseries, summarystring, changes,
-            action, announce_list)
+            action)
         send_mail(
             spr, archive, recipients, subject, body, dry_run,
             changesfile_content=changesfile_content,
@@ -215,10 +213,12 @@
     # If we're sending an acceptance notification for a non-PPA upload,
     # announce if possible. Avoid announcing backports, binary-only
     # security uploads, or autosync uploads.
-    if (action == 'accepted' and announce_list and not archive.is_ppa and
-        pocket != PackagePublishingPocket.BACKPORTS and
-        not (pocket == PackagePublishingPocket.SECURITY and spr is None) and
-        not is_auto_sync_upload(spr, bprs, pocket, changes['Changed-By'])):
+    if (action == 'accepted' and distroseries.changeslist
+        and not archive.is_ppa
+        and pocket != PackagePublishingPocket.BACKPORTS
+        and not (pocket == PackagePublishingPocket.SECURITY and spr is None)
+        and not is_auto_sync_upload(
+            spr, bprs, pocket, changes['Changed-By'])):
         from_addr = sanitize_string(changes['Changed-By'])
         name = None
         bcc_addr = None
@@ -230,11 +230,12 @@
             bcc_addr = '[e-mail]' % name
 
         build_and_send_mail(
-            'announcement', [str(announce_list)], from_addr, bcc_addr)
+            'announcement', [str(distroseries.changeslist)], from_addr,
+            bcc_addr)
 
 
 def assemble_body(blamer, spr, archive, distroseries, summary, changes,
-                  action, announce_list):
+                  action):
     """Assemble the e-mail notification body."""
     information = {
         'STATUS': ACTION_DESCRIPTIONS[action],
@@ -262,8 +263,9 @@
     if action == 'unapproved':
         information['SUMMARY'] += (
             "\nThis upload awaits approval by a distro manager\n")
-    if announce_list:
-        information['ANNOUNCE'] = "Announcing to %s" % announce_list
+    if distroseries.changeslist:
+        information['ANNOUNCE'] = "Announcing to %s" % (
+            distroseries.changeslist)
     if blamer is not None:
         signer_signature = '%s <%s>' % (
             blamer.displayname, blamer.preferredemail.email)

=== modified file 'lib/lp/soyuz/browser/queue.py'
--- lib/lp/soyuz/browser/queue.py	2010-08-25 11:00:07 +0000
+++ lib/lp/soyuz/browser/queue.py	2011-06-02 09:53:33 +0000
@@ -400,7 +400,7 @@
 
     def queue_action_accept(self, queue_item):
         """Reject the queue item passed."""
-        queue_item.acceptFromQueue(announce_list=self.context.changeslist)
+        queue_item.acceptFromQueue()
 
     def queue_action_reject(self, queue_item):
         """Accept the queue item passed."""

=== modified file 'lib/lp/soyuz/doc/distroseriesqueue-notify.txt'
--- lib/lp/soyuz/doc/distroseriesqueue-notify.txt	2011-05-27 06:17:14 +0000
+++ lib/lp/soyuz/doc/distroseriesqueue-notify.txt	2011-06-02 09:53:33 +0000
@@ -43,7 +43,6 @@
   >>> changes_file = open(changes_file_path,'r')
   >>> from lp.services.log.logger import FakeLogger
   >>> netapplet_upload.notify(
-  ...     announce_list="[e-mail]", 
   ...     changes_file_object=changes_file, logger=FakeLogger())
   DEBUG Building recipients list.
   DEBUG Changes file is unsigned, adding changer as recipient
@@ -116,7 +115,6 @@
   >>> changes_file = open(changes_file_path,'r')
   >>> netapplet_upload.setAccepted()
   >>> netapplet_upload.notify(
-  ...     announce_list="[e-mail]",
   ...     changes_file_object=changes_file, logger=FakeLogger())
   DEBUG Building recipients list.
   ...
@@ -124,7 +122,7 @@
   ...
   DEBUG     Recipients: Foo Bar <[e-mail]>, Daniel Silverstone <[e-mail]>
   ...
-  DEBUG Announcing to [e-mail]
+  DEBUG Announcing to [e-mail]
   ...
   DEBUG Sent a mail:
   ...
@@ -137,10 +135,10 @@
   2
 
 The mail 'To:' addresses contain the signer and the changer's email.  The
-announcement email contains the argument specified for "announce_list".
+announcement email contains the serieses changeslist.
 
   >>> [msg['To'] for msg in msgs]
-  ['Foo Bar <[e-mail]>,\n\tDaniel Silverstone <[e-mail]>', '[e-mail]']
+  ['Foo Bar <[e-mail]>,\n\tDaniel Silverstone <[e-mail]>', '[e-mail]']
 
 The mail 'Bcc:' address is the uploader.  The announcement has the uploader
 and the Debian derivatives address for the package uploaded.
@@ -185,8 +183,7 @@
   >>> fillLibrarianFile(1, content=changes_file.read())
   >>> changes_file.close()
   >>> netapplet_upload.setNew()
-  >>> netapplet_upload.notify(announce_list="[e-mail]",
-  ...     logger=FakeLogger())
+  >>> netapplet_upload.notify(logger=FakeLogger())
   DEBUG Building recipients list.
   ...
   DEBUG Sent a mail:

=== modified file 'lib/lp/soyuz/doc/distroseriesqueue.txt'
--- lib/lp/soyuz/doc/distroseriesqueue.txt	2011-01-17 21:51:09 +0000
+++ lib/lp/soyuz/doc/distroseriesqueue.txt	2011-06-02 09:53:33 +0000
@@ -982,7 +982,7 @@
   >>> insertFakeChangesFile(items[1].changesfile.id)
   >>> insertFakeChangesFile(items[3].changesfile.id)
   >>> try:
-  ...    items[1].acceptFromQueue('[e-mail]')
+  ...    items[1].acceptFromQueue()
   ... except QueueInconsistentStateError, e:
   ...    print 'FAILED'
   ... else:

=== modified file 'lib/lp/soyuz/interfaces/queue.py'
--- lib/lp/soyuz/interfaces/queue.py	2011-05-25 08:16:52 +0000
+++ lib/lp/soyuz/interfaces/queue.py	2011-06-02 09:53:33 +0000
@@ -238,7 +238,7 @@
             has no sources associated to it.
         """
 
-    def acceptFromQueue(announce_list, logger=None, dry_run=False):
+    def acceptFromQueue(logger=None, dry_run=False):
         """Call setAccepted, do a syncUpdate, and send notification email.
 
          * Grant karma to people involved with the upload.
@@ -280,15 +280,12 @@
         committed to have some updates actually written to the database.
         """
 
-    def notify(announce_list=None, summary_text=None,
-        changes_file_object=None, logger=None):
+    def notify(summary_text=None, changes_file_object=None, logger=None):
         """Notify by email when there is a new distroseriesqueue entry.
 
         This will send new, accept, announce and rejection messages as
         appropriate.
 
-        :param announce_list: The email address of the distro announcements
-
         :param summary_text: Any additional text to append to the auto-
             generated summary.  This is also the only text used if there is
             a rejection message generated.

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2011-05-27 07:29:08 +0000
+++ lib/lp/soyuz/model/queue.py	2011-06-02 09:53:33 +0000
@@ -392,7 +392,7 @@
         self._closeBugs(changesfile_path, logger)
         self._giveKarma()
 
-    def acceptFromQueue(self, announce_list, logger=None, dry_run=False):
+    def acceptFromQueue(self, logger=None, dry_run=False):
         """See `IPackageUpload`."""
         assert not self.is_delayed_copy, 'Cannot process delayed copies.'
 
@@ -402,7 +402,7 @@
         # is pulled from the librarian which are stripped of their
         # signature just before being stored.
         self.notify(
-            announce_list=announce_list, logger=logger, dry_run=dry_run,
+            logger=logger, dry_run=dry_run,
             changes_file_object=changes_file_object)
         self.syncUpdate()
 
@@ -633,7 +633,6 @@
                     changes_file_object = StringIO.StringIO(
                         changes_file.read())
                     self.notify(
-                        announce_list=self.distroseries.changeslist,
                         changes_file_object=changes_file_object,
                         logger=logger)
                     self.syncUpdate()
@@ -683,8 +682,8 @@
         # and uploading to any archive as the signer.
         return changes, strip_pgp_signature(changes_content).splitlines(True)
 
-    def notify(self, announce_list=None, summary_text=None,
-               changes_file_object=None, logger=None, dry_run=False):
+    def notify(self, summary_text=None, changes_file_object=None,
+               logger=None, dry_run=False):
         """See `IPackageUpload`."""
         status_action = {
             PackageUploadStatus.NEW: 'new',
@@ -704,8 +703,8 @@
             signer = None
         notify(
             signer, self.sourcepackagerelease, self.builds, self.customfiles,
-            self.archive, self.distroseries, self.pocket, announce_list,
-            summary_text, changes, changesfile_content, changes_file_object,
+            self.archive, self.distroseries, self.pocket, summary_text,
+            changes, changesfile_content, changes_file_object,
             status_action[self.status], dry_run, logger)
 
     def _isPersonUploader(self, person):

=== modified file 'lib/lp/soyuz/scripts/queue.py'
--- lib/lp/soyuz/scripts/queue.py	2010-09-27 20:47:58 +0000
+++ lib/lp/soyuz/scripts/queue.py	2011-06-02 09:53:33 +0000
@@ -79,8 +79,7 @@
 
     def __init__(self, distribution_name, suite_name, queue, terms,
                  component_name, section_name, priority_name,
-                 announcelist, display, no_mail=True, exact_match=False,
-                 log=None):
+                 display, no_mail=True, exact_match=False, log=None):
         """Initialises passed variables. """
         self.terms = terms
         # Some actions have addtional commands at the start of the terms
@@ -94,7 +93,6 @@
         self.no_mail = no_mail
         self.distribution_name = distribution_name
         self.suite_name = suite_name
-        self.announcelist = announcelist
         self.default_sender = "%s <%s>" % (
             config.uploader.default_sender_name,
             config.uploader.default_sender_address)
@@ -112,7 +110,7 @@
             pocket=self.pocket)
 
     def setDefaultContext(self):
-        """Set default distribuiton, distroseries, announcelist."""
+        """Set default distribuiton, distroseries."""
         # if not found defaults to 'ubuntu'
 
         # Avoid circular imports.
@@ -140,9 +138,6 @@
             self.distroseries = self.distribution.currentseries
             self.pocket = PackagePublishingPocket.RELEASE
 
-        if not self.announcelist:
-            self.announcelist = self.distroseries.changeslist
-
     def initialize(self):
         """Builds a list of affected records based on the filter argument."""
         self.setDefaultContext()
@@ -483,8 +478,7 @@
             self.display('Accepting %s' % queue_item.displayname)
             try:
                 queue_item.acceptFromQueue(
-                    announce_list=self.announcelist, logger=self.log,
-                    dry_run=self.no_mail)
+                    logger=self.log, dry_run=self.no_mail)
             except QueueInconsistentStateError, info:
                 self.display('** %s could not be accepted due to %s'
                              % (queue_item.displayname, info))
@@ -522,8 +516,7 @@
 
     def __init__(self, distribution_name, suite_name, queue, terms,
                  component_name, section_name, priority_name,
-                 announcelist, display, no_mail=True, exact_match=False,
-                 log=None):
+                 display, no_mail=True, exact_match=False, log=None):
         """Constructor for QueueActionOverride."""
 
         # This exists so that self.terms_start_index can be set as this action
@@ -532,8 +525,8 @@
         # over-ride.
         QueueAction.__init__(self, distribution_name, suite_name, queue,
                              terms, component_name, section_name,
-                             priority_name, announcelist, display,
-                             no_mail=True, exact_match=False, log=log)
+                             priority_name, display, no_mail=True,
+                             exact_match=False, log=log)
         self.terms_start_index = 1
         self.overrides_performed = 0
 
@@ -665,12 +658,11 @@
     """A wrapper for queue_action classes."""
 
     def __init__(self, queue, distribution_name, suite_name,
-                 announcelist, no_mail, component_name, section_name,
-                 priority_name, display=default_display, log=None):
+                 no_mail, component_name, section_name, priority_name,
+                 display=default_display, log=None):
         self.queue = queue
         self.distribution_name = distribution_name
         self.suite_name = suite_name
-        self.announcelist = announcelist
         self.no_mail = no_mail
         self.component_name = component_name
         self.section_name = section_name
@@ -701,7 +693,6 @@
             queue_action = queue_action_class(
                 distribution_name=self.distribution_name,
                 suite_name=self.suite_name,
-                announcelist=self.announcelist,
                 queue=self.queue,
                 no_mail=self.no_mail,
                 display=self.display,

=== modified file 'lib/lp/soyuz/scripts/tests/test_queue.py'
--- lib/lp/soyuz/scripts/tests/test_queue.py	2011-01-17 21:51:09 +0000
+++ lib/lp/soyuz/scripts/tests/test_queue.py	2011-06-02 09:53:33 +0000
@@ -91,10 +91,9 @@
         self.test_output.append(text)
 
     def execute_command(self, argument, queue_name='new', no_mail=True,
-                        distribution_name='ubuntu',announcelist=None,
-                        component_name=None, section_name=None,
-                        priority_name=None, suite_name='breezy-autotest',
-                        quiet=True):
+                        distribution_name='ubuntu', component_name=None,
+                        section_name=None, priority_name=None,
+                        suite_name='breezy-autotest', quiet=True):
         """Helper method to execute a queue command.
 
         Initialise output buffer and execute a command according
@@ -105,7 +104,7 @@
         self.test_output = []
         queue = name_queue_map[queue_name]
         runner = CommandRunner(
-            queue, distribution_name, suite_name, announcelist, no_mail,
+            queue, distribution_name, suite_name, no_mail,
             component_name, section_name, priority_name,
             display=self._test_display)
 

=== modified file 'lib/lp/soyuz/tests/test_packageupload.py'
--- lib/lp/soyuz/tests/test_packageupload.py	2011-05-25 07:09:27 +0000
+++ lib/lp/soyuz/tests/test_packageupload.py	2011-06-02 09:53:33 +0000
@@ -63,8 +63,7 @@
         delayed_copy = self.createEmptyDelayedCopy()
         self.assertRaisesWithContent(
             AssertionError,
-            'Cannot process delayed copies.',
-            delayed_copy.acceptFromQueue, 'some-announce-list')
+            'Cannot process delayed copies.', delayed_copy.acceptFromQueue)
 
     def test_acceptFromCopy_refuses_empty_copies(self):
         # Empty delayed-copies cannot be accepted.

=== modified file 'scripts/ftpmaster-tools/queue'
--- scripts/ftpmaster-tools/queue	2010-04-27 19:48:39 +0000
+++ scripts/ftpmaster-tools/queue	2011-06-02 09:53:33 +0000
@@ -59,12 +59,6 @@
                       default=False,
                       help="Ignore errors when performing a list of commands.")
 
-    parser.add_option("-A", "--announcelist",
-                      dest="announcelist", metavar="ANNOUNCELIST",
-                      default=None,
-                      help=("Overrides the announcement list for accepts, "
-                            "defaults to the distribution 'changeslist'"))
-
     parser.add_option("-f", "--file", metavar="FILE", default=None,
                       help="file containing a sequence of command lines.")
 
@@ -98,8 +92,8 @@
 
     cmd_runner = CommandRunner(
         queue, options.distribution_name, options.suite_name,
-        options.announcelist, no_mail, options.component_name,
-        options.section_name, options.priority_name, log=log)
+        no_mail, options.component_name, options.section_name,
+        options.priority_name, log=log)
 
     print ("Initialising connection to queue %s" % options.queue_name)