launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> 
> === 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-08 15:33:30 +0000
> @@ -16,6 +16,7 @@
>      'GitRepositoryEditReviewerView',
>      'GitRepositoryEditView',
>      'GitRepositoryNavigation',
> +    'GitRepositoryPermissionsActivityView',

Remove "Permissions" (and reorder).

>      'GitRepositoryURL',
>      'GitRepositoryView',
>      ]
> @@ -230,6 +231,11 @@
>              enabled=bool(getFeatureFlag('webhooks.new.enabled')))
>  
>      @enabled_with_permission("launchpad.Edit")
> +    def activity(self):
> +        text = "View activity"
> +        return Link("+activity", text, icon="edit")

I don't think the edit icon is right for this.  icon="info" maybe?

> +
> +    @enabled_with_permission("launchpad.Edit")
>      def delete(self):
>          text = "Delete repository"
>          return Link("+delete", text, icon="trash-icon")
> @@ -771,3 +777,29 @@
>      @property
>      def cancel_url(self):
>          return canonical_url(self.context)
> +
> +
> +class GitRepositoryActivityView(LaunchpadView):
> +
> +    @property
> +    def page_title(self):
> +        return "Activity"

As long as it's just a string, this could just be a plain attribute rather than a property.

> +
> +    @property
> +    def label(self):
> +        return "Activity log"

For the <h1>, I'd make this be `"Activity log for %s" % self.context.display_name` or similar.

> +
> +    @property
> +    def activity(self):
> +        return self.context.getPrecachedActivity()

You could drop this and just use context/getPrecachedActivity in the template (TALES will call it automatically).

> +
> +    def display_permissions(self, values):

displayPermissions

> +        """Assemble a human readable list from the permissions changes."""
> +        permissions = []
> +        if values.get('can_create'):
> +            permissions.append('create')
> +        if values.get('can_push'):
> +            permissions.append('push')
> +        if values.get('can_force_push'):
> +            permissions.append('force-push')
> +        return ', '.join(permissions)
> 
> === 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-08 15:33:30 +0000
> @@ -137,7 +137,10 @@
>  from lp.registry.interfaces.distributionsourcepackage import (
>      IDistributionSourcePackage,
>      )
> -from lp.registry.interfaces.person import IPerson
> +from lp.registry.interfaces.person import (
> +    IPerson,
> +    IPersonSet

Needs a trailing comma (utilities/format-new-and-modified-imports should handle this).

> +    )
>  from lp.registry.interfaces.product import IProduct
>  from lp.registry.interfaces.role import IHasOwner
>  from lp.registry.interfaces.sharingjob import (
> @@ -1287,6 +1290,22 @@
>          return Store.of(self).find(GitActivity, *clauses).order_by(
>              Desc(GitActivity.date_changed), Desc(GitActivity.id))
>  
> +    def getPrecachedActivity(self, changed_after=None):
> +
> +        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
> +
> +        results = self.getActivity(changed_after)

This might be a reasonable use case for **kwargs, since getPrecachedActivity shouldn't care about the details and should just pass everything through.

> +        return DecoratedResultSet(
> +            results, pre_iter_hook=preloadDataForActivities)
> +
>      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-08 15:33:30 +0000
> @@ -0,0 +1,78 @@
> +<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">
> +      <ul tal:condition="value">
> +        <li><b>Pattern:</b> <span tal:content="value/ref_pattern" /></li>

<strong> again.

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

The nesting of the <strong> here is a bit weird; perhaps move it to the next line and indent it?

> +          <span metal:define-slot="permissions"></span>
> +        </li>
> +      </ul>
> +    </metal:macro>
> +  </metal:macros>
> +
> +  <div metal:fill-slot="main">
> +    <div class="yui-g">
> +      <div class="top-portlet">
> +        <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">
> +                    <span metal:fill-slot="permissions" tal:content="python:view.display_permissions(log.old_value)"></span>
> +                  </metal:logs>
> +                </tal:activity>
> +              </td>
> +              <td>
> +                <tal:activity define="value log/new_value">
> +                  <metal:logs use-macro="template/macros/activity-value">
> +                    <span metal:fill-slot="permissions" tal:content="python:view.display_permissions(log.new_value)"></span>
> +                  </metal:logs>
> +                </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.