launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

39	+ @property
40	+ def show_information_type_in_ui(self):
41	+ feature_flag_base = 'disclosure.show_information_type_in_ui.enabled'
42	+ if IBug.providedBy(self.context):
43	+ pass
44	+ elif IBranch.providedBy(self.context):
45	+ feature_flag_base = feature_flag_base.replace('ui', 'branch_ui')
46	+ else:
47	+ raise NotImplemented()
48	+ return bool(getFeatureFlag(feature_flag_base))

Since InformationTypePortlet isn't used as its own view (always as a mixin for an edit form class), this horror can probably die or be replaced with "raise NotImplementedError()". It should also possibly be renamed to InformationTypePortletMixin, and the module name should be informationtype, not information_type.

60	+ if (
61	+ self.context.information_type == InformationType.USERDATA and
62	+ self.show_userdata_as_private):

Why is that extra newline there?

if (self.context.information_type == InformationType.USERDATA
    and self.show_userdata_as_private):

71	+ if (
72	+ self.context.information_type == InformationType.USERDATA and
73	+ self.show_userdata_as_private):

Likewise.

107	def initialize(self):
108	super(BugView, self).initialize()

These can be deleted, as the method looks empty apart from that super() call.

433	+class TestBranchPrivacyPortlet(TestCaseWithFactory):

That's a bit odd. Is there really nowhere else that tests the portlet?

485	+ This branch contains <strong id="information-type" tal:content="view/information_type"></strong> information&nbsp;
486	+ <div id='information-type-description' style='padding-top: 5px' tal:content="view/information_type_description"></div>

The nbsp before a new div doesn't strike me as particularly useful. Do you know why it's there?
-- 
https://code.launchpad.net/~stevenk/launchpad/branch-information_type-ui/+merge/107909
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.