launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Review: Approve code

Looks good, just one suggestion, see below.

> This branch adapts Milestone to Product for IInformationType so that
> Milestone has its related Product's information_type. Abel's excellent
> security adapter work will secure Milestone once it provides
> IInformationType. So this branch does the adaptation and deals with the

I think we should nevertheless guard access to milestones and product
series instances with dedicated security adapters. People without
policy grants now get 404 errors because the main product has to be
traversed. Such an indirect security check is better than nothing, but
an explicit check if somebody can view a milestone/series seems
reasonable to me. Otherwise can imagine situations once we allow
grants for artifacts of private projects, where we need to allow
"partial access" to the main product. This could inadvertently give
access to milestoines/serieses too.

> remaining UI work for the web UI.
> 
> I discussed the approach here with abentley on a couple different occasions.
> 
> The primary work is to provide an adapter. I have also added two tests
> for that. One to check the adapter function itself, and another to ensure
> that Milestone has IInformationType. The rest of the work is adding the
> privacy portlet UI. I did not add the edit icon for this because this UI
> should informational. One could even argue it's un-needed since the privacy
> banner works, but I added it to be consistent with other pages. The main
> work for this part was updating the InformationTypePortletMixin to also
> handle an adapted object. The rest is just templating and zcml to wire up
> the portlet and a few tests.
> 
> I also added information_type_description to LaunchpadView as a small
> add-on fix. LaunchpadView is providing information_type, so it made since
> to me to also add the description.
> 
> I have a few tests I'm fixing but I thought it worth getting this up
> for review now. We are lint free, too.
> 
> 
> === modified file 'lib/lp/app/browser/informationtype.py'
> --- lib/lp/app/browser/informationtype.py	2012-09-17 15:19:10 +0000
> +++ lib/lp/app/browser/informationtype.py	2012-10-05 11:22:24 +0000
> @@ -9,28 +9,51 @@
>  from lazr.restful.interfaces import IJSONRequestCache
>  
>  from lp.app.enums import PRIVATE_INFORMATION_TYPES
> +from lp.app.interfaces.informationtype import IInformationType
>  from lp.app.utilities import json_dump_information_types
>  
>  
>  class InformationTypePortletMixin:
>  
>      def initialize(self):
> +        information_typed = IInformationType(self.context, None)
> +        if information_typed is None:
> +            information_typed = self.context

The three lines above are repeated in other methods. What about moving
them into a separate method?


-- 
https://code.launchpad.net/~deryck/launchpad/milestone-information-type/+merge/128227
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.