launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> 
> === modified file 'lib/lp/code/browser/tests/test_branch.py'
> --- lib/lp/code/browser/tests/test_branch.py	2019-01-30 17:24:15 +0000
> +++ lib/lp/code/browser/tests/test_branch.py	2019-02-07 16:29:23 +0000
> @@ -649,6 +649,41 @@
>          self.assertThat(recorder, HasQueryCount(Equals(30)))
>  
>  
> +class TestBranchRescanView(BrowserTestCase):
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def _getBrowser(self, user=None):
> +        if user is None:
> +            browser = setupBrowser()
> +            logout()
> +            return browser
> +        else:
> +            login_person(user)
> +            return setupBrowserForUser(user=user)

Isn't this basically BrowserTestCase.getViewBrowser?  You ought to be able to use that.

> +
> +    def test_owner_can_see_rescan(self):
> +        branch = self.factory.makeAnyBranch()
> +        job = BranchScanJob.create(branch)
> +        job.job._status = JobStatus.FAILED
> +        branch_url = canonical_url(branch, view_name='+rescan', rootsite='code')
> +        browser = self._getBrowser(branch.owner)
> +        browser.open(branch_url)
> +        self.assertTrue('schedule a rescan' in browser.contents)
> +
> +    def test_other_user_can_see_rescan(self):

This isn't so much "other user" as "project owner".  (I'd normally read "other user" as "person without any relevant privileges".)

> +        other_user = self.factory.makePerson()
> +        product = self.factory.makeProduct(owner=other_user)
> +        branch = self.factory.makeAnyBranch(product=product)
> +        job = BranchScanJob.create(branch)
> +        job.job._status = JobStatus.FAILED
> +        branch_url = canonical_url(branch, view_name='+rescan', rootsite='code')
> +        browser = self._getBrowser(other_user)
> +        browser.open(branch_url)
> +        self.assertTrue('schedule a rescan' in browser.contents)

self.assertIn('schedule a rescan', browser.contents)

Same in test_owner_can_see_rescan.

Shouldn't there be a test that an ordinary mortal can't see this option?  We shouldn't generally have only positive permission tests.

(Also, super-important review comment: this should be followed by two blank lines rather than three.)

> +
> +
> +
>  class TestBranchViewPrivateArtifacts(BrowserTestCase):
>      """ Tests that branches with private team artifacts can be viewed.
>  


-- 
https://code.launchpad.net/~twom/launchpad/rescan-button-needs-less-permissions/+merge/362874
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.