Merge ~apw/launchpad:archive-removeCopyNotification-fixes into launchpad:master

Proposed by Andy Whitcroft
Status: Needs review
Proposed branch: ~apw/launchpad:archive-removeCopyNotification-fixes
Merge into: launchpad:master
Diff against target: 34 lines (+3/-2)
2 files modified
lib/lp/soyuz/model/archive.py (+2/-1)
lib/lp/soyuz/model/packagecopyjob.py (+1/-1)
Reviewer Review Type Date Requested Status
Colin Watson (community) Needs Fixing
Review via email: mp+431805@code.qastaging.launchpad.net

Commit message

removeCopyNotification: error handling fixes

Handle invalid and already acknowledged errors more cleanly reporting NotFound and Gone errors respectively.

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

Strictly, I think 410 Gone is really meant to be for cases where the resource identified by the request URL is no longer available, which isn't quite the case here since the archive still exists. Similarly, 404 Not Found would indicate that the archive doesn't exist.

I think we should probably just use 400 Bad Request for this sort of thing. The clearest way to do that would be to invent a new exception type in `lp.soyuz.interfaces.archive` decorated with `@error_status(http.client.BAD_REQUEST)`; you could use the same exception with a different message for the two cases here if you wanted, sticking a `try:/except NotFoundError:` around the `PlainPackageCopyJob.get` call to re-raise it as an exception with the right error status decoration.

You'll also need to update tests for these corner cases. It looks as though they mostly already exist in `lp.soyuz.tests.test_archive.TestRemovingCopyNotifications`, although you might need to add a test for the case of a job ID that doesn't exist at all.

Finally, I wonder if `test_removeCopyNotification_raises_for_wrong_archive` really tests what it claims to be testing. I haven't stepped through it so I might be missing something, but I don't actually see any code that ensures that the job ID is for the context archive; I suspect that the test only passes because the job ID it supplies isn't for a failed job, so it ends up being silently equivalent to `test_removeCopyNotification_raises_for_not_failed`. We'll need to fix this. I'd suggest that all these tests should use `self.assertRaisesWithContent` to assert on the exception string as well as its type; being painfully precise about the exceptions we expect is a good way to avoid this sort of accidental-pass situation.

review: Needs Fixing

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to status/vote changes: