launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code

108	+ def _findSourcePublication(self, packageupload):
109	+ """Find destination source publishing record of the packageupload."""
110	+ if packageupload.package_name is None:
111	+ self.setComponents(self.tarfile_path)

It's a bit weird to have this take packageupload as an argument, but then use self.tarfile_path in the other path. I'd take filename as an argument.

112	+ return packageupload.archive.getPublishedSources(
113	+ name=packageupload.package_name, exact_match=True,
114	+ distroseries=packageupload.distroseries,
115	+ pocket=packageupload.pocket).first()

That needs to use self.package_name if packageupload.package_name is unset.

613	base_json_data=simplejson.dumps(
614	- {'sourcepackagerelease': sourcepackagerelease.id,
615	+ {'packageupload': packageupload.id,
616	+ 'sourcepackagerelease': sourcepackagerelease.id,
617	'libraryfilealias': libraryfilealias.id}))

The job actually only uses the sourcepackagename and distroseries. It'd be simpler, clearer and easier to test if this code didn't touch the sourcepackagerelease or packageupload.

Your test changes look good. But we'll want to QA this fairly heavily on dogfood, and not deploy it until saucy's well released.
-- 
https://code.launchpad.net/~ursinha/launchpad/qa-bad-bug-1201485-rebased/+merge/191077
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.