Code review comment for ~apw/launchpad:archive-removeCopyNotification-fixes

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

« Back to merge proposal