Launchpad itself

Merge lp:~wgrant/launchpad/reject-not-accepted into lp:launchpad

  1. reject-not-accepted
  2. Merge into devel
Proposed by William Grant
Status: Merged
Merged at revision: 17882
Proposed branch: lp:~wgrant/launchpad/reject-not-accepted
Merge into: lp:launchpad
Diff against target: 60 lines (+3/-20)
2 files modified
lib/lp/archiveuploader/nascentupload.py (+2/-9)
lib/lp/archiveuploader/tests/nascentupload.txt (+1/-11)
To merge this branch: bzr merge lp:~wgrant/launchpad/reject-not-accepted
Reviewer Review Type Date Requested Status
Colin Watson (community) Approve
Review via email: mp+281435@code.launchpad.net

Commit message

Fix NascentUpload.do_reject to not send an erroneous Accepted email.

Description of the change

Fix NascentUpload.do_reject to not send an erroneous Accepted email.

It would previously do so if the upload made it to Done before an exception occurred (eg. because build creation failed).

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) wrote :

This seems to change do_reject to only send the mail, and not actually change the status (PU.notify doesn't do that). That seems odd. Can you explain what's going on there?

review: Needs Information
Revision history for this message
William Grant (wgrant) wrote :

On 05/01/16 01:57, Colin Watson wrote:
> Review: Needs Information
>
> This seems to change do_reject to only send the mail, and not
> actually change the status (PU.notify doesn't do that). That seems
> odd. Can you explain what's going on there?

The transaction is about to be aborted, so the only side-effect that can
depend on the status is the email. Since we can't reliably set the
status, I opted to be consistent and rely solely on the override.

Revision history for this message
Colin Watson (cjwatson) wrote :

OK, I hadn't traced things through to confirm that do_reject is always followed by an abort (and was put off by the original nascentupload.txt test that you removed; I now realise that it goes through a path that doesn't involve the abort). Now that I have, I agree with you.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/archiveuploader/nascentupload.py'
2--- lib/lp/archiveuploader/nascentupload.py 2014-08-13 07:47:43 +0000
3+++ lib/lp/archiveuploader/nascentupload.py 2015-12-30 23:59:31 +0000
4@@ -50,6 +50,7 @@
5 FromExistingOverridePolicy,
6 SourceOverride,
7 )
8+from lp.soyuz.enums import PackageUploadStatus
9 from lp.soyuz.interfaces.binarypackagename import IBinaryPackageNameSet
10 from lp.soyuz.interfaces.component import IComponentSet
11 from lp.soyuz.interfaces.queue import QueueInconsistentStateError
12@@ -755,17 +756,9 @@
13 if not self.queue_root:
14 self.queue_root = self._createQueueEntry()
15
16- # Avoid cyclic imports.
17- from lp.soyuz.interfaces.queue import QueueInconsistentStateError
18- try:
19- self.queue_root.setRejected()
20- except QueueInconsistentStateError:
21- # These exceptions are ignored, we want to force the rejected
22- # state.
23- pass
24-
25 with open(self.changes.filepath, "r") as changes_file_object:
26 self.queue_root.notify(
27+ status=PackageUploadStatus.REJECTED,
28 summary_text=self.rejection_message,
29 changes_file_object=changes_file_object, logger=self.logger)
30
31
32=== modified file 'lib/lp/archiveuploader/tests/nascentupload.txt'
33--- lib/lp/archiveuploader/tests/nascentupload.txt 2015-04-21 10:03:15 +0000
34+++ lib/lp/archiveuploader/tests/nascentupload.txt 2015-12-30 23:59:31 +0000
35@@ -489,17 +489,6 @@
36 cannot upload the same version within the same distribution. You
37 have to modify the source version and re-upload.
38
39-We rely on process-upload transaction rollback to not store bogus
40-queue entry in the database.
41-
42- >>> print ed_src_dup.queue_root.status.name
43- REJECTED
44-
45- >>> from lp.soyuz.enums import PackageUploadStatus
46- >>> hoary.getPackageUploads(
47- ... status=PackageUploadStatus.REJECTED, name=u"ed").count()
48- 1
49-
50
51 Staged Source and Binary upload with multiple binaries
52 ......................................................
53@@ -717,6 +706,7 @@
54 content, it should have all the required fields except the
55 'dsc_standards_version':
56
57+ >>> from lp.soyuz.enums import PackageUploadStatus
58 >>> inst_queue = hoary.getPackageUploads(
59 ... PackageUploadStatus.NEW, name=u'test75874', exact_match=True)[0]
60 >>> inst_spr = inst_queue.sources[0].sourcepackagerelease