launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/testfix-builders-visibility into lp:launchpad.

Commit message:
Check visibility of the archive owner, not the archive; unembargoed builds in private archives for public teams can be rendered normally.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/testfix-builders-visibility/+merge/170941

builders-visibility restricted build visibility on /builders (arguably) a bit too much and broke lib/lp/soyuz/stories/soyuz/xx-private-builds.txt.  This widens it a little again to make that test happy.

09:26 <cjwatson> wgrant: The "Unembargoed builds" section of lib/lp/soyuz/stories/soyuz/xx-private-builds.txt is deliberately testing a property that my branch makes not true.  Do you think perhaps I ought to be testing just for visibility of the archive owner, rather than of the archive?
09:28 <cjwatson> Since a private team means the very existence of the archive is confidential, while a private archive whose builds are being copied elsewhere is not quite so confidential.
09:28 <cjwatson> I'm not sure whether this is test-highlighting-wrong-design or fundamentally-wrong-test.
09:30 <cjwatson> It is, I suppose, slightly weird that with my branch you can read the +build if you have the link, but /builders refuses to link to it ...
09:32 <cjwatson> Archive.name is always public, just not necessarily Archive.owner.name.
09:54 <wgrant> cjwatson: Right, your branch isn't strictly correct, but I felt it was correct enough that we should perhaps not overcomplicate things.
09:55 <cjwatson> https://code.launchpad.net/~cjwatson/launchpad/testfix-builders-visibility is more correct in that sense, if that's a direction you're happy with.
09:55 <wgrant> Checking visibility of archive.owner would work, but I'm not sure it's worth the complication.
09:55 <wgrant> Maybe
09:55 <wgrant> Really we need to have a definition of build privacy that isn't completely insane
09:55 <wgrant> But such is launchpad
09:56 <wgrant> cjwatson: Besides being slightly less obvious, that seems fine.
09:56 <cjwatson> Arguably this belongs in ViewBinaryPackageBuild or something, but then I'd have had to do something separate for recipes ...
09:57 <wgrant> It's a pretty unfortunate hack either way, so I'm glad it's restricted to the view :)
09:57 <cjwatson> And there's possibly an argument that it's the formatter that's trying to poke about inside the build's archive and therefore the formatter should also be the thing that checks whether it's OK to do so
-- 
https://code.launchpad.net/~cjwatson/launchpad/testfix-builders-visibility/+merge/170941
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/testfix-builders-visibility into lp:launchpad.

=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py	2013-06-21 13:37:38 +0000
+++ lib/lp/app/browser/tales.py	2013-06-22 08:59:27 +0000
@@ -1750,7 +1750,7 @@
     def link(self, view_name, rootsite=None):
         build = self._context
         if (not check_permission('launchpad.View', build) or
-            not check_permission('launchpad.View', build.archive)):
+            not check_permission('launchpad.View', build.archive.owner)):
             return 'private job'
 
         url = self.url(view_name=view_name, rootsite=rootsite)