launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === added file 'lib/lp/snappy/interfaces/webservice.py'
> --- lib/lp/snappy/interfaces/webservice.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/snappy/interfaces/webservice.py	2015-07-30 15:20:46 +0000
> @@ -0,0 +1,28 @@
> +# Copyright 2015 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""All the interfaces that are exposed through the webservice.
> +
> +There is a declaration in ZCML somewhere that looks like:
> +  <webservice:register module="lp.snappy.interfaces.webservice" />
> +
> +which tells `lazr.restful` that it should look for webservice exports here.
> +"""
> +
> +__all__ = [
> +    'ISnap',
> +    'ISnapBuild',
> +    'ISnapSet',
> +    ]
> +
> +# XXX: JonathanLange 2010-11-09 bug=673083: Legacy work-around for circular
> +# import bugs.  Break this up into a per-package thing.

We should indeed not further perpetuate the great horror of _schema_circular_imports. Can you move the snap-specific patches into this file directly?

> +from lp import _schema_circular_imports
> +from lp.snappy.interfaces.snap import (
> +    ISnap,
> +    ISnapSet,
> +    )
> +from lp.snappy.interfaces.snapbuild import ISnapBuild
> +
> +
> +_schema_circular_imports
> 
> === modified file 'lib/lp/snappy/tests/test_snapbuild.py'
> --- lib/lp/snappy/tests/test_snapbuild.py	2015-07-30 15:20:46 +0000
> +++ lib/lp/snappy/tests/test_snapbuild.py	2015-07-30 15:20:46 +0000
> @@ -224,3 +248,179 @@
>      def test_getByBuildFarmJobs_works_empty(self):
>          self.assertContentEqual(
>              [], getUtility(ISnapBuildSet).getByBuildFarmJobs([]))
> +
> +
> +class NonRedirectingMechanizeBrowser(PublisherMechanizeBrowser):
> +    """A `mechanize.Browser` that does not handle redirects."""
> +
> +    default_features = [
> +        feature for feature in PublisherMechanizeBrowser.default_features
> +        if feature != "_redirect"]

PackageUpload, SourcePackageRecipeBuild, LiveFSBuild and now SnapBuild all have something like this. Perhaps put it in lp.testing?

> +
> +
> +class TestSnapBuildWebservice(TestCaseWithFactory):
> +
> +    layer = LaunchpadFunctionalLayer
> +
> +    def setUp(self):
> +        super(TestSnapBuildWebservice, self).setUp()
> +        self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
> +        self.person = self.factory.makePerson()
> +        self.webservice = webservice_for_person(
> +            self.person, permission=OAuthPermission.WRITE_PRIVATE)
> +        self.webservice.default_api_version = "devel"
> +        login(ANONYMOUS)
> +
> +    def getURL(self, obj):
> +        return self.webservice.getAbsoluteUrl(api_url(obj))
> +
> +    def test_properties(self):
> +        # The basic properties of a SnapBuild are sensible.
> +        db_build = self.factory.makeSnapBuild(
> +            requester=self.person,
> +            date_created=datetime(2014, 04, 25, 10, 38, 0, tzinfo=pytz.UTC))
> +        build_url = api_url(db_build)
> +        logout()
> +        build = self.webservice.get(build_url).jsonBody()
> +        with person_logged_in(self.person):
> +            self.assertEqual(self.getURL(self.person), build["requester_link"])
> +            self.assertEqual(self.getURL(db_build.snap), build["snap_link"])
> +            self.assertEqual(
> +                self.getURL(db_build.archive), build["archive_link"])
> +            self.assertEqual(
> +                self.getURL(db_build.distro_arch_series),
> +                build["distro_arch_series_link"])
> +            self.assertEqual("Release", build["pocket"])
> +            self.assertIsNone(build["score"])
> +            self.assertFalse(build["can_be_rescored"])
> +            self.assertFalse(build["can_be_cancelled"])
> +
> +    def test_public(self):
> +        # A SnapBuild with a public Snap and archive is itself public.
> +        db_build = self.factory.makeSnapBuild()
> +        build_url = api_url(db_build)
> +        unpriv_webservice = webservice_for_person(
> +            self.factory.makePerson(), permission=OAuthPermission.WRITE_PUBLIC)
> +        unpriv_webservice.default_api_version = "devel"
> +        logout()
> +        self.assertEqual(200, self.webservice.get(build_url).status)
> +        self.assertEqual(200, unpriv_webservice.get(build_url).status)
> +
> +    def test_private_snap(self):
> +        # A SnapBuild with a private Snap is private.
> +        db_team = self.factory.makeTeam(
> +            owner=self.person, visibility=PersonVisibility.PRIVATE)
> +        with person_logged_in(self.person):
> +            db_build = self.factory.makeSnapBuild(
> +                requester=self.person, owner=db_team)
> +            build_url = api_url(db_build)
> +        unpriv_webservice = webservice_for_person(
> +            self.factory.makePerson(), permission=OAuthPermission.WRITE_PUBLIC)
> +        unpriv_webservice.default_api_version = "devel"
> +        logout()
> +        self.assertEqual(200, self.webservice.get(build_url).status)
> +        # 404 since we aren't allowed to know that the private team exists.
> +        self.assertEqual(404, unpriv_webservice.get(build_url).status)
> +
> +    def test_private_archive(self):
> +        # A SnapBuild with a private archive is private.
> +        db_archive = self.factory.makeArchive(owner=self.person, private=True)
> +        with person_logged_in(self.person):
> +            db_build = self.factory.makeSnapBuild(archive=db_archive)
> +            build_url = api_url(db_build)
> +        unpriv_webservice = webservice_for_person(
> +            self.factory.makePerson(), permission=OAuthPermission.WRITE_PUBLIC)
> +        unpriv_webservice.default_api_version = "devel"
> +        logout()
> +        self.assertEqual(200, self.webservice.get(build_url).status)
> +        self.assertEqual(401, unpriv_webservice.get(build_url).status)
> +
> +    def test_cancel(self):
> +        # The owner of a build can cancel it.
> +        db_build = self.factory.makeSnapBuild(requester=self.person)
> +        db_build.queueBuild()
> +        build_url = api_url(db_build)
> +        unpriv_webservice = webservice_for_person(
> +            self.factory.makePerson(), permission=OAuthPermission.WRITE_PUBLIC)
> +        unpriv_webservice.default_api_version = "devel"
> +        logout()
> +        build = self.webservice.get(build_url).jsonBody()
> +        self.assertTrue(build["can_be_cancelled"])
> +        response = unpriv_webservice.named_post(build["self_link"], "cancel")
> +        self.assertEqual(401, response.status)
> +        response = self.webservice.named_post(build["self_link"], "cancel")
> +        self.assertEqual(200, response.status)
> +        build = self.webservice.get(build_url).jsonBody()
> +        self.assertFalse(build["can_be_cancelled"])
> +        with person_logged_in(self.person):
> +            self.assertEqual(BuildStatus.CANCELLED, db_build.status)
> +
> +    def test_rescore(self):
> +        # Buildd administrators can rescore builds.
> +        db_build = self.factory.makeSnapBuild(requester=self.person)
> +        db_build.queueBuild()
> +        build_url = api_url(db_build)
> +        buildd_admin = self.factory.makePerson(
> +            member_of=[getUtility(ILaunchpadCelebrities).buildd_admin])
> +        buildd_admin_webservice = webservice_for_person(
> +            buildd_admin, permission=OAuthPermission.WRITE_PUBLIC)
> +        buildd_admin_webservice.default_api_version = "devel"
> +        logout()
> +        build = self.webservice.get(build_url).jsonBody()
> +        self.assertEqual(2505, build["score"])
> +        self.assertTrue(build["can_be_rescored"])
> +        response = self.webservice.named_post(
> +            build["self_link"], "rescore", score=5000)
> +        self.assertEqual(401, response.status)
> +        response = buildd_admin_webservice.named_post(
> +            build["self_link"], "rescore", score=5000)
> +        self.assertEqual(200, response.status)
> +        build = self.webservice.get(build_url).jsonBody()
> +        self.assertEqual(5000, build["score"])
> +
> +    def makeNonRedirectingBrowser(self, person):
> +        # The test browser can only work with the appserver, not the
> +        # librarian, so follow one layer of redirection through the
> +        # appserver and then ask the librarian for the real file.
> +        browser = Browser(mech_browser=NonRedirectingMechanizeBrowser())
> +        browser.handleErrors = False
> +        with person_logged_in(person):
> +            browser.addHeader(
> +                "Authorization", "Basic %s:test" % person.preferredemail.email)
> +        return browser
> +
> +    def assertCanOpenRedirectedUrl(self, browser, url):
> +        redirection = self.assertRaises(HTTPError, browser.open, url)
> +        self.assertEqual(303, redirection.code)
> +        urlopen(redirection.hdrs["Location"]).close()
> +
> +    def test_logs(self):
> +        # API clients can fetch the build and upload logs.
> +        db_build = self.factory.makeSnapBuild(requester=self.person)
> +        db_build.setLog(self.factory.makeLibraryFileAlias("buildlog.txt.gz"))
> +        db_build.storeUploadLog("uploaded")
> +        build_url = api_url(db_build)
> +        logout()
> +        build = self.webservice.get(build_url).jsonBody()
> +        browser = self.makeNonRedirectingBrowser(self.person)
> +        self.assertIsNotNone(build["build_log_url"])
> +        self.assertCanOpenRedirectedUrl(browser, build["build_log_url"])
> +        self.assertIsNotNone(build["upload_log_url"])
> +        self.assertCanOpenRedirectedUrl(browser, build["upload_log_url"])
> +
> +    def test_getFileUrls(self):
> +        # API clients can fetch files attached to builds.
> +        db_build = self.factory.makeSnapBuild(requester=self.person)
> +        db_files = [
> +            self.factory.makeSnapFile(snapbuild=db_build) for i in range(2)]
> +        build_url = api_url(db_build)
> +        file_urls = [
> +            ProxiedLibraryFileAlias(file.libraryfile, db_build).http_url
> +            for file in db_files]
> +        logout()
> +        response = self.webservice.named_get(build_url, "getFileUrls")
> +        self.assertEqual(200, response.status)
> +        self.assertContentEqual(file_urls, response.jsonBody())
> +        browser = self.makeNonRedirectingBrowser(self.person)
> +        for file_url in file_urls:
> +            self.assertCanOpenRedirectedUrl(browser, file_url)


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-webservice/+merge/265700
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.