launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'lib/lp/code/browser/configure.zcml'
> --- lib/lp/code/browser/configure.zcml	2018-08-29 15:38:17 +0000
> +++ lib/lp/code/browser/configure.zcml	2018-11-06 11:54:49 +0000
> @@ -877,6 +877,12 @@
>          template="../templates/gitrepository-delete.pt"/>
>      <browser:page
>          for="lp.code.interfaces.gitrepository.IGitRepository"
> +        class="lp.code.browser.gitrepository.GitRepositoryPermissionsActivityView"
> +        permission="launchpad.Edit"
> +        name="+permissionsactivity"

Although today it just logs permissions-related activity, I intended GitActivity to be extendable to cover more general activity on repositories in time, hence the more general name; for example, other hosting providers show a summary of pushes in their analogous views.  Could you make this be GitRepositoryActivityView and (especially) just +activity to allow for this in future?

> +        template="../templates/gitrepository-activity.pt" />
> +    <browser:page
> +        for="lp.code.interfaces.gitrepository.IGitRepository"
>          class="lp.code.browser.gitsubscription.GitSubscriptionAddView"
>          permission="launchpad.AnyPerson"
>          name="+subscribe"
> 
> === modified file 'lib/lp/code/browser/gitrepository.py'
> --- lib/lp/code/browser/gitrepository.py	2018-08-31 13:36:38 +0000
> +++ lib/lp/code/browser/gitrepository.py	2018-11-06 11:54:49 +0000
> @@ -771,3 +777,14 @@
>      @property
>      def cancel_url(self):
>          return canonical_url(self.context)
> +
> +
> +class GitRepositoryPermissionsActivityView(LaunchpadView):
> +
> +    @property
> +    def page_title(self):
> +        return "Permissions Activity"

Page titles should be "Sentence case" rather than "Headline Case", per https://dev.launchpad.net/UserInterfaceWording#Capitalisation.  (Though this may be moot per my earlier comment, since "Repository activity" is probably too much and this could just be "Activity".  page_title goes in the breadcrumbs at the top of the page.)

GitHub calls the analogous page "Audit log"; GitLab calls it "Activity".  Since "Activity" is in use elsewhere in Launchpad, that choice seems best.

> +
> +    @property
> +    def activity(self):
> +        return self.context.getPrecachedActivity()
> 
> === modified file 'lib/lp/code/browser/tests/test_gitrepository.py'
> --- lib/lp/code/browser/tests/test_gitrepository.py	2018-09-11 15:04:43 +0000
> +++ lib/lp/code/browser/tests/test_gitrepository.py	2018-11-06 11:54:49 +0000
> @@ -1154,3 +1154,53 @@
>          self.assertEqual(
>              ["Repository %s deleted." % name],
>              get_feedback_messages(browser.contents))
> +
> +
> +class TestGitRepositoryPermissionsActivityView(BrowserTestCase):
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_render(self):
> +        requester = self.factory.makePerson()
> +        repository = removeSecurityProxy(
> +            self.factory.makeGitRepository(owner=requester))
> +
> +        rule = self.factory.makeGitRule(repository)
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=requester, can_push=True, can_create=True)
> +
> +        browser = self.getViewBrowser(
> +            repository, "+permissionsactivity", rootsite="code",
> +            user=repository.owner)
> +        self.assertIsNotNone(
> +            find_tag_by_id(browser.contents, 'activity-listing'))

Can we have some kind of tests that cover the contents of this table?

> +
> +    def test_activity_query_count(self):
> +        requester = self.factory.makePerson()
> +        repository = repository = removeSecurityProxy(

"repository = repository = ..." seems a bit odd.  Typo?

> +            self.factory.makeGitRepository(owner=requester))
> +        rule = self.factory.makeGitRule(repository)
> +
> +        grantees = iter([self.factory.makePerson() for _ in range(4)])
> +
> +        def login_and_view():
> +            browser = self.getViewBrowser(
> +                repository,
> +                "+permissionsactivity",
> +                rootsite="code",
> +                user=repository.owner)
> +            self.assertIsNotNone(
> +                find_tag_by_id(browser.contents, 'activity-listing'))
> +
> +        def create_activity():
> +            self.factory.makeGitRuleGrant(
> +                rule=rule,
> +                grantee=next(grantees),
> +                can_push=True,
> +                can_create=True)
> +
> +        recorder1, recorder2 = record_two_runs(
> +            login_and_view,
> +            create_activity,
> +            2)
> +        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
> 
> === modified file 'lib/lp/code/model/gitactivity.py'
> --- lib/lp/code/model/gitactivity.py	2018-09-11 19:43:53 +0000
> +++ lib/lp/code/model/gitactivity.py	2018-11-06 11:54:49 +0000
> @@ -83,6 +85,17 @@
>          else:
>              return None
>  
> +    @staticmethod
> +    def preloadDataForActivities(activities):
> +        # Utility to load related data for a list of GitActivity
> +        person_ids = set()
> +        for activity in activities:
> +            person_ids.add(activity.changer_id)
> +            person_ids.add(activity.changee_id)
> +        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
> +            person_ids, need_validity=True))
> +        return activities

