launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code

44	=== modified file 'lib/lp/bugs/browser/tests/test_bugs.py'

It might be worth adding a test for the behaviour when the sharing policy is PUBLIC.

388	=== modified file 'lib/lp/bugs/model/bug.py'
389	--- lib/lp/bugs/model/bug.py 2012-10-04 14:22:59 +0000
390	+++ lib/lp/bugs/model/bug.py 2012-10-07 23:54:23 +0000
391	@@ -2629,7 +2629,7 @@
392	# XXX: ElliotMurphy 2007-06-14: If we ever allow filing private
393	# non-security bugs, this test might be simplified to checking
394	# params.private.
395	- if (IProduct.providedBy(params.target) and params.target.private_bugs
396	+ if (IProduct.providedBy(params.target)
397	and params.target.bug_sharing_policy is None
398	and params.information_type not in SECURITY_INFORMATION_TYPES):
399	# Subscribe the bug supervisor to all bugs,

private_bugs was a top-level conjunct, so the block could never fire unless it was set. By removing private_bugs we're treating it as if it was false everywhere, so this whole block should be deleted.

408	-class TestBugPrivateAndSecurityRelatedUpdatesMixin:
409	+class TestBugPrivateAndSecurityRelatedUpdatesMixin(TestCaseWithFactory):

This is no longer a mixin, so please rename the class.

448	=== modified file 'lib/lp/bugs/scripts/bugimport.py'
449	--- lib/lp/bugs/scripts/bugimport.py 2012-06-29 08:40:05 +0000
450	+++ lib/lp/bugs/scripts/bugimport.py 2012-10-07 23:54:23 +0000
451	@@ -295,9 +295,6 @@
452	
453	private = get_value(bugnode, 'private') == 'True'
454	security_related = get_value(bugnode, 'security_related') == 'True'
455	- # If the product has private_bugs, we force private to True.
456	- if self.product.private_bugs:
457	- private = True
458	information_type = convert_to_information_type(
459	private, security_related)

Do we want to add an assertion in the importer to reject imports into a product with a policy other than PUBLIC?

461	@@ -323,11 +320,6 @@
462	bug.linkMessage(msg)
463	self.createAttachments(bug, msg, commentnode)
464	
465	- # Security bugs must be created private, so set it correctly.
466	- if not self.product.private_bugs:
467	- information_type = convert_to_information_type(
468	- private, security_related)
469	- bug.transitionToInformationType(information_type, owner)

This block should simply be enabled, not removed. We'd ideally also create it with the correct information_type, now that we have that capability.

516	def test_createBug_proprietary_subscribers(self):

This test will probably fail due to your change on line 395. Once that block is removed, this test no longer has value and can be removed.

650	- field_names = [
651	- "project_reviewed",
652	- "license_approved",
653	- "active",
654	- "private_bugs",
655	- "reviewer_whiteboard",
656	- ]
657	+ field_names = ["project_reviewed", "license_approved", "active",
658	+ "reviewer_whiteboard"]

I think we prefer the old style, but we certainly don't prefer the new style without an initial newline.

795	"""See `IDistribution.`"""

orly
-- 
https://code.launchpad.net/~stevenk/launchpad/death-to-private_bugs/+merge/128398
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.