launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05793
[Merge] lp:~rvb/launchpad/authorize-bug-898237 into lp:launchpad
Raphaël Badin has proposed merging lp:~rvb/launchpad/authorize-bug-898237 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #898237 in Launchpad itself: "Unable to access Builder:+history if one of the sourcepackagerecipebuild is from a private archive I can't see"
https://bugs.launchpad.net/launchpad/+bug/898237
For more details, see:
https://code.launchpad.net/~rvb/launchpad/authorize-bug-898237/+merge/84269
This branch fixes the display of the list of builds on builder:+history in case one of the builds is private. Right now, if there is a single private build on such a page that one cannot see, an Unauthorized exception will be raised. To fix this, we now return a None value instead of the private build in the build list and this branch also changes the template to deal with a None value gracefully.
Note that I've moved getSpecificJobs where it belongs (because it deals with security proxied objects): in the view.
= Tests =
./bin/test -vvc test_builder_views test_build_history_private_build_display
./bin/test -vvc test_builder_views test_build_history_private_build_view
= Q/A =
As a non-privileged user, visit
https://qastaging.launchpad.net/builders/titanium/+history. The page should render ok and a private build should be displayed.
--
https://code.launchpad.net/~rvb/launchpad/authorize-bug-898237/+merge/84269
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/authorize-bug-898237 into lp:launchpad.
=== modified file 'lib/lp/buildmaster/interfaces/buildfarmjob.py'
--- lib/lp/buildmaster/interfaces/buildfarmjob.py 2011-11-09 11:36:44 +0000
+++ lib/lp/buildmaster/interfaces/buildfarmjob.py 2011-12-02 14:53:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=E0211,E0213
@@ -322,12 +322,6 @@
class IBuildFarmJobSet(Interface):
"""A utility representing a set of build farm jobs."""
- def getSpecificJobs(jobs):
- """Return the specific build jobs associated with each of the jobs
- in the provided job list.
-
- """
-
def getBuildsForBuilder(builder_id, status=None, user=None):
"""Return `IBuildFarmJob` records touched by a builder.
=== modified file 'lib/lp/buildmaster/model/buildfarmjob.py'
--- lib/lp/buildmaster/model/buildfarmjob.py 2011-11-21 12:23:40 +0000
+++ lib/lp/buildmaster/model/buildfarmjob.py 2011-12-02 14:53:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -11,8 +11,6 @@
import hashlib
-from itertools import groupby
-from operator import attrgetter
from lazr.delegates import delegates
import pytz
@@ -70,7 +68,6 @@
)
from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
from lp.registry.model.teammembership import TeamParticipation
-from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
class BuildFarmJobOld:
@@ -399,34 +396,6 @@
class BuildFarmJobSet:
implements(IBuildFarmJobSet)
- def getSpecificJobs(self, jobs):
- """See `IBuildFarmJobSet`."""
- # Adapt a list of jobs based on their job type.
- builds = []
- key = attrgetter('job_type.name')
- sorted_jobs = sorted(jobs, key=key)
- job_builds = {}
- for job_type_name, grouped_jobs in groupby(sorted_jobs, key=key):
- # Fetch the jobs in batches grouped by their job type.
- source = getUtility(
- ISpecificBuildFarmJobSource, job_type_name)
- builds = [build for build
- in source.getByBuildFarmJobs(list(grouped_jobs))
- if build is not None]
- is_binary_package_build = IBinaryPackageBuildSet.providedBy(
- source)
- for build in builds:
- if is_binary_package_build:
- job_builds[build.package_build.build_farm_job.id] = build
- else:
- job_builds[build.build_farm_job.id] = build
- # Return the corresponding builds.
- try:
- return [job_builds[job.id] for job in jobs]
- except KeyError:
- raise InconsistentBuildFarmJobError(
- "Could not find all the related specific jobs.")
-
def getBuildsForBuilder(self, builder_id, status=None, user=None):
"""See `IBuildFarmJobSet`."""
# Imported here to avoid circular imports.
=== modified file 'lib/lp/buildmaster/tests/test_buildfarmjob.py'
--- lib/lp/buildmaster/tests/test_buildfarmjob.py 2011-11-21 12:23:40 +0000
+++ lib/lp/buildmaster/tests/test_buildfarmjob.py 2011-12-02 14:53:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for `IBuildFarmJob`."""
@@ -12,7 +12,6 @@
import pytz
from storm.store import Store
-from testtools.matchers import Equals
from zope.component import getUtility
from zope.security.interfaces import Unauthorized
from zope.security.proxy import removeSecurityProxy
@@ -37,13 +36,8 @@
from lp.buildmaster.model.buildfarmjob import BuildFarmJob
from lp.testing import (
login,
- StormStatementRecorder,
TestCaseWithFactory,
)
-from lp.testing.matchers import HasQueryCount
-from lp.translations.interfaces.translationtemplatesbuild import (
- ITranslationTemplatesBuildSource,
- )
class TestBuildFarmJobMixin:
@@ -235,83 +229,6 @@
self.builder = self.factory.makeBuilder()
self.build_farm_job_set = getUtility(IBuildFarmJobSet)
- def createTranslationTemplateBuild(self):
- build_farm_job_source = getUtility(IBuildFarmJobSource)
- build_farm_job = build_farm_job_source.new(
- BuildFarmJobType.TRANSLATIONTEMPLATESBUILD)
- source = getUtility(ITranslationTemplatesBuildSource)
- branch = self.factory.makeBranch()
- return source.create(build_farm_job, branch)
-
- def createSourcePackageRecipeBuild(self):
- sprb = self.factory.makeSourcePackageRecipeBuild()
- Store.of(sprb).flush()
- return sprb
-
- def createBinaryPackageBuild(self):
- build = self.factory.makeBinaryPackageBuild()
- return build
-
- def createBuilds(self):
- builds = []
- for i in xrange(2):
- builds.append(self.createBinaryPackageBuild())
- builds.append(self.createTranslationTemplateBuild())
- builds.append(self.createSourcePackageRecipeBuild())
- return builds
-
- def test_getSpecificJobs(self):
- builds = self.createBuilds()
- specific_jobs = self.build_farm_job_set.getSpecificJobs(
- [build.build_farm_job for build in builds])
- self.assertContentEqual(
- builds, specific_jobs)
-
- def test_getSpecificJobs_preserves_order(self):
- builds = self.createBuilds()
- specific_jobs = self.build_farm_job_set.getSpecificJobs(
- [build.build_farm_job for build in builds])
- self.assertEqual(
- [(build.id, build.__class__) for build in builds],
- [(job.id, job.__class__) for job in specific_jobs])
-
- def test_getSpecificJobs_duplicated_builds(self):
- builds = self.createBuilds()
- duplicated_builds = builds + builds
- specific_jobs = self.build_farm_job_set.getSpecificJobs(
- [build.build_farm_job for build in duplicated_builds])
- self.assertEqual(len(duplicated_builds), len(specific_jobs))
-
- def test_getSpecificJobs_empty(self):
- self.assertContentEqual(
- [],
- self.build_farm_job_set.getSpecificJobs([]))
-
- def test_getSpecificJobs_sql_queries_count(self):
- # getSpecificJobs issues a constant number of queries.
- builds = self.createBuilds()
- build_farm_jobs = [build.build_farm_job for build in builds]
- flush_database_updates()
- with StormStatementRecorder() as recorder:
- self.build_farm_job_set.getSpecificJobs(
- build_farm_jobs)
- builds2 = self.createBuilds()
- build_farm_jobs.extend([build.build_farm_job for build in builds2])
- flush_database_updates()
- with StormStatementRecorder() as recorder2:
- self.build_farm_job_set.getSpecificJobs(
- build_farm_jobs)
- self.assertThat(recorder, HasQueryCount(Equals(recorder2.count)))
-
- def test_getSpecificJobs_no_specific_job(self):
- build_farm_job_source = getUtility(IBuildFarmJobSource)
- build_farm_job = build_farm_job_source.new(
- BuildFarmJobType.TRANSLATIONTEMPLATESBUILD)
- flush_database_updates()
- self.assertRaises(
- InconsistentBuildFarmJobError,
- self.build_farm_job_set.getSpecificJobs, [build_farm_job])
-
def test_getBuildsForBuilder_all(self):
# The default call without arguments returns all builds for the
# builder, and not those for other builders.
=== modified file 'lib/lp/soyuz/browser/build.py'
--- lib/lp/soyuz/browser/build.py 2011-11-11 15:55:53 +0000
+++ lib/lp/soyuz/browser/build.py 2011-12-02 14:53:29 +0000
@@ -19,6 +19,9 @@
]
+from itertools import groupby
+from operator import attrgetter
+
from lazr.batchnavigator import ListRangeFactory
from lazr.delegates import delegates
from lazr.restful.utils import safe_hasattr
@@ -27,6 +30,8 @@
implements,
Interface,
)
+from zope.security.interfaces import Unauthorized
+from zope.security.proxy import removeSecurityProxy
from canonical.launchpad import _
from canonical.launchpad.browser.librarian import (
@@ -59,7 +64,11 @@
UnexpectedFormData,
)
from lp.buildmaster.enums import BuildStatus
-from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJobSet
+from lp.buildmaster.interfaces.buildfarmjob import (
+ IBuildFarmJobSet,
+ InconsistentBuildFarmJobError,
+ ISpecificBuildFarmJobSource,
+ )
from lp.buildmaster.interfaces.packagebuild import IPackageBuild
from lp.code.interfaces.sourcepackagerecipebuild import (
ISourcePackageRecipeBuildSource,
@@ -463,8 +472,7 @@
Return a list of built CompleteBuild instances, or empty
list if no builds were contained in the received batch.
"""
- build_farm_job_set = getUtility(IBuildFarmJobSet)
- builds = build_farm_job_set.getSpecificJobs(
+ builds = getSpecificJobs(
[build.build_farm_job if IPackageBuild.providedBy(build) else build
for build in batch])
if not builds:
@@ -492,6 +500,45 @@
return complete_builds
+def getSpecificJobs(jobs):
+ """Return the specific build jobs associated with each of the jobs
+ in the provided job list.
+ """
+ builds = []
+ key = attrgetter('job_type.name')
+ sorted_jobs = sorted(jobs, key=key)
+ job_builds = {}
+ for job_type_name, grouped_jobs in groupby(sorted_jobs, key=key):
+ # Fetch the jobs in batches grouped by their job type.
+ source = getUtility(
+ ISpecificBuildFarmJobSource, job_type_name)
+ builds = [build for build
+ in source.getByBuildFarmJobs(list(grouped_jobs))
+ if build is not None]
+ is_binary_package_build = IBinaryPackageBuildSet.providedBy(
+ source)
+ for build in builds:
+ if is_binary_package_build:
+ job_builds[build.package_build.build_farm_job.id] = build
+ else:
+ try:
+ job_builds[build.build_farm_job.id] = build
+ except Unauthorized:
+ # If the build farm job is private, we will get an
+ # Unauthorized exception; we only use
+ # removeSecurityProxy to get the id of build_farm_job
+ # but the corresponding build returned in the list
+ # will be 'None'.
+ naked_build = removeSecurityProxy(build)
+ job_builds[naked_build.build_farm_job.id] = None
+ # Return the corresponding builds.
+ try:
+ return [job_builds[job.id] for job in jobs]
+ except KeyError:
+ raise InconsistentBuildFarmJobError(
+ "Could not find all the related specific jobs.")
+
+
class BuildRecordsView(LaunchpadView):
"""Base class used to present objects that contains build records.
=== modified file 'lib/lp/soyuz/browser/tests/test_builder_views.py'
--- lib/lp/soyuz/browser/tests/test_builder_views.py 2011-11-28 10:28:54 +0000
+++ lib/lp/soyuz/browser/tests/test_builder_views.py 2011-12-02 14:53:29 +0000
@@ -5,12 +5,19 @@
from functools import partial
+import soupmatchers
from storm.locals import Store
-from testtools.matchers import Equals
+from testtools.matchers import (
+ Equals,
+ MatchesAll,
+ )
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
-from canonical.database.sqlbase import flush_database_caches
+from canonical.database.sqlbase import (
+ flush_database_caches,
+ flush_database_updates,
+ )
from canonical.launchpad.ftests import login
from canonical.launchpad.webapp.servers import LaunchpadTestRequest
from canonical.testing.layers import LaunchpadFunctionalLayer
@@ -18,9 +25,15 @@
BuildFarmJobType,
BuildStatus,
)
-from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJobSource
+from lp.buildmaster.interfaces.buildfarmjob import (
+ IBuildFarmJobSource,
+ InconsistentBuildFarmJobError,
+ )
+from lp.registry.interfaces.person import IPersonSet
+from lp.soyuz.browser.build import getSpecificJobs
from lp.soyuz.browser.builder import BuilderEditView
from lp.testing import (
+ celebrity_logged_in,
StormStatementRecorder,
TestCaseWithFactory,
)
@@ -70,6 +83,84 @@
self.assertTrue(view.context.slaveStatusSentence.call_count == 0)
+class TestgetSpecificJobs(TestCaseWithFactory):
+
+ layer = LaunchpadFunctionalLayer
+
+ def createTranslationTemplateBuild(self):
+ build_farm_job_source = getUtility(IBuildFarmJobSource)
+ build_farm_job = build_farm_job_source.new(
+ BuildFarmJobType.TRANSLATIONTEMPLATESBUILD)
+ source = getUtility(ITranslationTemplatesBuildSource)
+ branch = self.factory.makeBranch()
+ return source.create(build_farm_job, branch)
+
+ def createSourcePackageRecipeBuild(self):
+ sprb = self.factory.makeSourcePackageRecipeBuild()
+ Store.of(sprb).flush()
+ return sprb
+
+ def createBinaryPackageBuild(self):
+ build = self.factory.makeBinaryPackageBuild()
+ return build
+
+ def createBuilds(self):
+ builds = []
+ for i in xrange(2):
+ builds.append(self.createBinaryPackageBuild())
+ builds.append(self.createTranslationTemplateBuild())
+ builds.append(self.createSourcePackageRecipeBuild())
+ return builds
+
+ def test_getSpecificJobs(self):
+ builds = self.createBuilds()
+ specific_jobs = getSpecificJobs(
+ [build.build_farm_job for build in builds])
+ self.assertContentEqual(
+ builds, specific_jobs)
+
+ def test_getSpecificJobs_preserves_order(self):
+ builds = self.createBuilds()
+ specific_jobs = getSpecificJobs(
+ [build.build_farm_job for build in builds])
+ self.assertEqual(
+ [(build.id, build.__class__) for build in builds],
+ [(job.id, job.__class__) for job in specific_jobs])
+
+ def test_getSpecificJobs_duplicated_builds(self):
+ builds = self.createBuilds()
+ duplicated_builds = builds + builds
+ specific_jobs = getSpecificJobs(
+ [build.build_farm_job for build in duplicated_builds])
+ self.assertEqual(len(duplicated_builds), len(specific_jobs))
+
+ def test_getSpecificJobs_empty(self):
+ self.assertContentEqual([], getSpecificJobs([]))
+
+ def test_getSpecificJobs_sql_queries_count(self):
+ # getSpecificJobs issues a constant number of queries.
+ builds = self.createBuilds()
+ build_farm_jobs = [build.build_farm_job for build in builds]
+ flush_database_updates()
+ with StormStatementRecorder() as recorder:
+ getSpecificJobs(build_farm_jobs)
+ builds2 = self.createBuilds()
+ build_farm_jobs.extend([build.build_farm_job for build in builds2])
+ flush_database_updates()
+ with StormStatementRecorder() as recorder2:
+ getSpecificJobs(build_farm_jobs)
+ self.assertThat(recorder, HasQueryCount(Equals(recorder2.count)))
+
+ def test_getSpecificJobs_no_specific_job(self):
+ build_farm_job_source = getUtility(IBuildFarmJobSource)
+ build_farm_job = build_farm_job_source.new(
+ BuildFarmJobType.TRANSLATIONTEMPLATESBUILD)
+ flush_database_updates()
+ self.assertRaises(
+ InconsistentBuildFarmJobError,
+ getSpecificJobs, [build_farm_job])
+
+
class TestBuilderHistoryView(TestCaseWithFactory):
layer = LaunchpadFunctionalLayer
@@ -91,12 +182,16 @@
self.addFakeBuildLog(build)
return build
- def createRecipeBuildWithBuilder(self):
+ def createRecipeBuildWithBuilder(self, private_branch=False):
+ branch2 = self.factory.makeAnyBranch()
branch1 = self.factory.makeAnyBranch()
- branch2 = self.factory.makeAnyBranch()
build = self.factory.makeSourcePackageRecipeBuild(
recipe=self.factory.makeSourcePackageRecipe(
branches=[branch1, branch2]))
+ if private_branch:
+ with celebrity_logged_in('admin'):
+ branch1.setPrivate(
+ True, getUtility(IPersonSet).getByEmail(ADMIN_EMAIL))
Store.of(build).flush()
removeSecurityProxy(build).builder = self.builder
self.addFakeBuildLog(build)
@@ -191,3 +286,25 @@
self.createTranslationTemplateBuildWithBuilder)
self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
+
+ def test_build_history_private_build_view(self):
+ self.createRecipeBuildWithBuilder()
+ self.createRecipeBuildWithBuilder(private_branch=True)
+ view = create_initialized_view(self.builder, '+history')
+ view.setupBuildList()
+
+ self.assertIn(None, view.complete_builds)
+
+ def test_build_history_private_build_display(self):
+ self.createRecipeBuildWithBuilder()
+ self.createRecipeBuildWithBuilder(private_branch=True)
+ view = create_initialized_view(self.builder, '+history')
+ private_build_icon_matcher = soupmatchers.HTMLContains(
+ soupmatchers.Tag(
+ 'Private build icon', 'img', attrs={'src': '/@@/private'}))
+ private_build_matcher = soupmatchers.HTMLContains(
+ soupmatchers.Tag('Private build', 'td', text='private Build'))
+
+ self.assertThat(
+ view.render(),
+ MatchesAll(private_build_matcher, private_build_icon_matcher))
=== modified file 'lib/lp/soyuz/templates/builds-list.pt'
--- lib/lp/soyuz/templates/builds-list.pt 2010-05-17 13:49:06 +0000
+++ lib/lp/soyuz/templates/builds-list.pt 2011-12-02 14:53:29 +0000
@@ -40,7 +40,7 @@
<table class="listing" tal:condition="batch">
<tbody>
<tal:batch_repeat repeat="build batch">
- <tr class="build-row">
+ <tr class="build-row" tal:condition="build">
<td class="left icon">
<tal:icon replace="structure build/image:icon" />
</td>
@@ -111,6 +111,13 @@
</div>
</td>
</tr>
+ <tr class="build-row" tal:condition="not: build">
+ <td class="left icon">
+ <img width="14" height="14" alt="[PRIVATE BUILD]"
+ title="Private" src="/@@/private" />
+ </td>
+ <td tal:condition="not: build">private Build</td>
+ </tr>
</tal:batch_repeat>
</tbody>
</table>
