launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

Richard Harding has proposed merging lp:~rharding/launchpad/changing_maintainer_1077841 into lp:launchpad.

Commit message:
Add private property to Product so that it has LP.cache.context.private for the JS pickers to determine context.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1077941 in Launchpad itself: "changing maintainer of proprietary project gives incorrect message"
  https://bugs.launchpad.net/launchpad/+bug/1077941

For more details, see:
https://code.launchpad.net/~rharding/launchpad/changing_maintainer_1077841/+merge/137849

= Summary =

The bug is that the JavaScript in the picker is throwing an invalid message to
the user when they change the maintainer of a project. In looking, this is
because that JavaScript it determine if things are private by looking at
LP.cache.context.private, which project isn't implementing.


== Pre Implementation ==

Talked with Abel some during implementation to debug .zcml work.


== Implementation Notes ==

The trick here is in the IProduct interface through IInformationType and from
IPrivacy. However, it's not exported so it doesn't get dumped with the rest of
the Product data into LP.cache.context when you're on the product home screen.

To fix this, I added the property to the IProductPublic interface and removed
the IInformationType from the zcml. I then had to allow the attribute
IInformationType was supplying. This basically hid away the private from
IPrivacy, but exposed my updated one in IProjectPublic.

The private method on the Product model is just a simple check against
PRIVATE_INFORMATION_TYPES.


== Tests ==

For testing, I just updated the existing information type tests to not only
verify it gets the right information type, but that the private property is
correct.

The JavaScript is correct as long as the context is correct so there aren't
any changes actually required in there.

-- 
https://code.launchpad.net/~rharding/launchpad/changing_maintainer_1077841/+merge/137849
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rharding/launchpad/changing_maintainer_1077841 into lp:launchpad.

=== modified file 'lib/lp/app/templates/inline-picker.pt'
--- lib/lp/app/templates/inline-picker.pt	2012-06-15 16:23:50 +0000
+++ lib/lp/app/templates/inline-picker.pt	2012-12-04 14:05:23 +0000
@@ -23,7 +23,7 @@
   <script tal:condition="view/can_write"
         tal:content="structure string:
 LPJS.use('lp.app.picker', 'lp.client', function(Y) {
-    var picker_config = ${view/json_config}
+    var picker_config = ${view/json_config};
     picker_config.validate_callback = Y.lp.app.picker.public_private_warning;
     Y.on('load', function(e) {
       Y.lp.app.picker.addPickerPatcher(

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2012-11-27 22:02:34 +0000
+++ lib/lp/registry/configure.zcml	2012-12-04 14:05:23 +0000
@@ -1268,11 +1268,10 @@
         <require
             permission="launchpad.Edit"
             interface="lp.registry.interfaces.product.IProductEditRestricted"/>
-        <allow
-            interface="lp.app.interfaces.informationtype.IInformationType"/>
         <require
             permission="launchpad.Edit"
             set_schema="lp.app.interfaces.informationtype.IInformationType"/>
+        <allow attributes="information_type" />
         <require
             permission="launchpad.Edit"
             set_attributes="answers_usage blueprints_usage codehosting_usage

=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py	2012-11-26 21:01:54 +0000
+++ lib/lp/registry/interfaces/product.py	2012-12-04 14:05:23 +0000
@@ -427,6 +427,13 @@
     def userCanView(user):
         """True if the given user has access to this product."""
 
+    private = exported(
+        Bool(
+            title=_("Product is confidential"), required=False,
+            readonly=True, default=False,
+            description=_(
+                "This product is visible only to those with access grants.")))
+
 
 class IProductLimitedView(IHasIcon, IHasLogo, IHasOwner, ILaunchpadUsage):
     """Attributes that must be visible for person with artifact grants
@@ -476,7 +483,6 @@
             description=_("The restricted team, moderated team, or person "
                           "who maintains the project information in "
                           "Launchpad.")))
-
     project = exported(
         ReferenceChoice(
             title=_('Part of'),

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-12-01 00:59:46 +0000
+++ lib/lp/registry/model/product.py	2012-12-04 14:05:23 +0000
@@ -442,6 +442,11 @@
         name='remote_product', allow_none=True, default=None)
 
     @property
+    def private(self):
+        """See `IProductPublic`"""
+        return self.information_type in PRIVATE_INFORMATION_TYPES
+
+    @property
     def date_next_suggest_packaging(self):
         """See `IProduct`
 
@@ -1807,10 +1812,6 @@
     def people(self):
         return getUtility(IPersonSet)
 
-    @property
-    def private(self):
-        return self.information_type in PRIVATE_INFORMATION_TYPES
-
     @classmethod
     def latest(cls, user, quantity=5):
         """See `IProductSet`."""

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2012-11-29 17:12:20 +0000
+++ lib/lp/registry/tests/test_product.py	2012-12-04 14:05:23 +0000
@@ -633,6 +633,7 @@
         store.reset()
         product = store.get(Product, product.id)
         self.assertEqual(InformationType.PROPRIETARY, product.information_type)
+        self.assertTrue(product.private)
 
     def test_product_information_type_default(self):
         # Default information_type is PUBLIC
@@ -640,6 +641,7 @@
         product = getUtility(IProductSet).createProduct(
             owner, 'fnord', 'Fnord', 'Fnord', 'test 1', 'test 2')
         self.assertEqual(InformationType.PUBLIC, product.information_type)
+        self.assertFalse(product.private)
 
     invalid_information_types = [info_type for info_type in
             InformationType.items if info_type not in