I'm not sure this really puts its weight here, since it only has one call site.  Can you just make this a nested function in GitRepository.getPrecachedActivity instead?

> +
>  
>  def _make_rule_value(rule, position=None):
>      return {
> 
> === modified file 'lib/lp/code/model/gitrepository.py'
> --- lib/lp/code/model/gitrepository.py	2018-10-22 16:24:46 +0000
> +++ lib/lp/code/model/gitrepository.py	2018-11-06 11:54:49 +0000
> @@ -1287,6 +1287,11 @@
>          return Store.of(self).find(GitActivity, *clauses).order_by(
>              Desc(GitActivity.date_changed), Desc(GitActivity.id))
>  
> +    def getPrecachedActivity(self, changed_after=None):
> +        results = self.getActivity(changed_after)
> +        loader = partial(GitActivity.preloadDataForActivities)

partial without any args or kwargs is a no-op.

> +        return DecoratedResultSet(results, pre_iter_hook=loader)
> +
>      def canBeDeleted(self):
>          """See `IGitRepository`."""
>          # Can't delete if the repository is associated with anything.
> 
> === added file 'lib/lp/code/templates/gitrepository-activity.pt'
> --- lib/lp/code/templates/gitrepository-activity.pt	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/templates/gitrepository-activity.pt	2018-11-06 11:54:49 +0000
> @@ -0,0 +1,74 @@
> +<html
> +  xmlns="http://www.w3.org/1999/xhtml";
> +  xmlns:tal="http://xml.zope.org/namespaces/tal";
> +  xmlns:metal="http://xml.zope.org/namespaces/metal";
> +  xmlns:i18n="http://xml.zope.org/namespaces/i18n";
> +  metal:use-macro="view/macro:page/main_only"
> +  i18n:domain="launchpad">
> +<body>
> +
> +<metal:macros fill-slot="bogus">
> +    <metal:macro define-macro="activity-value">

Can you pick one of two- or four-space indentation throughout this file and stick to it?  (Two-space is a bit more usual for .pt files, I think.)

> +        <ul tal:condition="value">
> +          <li><b>Ref Pattern:</b> <span tal:content="value/ref_pattern" /></li>

Should be sentence case, but in any case we generally don't want to expose "ref" in the UI these days.  I think just "Pattern" would do.

Also, <strong> throughout rather than <b>.

> +          <tal:comment condition="nothing">
> +            Grant changes
> +          </tal:comment>
> +          <span tal:condition="python:'can_create' in value"><b>create: </b><span tal:content="value/can_create" />, </span>
> +          <span tal:condition="python:'can_push' in value"><b>push: </b><span tal:content="value/can_push" />, </span>
> +          <span tal:condition="python:'can_force_push' in value"><b>force-push: </b><span tal:content="value/can_force_push" /></span>

Rather than something like "<strong>create</strong>: true, <strong>push</strong>: true, <strong>force-push</strong>: false", I'd rather see "<strong>Permissions</strong>: create, push".

> +          <tal:comment condition="nothing">
> +            Rule changes
> +          </tal:comment>
> +          <li tal:condition="python: 'position' in value">Rule position: <span tal:content="value/position" /></li>

I'd move this up so that it immediately follows the pattern.  And should "Rule position" be in <strong> for consistency?

> +        </ul>
> +    </metal:macro>
> +</metal:macros>
> +
> +<div metal:fill-slot="main">
> +    <div class="yui-g">
> +      <div class="top-portlet">
> +        <h1>Activity log for repository <span tal:replace="context/display_name" /></h1>

This should probably be in the "label" property of the view instead, which lib/lp/app/templates/base-layout.pt uses to build an <h1> tag.

> +        <table id="activity-listing" class="listing">
> +          <thead>
> +            <tr>
> +              <th>Date</th>
> +              <th>Author</th>
> +              <th>Target</th>
> +              <th>What changed</th>
> +              <th>Old value</th>
> +              <th>New value</th>
> +            </tr>
> +          </thead>
> +          <tbody>
> +            <tr tal:repeat="log view/activity">
> +              <tal:comment condition="nothing">
> +                XXX: Gavin Panella 2009-08-12 bug=412963: Using strftime()
> +                here because fmt:datetime changes timezone, even though we
> +                always want to show only UTC.
> +              </tal:comment>
> +              <td tal:content="python:log.date_changed.strftime('%Y-%m-%d %T')">
> +                2004-09-24 12:04:43
> +              </td>
> +              <td tal:content="structure log/changer/fmt:link">Changer</td>
> +              <td tal:content="structure log/changee/fmt:link">Changee</td>
> +              <td tal:content="log/what_changed/title">description</td>
> +              <td>
> +                <tal:activity define="value log/old_value">
> +                    <metal:logs use-macro="template/macros/activity-value" />
> +                </tal:activity>
> +              </td>
> +              <td>
> +                <tal:activity define="value log/new_value">
> +                    <metal:logs use-macro="template/macros/activity-value" />
> +                </tal:activity>
> +              </td>
> +            </tr>
> +          </tbody>
> +        </table>
> +      </div>
> +    </div>
> +</div>
> +
> +</body>
> +</html>


-- 
https://code.launchpad.net/~twom/launchpad/git-permissions-activity-view/+merge/358375
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.