launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #22898
[Merge] lp:~cjwatson/launchpad/git-permissions-notifications into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/git-permissions-notifications into lp:launchpad with lp:~cjwatson/launchpad/git-activity-model as a prerequisite.
Commit message:
Send email notifications for GitRule and GitGrant changes.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1517559 in Launchpad itself: "git fine-grained permissions"
https://bugs.launchpad.net/launchpad/+bug/1517559
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-permissions-notifications/+merge/354500
We'll have to be careful to send the right ObjectModifiedEvents in the webservice API and the web UI (slightly fiddly as you have to send them both for any rules/grants and for the repository), but that should be manageable in later branches.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-permissions-notifications into lp:launchpad.
=== modified file 'lib/lp/code/adapters/gitrepository.py'
--- lib/lp/code/adapters/gitrepository.py 2015-07-08 16:05:11 +0000
+++ lib/lp/code/adapters/gitrepository.py 2018-09-07 16:10:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Components related to Git repositories."""
@@ -27,12 +27,14 @@
interface = IGitRepository
- def __init__(self, repository, user, name=None, git_identity=None):
+ def __init__(self, repository, user, name=None, git_identity=None,
+ activities=None):
self.repository = repository
self.user = user
self.name = name
self.git_identity = git_identity
+ self.activities = activities
@classmethod
def construct(klass, old_repository, new_repository, user):
@@ -43,10 +45,13 @@
"""
delta = ObjectDelta(old_repository, new_repository)
delta.recordNewAndOld(klass.delta_values)
- if delta.changes:
+ activities = new_repository.getActivity(
+ changed_after=old_repository.date_last_modified)
+ if delta.changes or activities:
changes = delta.changes
changes["repository"] = new_repository
changes["user"] = user
+ changes["activities"] = activities
return GitRepositoryDelta(**changes)
else:
=== modified file 'lib/lp/code/interfaces/gitactivity.py'
--- lib/lp/code/interfaces/gitactivity.py 2018-09-07 16:10:28 +0000
+++ lib/lp/code/interfaces/gitactivity.py 2018-09-07 16:10:29 +0000
@@ -12,7 +12,10 @@
]
from lazr.restful.fields import Reference
-from zope.interface import Interface
+from zope.interface import (
+ Attribute,
+ Interface,
+ )
from zope.schema import (
Choice,
Datetime,
@@ -55,6 +58,9 @@
vocabulary="ValidPersonOrTeam",
description=_("The person or team that this change was applied to."))
+ changee_description = Attribute(
+ "A human-readable description of the changee.")
+
what_changed = Choice(
title=_("What changed"), required=True, readonly=True,
vocabulary=GitActivityType,
=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py 2018-09-07 16:10:28 +0000
+++ lib/lp/code/interfaces/gitrepository.py 2018-09-07 16:10:29 +0000
@@ -622,9 +622,11 @@
:return: The diff as a binary string.
"""
- def getActivity():
+ def getActivity(changed_after=None):
"""Get activity log entries for this repository.
+ :param changed_after: If supplied, only return entries for changes
+ made after this date.
:return: A `ResultSet` of `IGitActivity`.
"""
=== modified file 'lib/lp/code/model/gitactivity.py'
--- lib/lp/code/model/gitactivity.py 2018-09-07 16:10:28 +0000
+++ lib/lp/code/model/gitactivity.py 2018-09-07 16:10:29 +0000
@@ -72,6 +72,17 @@
self.old_value = old_value
self.new_value = new_value
+ @property
+ def changee_description(self):
+ if self.changee is not None:
+ return "~" + self.changee.name
+ elif self.new_value is not None and "changee_type" in self.new_value:
+ return self.new_value["changee_type"].lower()
+ elif self.old_value is not None and "changee_type" in self.old_value:
+ return self.old_value["changee_type"].lower()
+ else:
+ return None
+
def _make_rule_value(rule):
return {"ref_pattern": rule.ref_pattern}
=== modified file 'lib/lp/code/model/gitjob.py'
--- lib/lp/code/model/gitjob.py 2015-10-01 14:45:57 +0000
+++ lib/lp/code/model/gitjob.py 2018-09-07 16:10:29 +0000
@@ -1,9 +1,10 @@
-# Copyright 2015 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
__all__ = [
+ 'describe_repository_delta',
'GitJob',
'GitJobType',
'GitRefScanJob',
@@ -32,6 +33,7 @@
from zope.security.proxy import removeSecurityProxy
from lp.app.errors import NotFoundError
+from lp.code.enums import GitActivityType
from lp.code.interfaces.githosting import IGitHostingClient
from lp.code.interfaces.gitjob import (
IGitJob,
@@ -57,6 +59,7 @@
)
from lp.services.database.stormbase import StormBase
from lp.services.features import getFeatureFlag
+from lp.services.helpers import english_list
from lp.services.job.model.job import (
EnumeratedSubclass,
Job,
@@ -298,6 +301,51 @@
getUtility(IGitHostingClient).delete(self.repository_path, logger=log)
+activity_descriptions = {
+ GitActivityType.RULE_ADDED: "Added protected ref: {new[ref_pattern]}",
+ GitActivityType.RULE_CHANGED: (
+ "Changed protected ref: {old[ref_pattern]} => {new[ref_pattern]}"),
+ GitActivityType.RULE_REMOVED: "Removed protected ref: {old[ref_pattern]}",
+ GitActivityType.GRANT_ADDED: (
+ "Added access for {changee} to {new[ref_pattern]}: {new_grants}"),
+ GitActivityType.GRANT_CHANGED: (
+ "Changed access for {changee} to {new[ref_pattern]}: "
+ "{old_grants} => {new_grants}"),
+ GitActivityType.GRANT_REMOVED: (
+ "Removed access for {changee} to {old[ref_pattern]}: {old_grants}"),
+ }
+
+
+def describe_grants(activity_value):
+ output = []
+ if activity_value is not None:
+ if activity_value.get("can_create"):
+ output.append("create")
+ if activity_value.get("can_push"):
+ output.append("push")
+ if activity_value.get("can_force_push"):
+ output.append("force-push")
+ return english_list(output)
+
+
+def describe_repository_delta(repository_delta):
+ output = text_delta(
+ repository_delta, repository_delta.delta_values,
+ repository_delta.new_values, repository_delta.interface).split("\n")
+ if output and not output[-1]: # text_delta returned empty string
+ output.pop()
+ indent = " " * 4
+ for activity in repository_delta.activities:
+ if activity.what_changed in activity_descriptions:
+ description = activity_descriptions[activity.what_changed].format(
+ old=activity.old_value, new=activity.new_value,
+ changee=activity.changee_description,
+ old_grants=describe_grants(activity.old_value),
+ new_grants=describe_grants(activity.new_value))
+ output.append(indent + description)
+ return "\n".join(output)
+
+
@implementer(IGitRepositoryModifiedMailJob)
@provider(IGitRepositoryModifiedMailJobSource)
class GitRepositoryModifiedMailJob(GitJobDerived):
@@ -312,9 +360,7 @@
"""See `IGitRepositoryModifiedMailJobSource`."""
metadata = {
"user": user.id,
- "repository_delta": text_delta(
- repository_delta, repository_delta.delta_values,
- repository_delta.new_values, repository_delta.interface),
+ "repository_delta": describe_repository_delta(repository_delta),
}
git_job = GitJob(repository, cls.class_job_type, metadata)
job = cls(git_job)
=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py 2018-09-07 16:10:28 +0000
+++ lib/lp/code/model/gitrepository.py 2018-09-07 16:10:29 +0000
@@ -238,7 +238,7 @@
"""
if event.edited_fields:
repository.date_last_modified = UTC_NOW
- send_git_repository_modified_notifications(repository, event)
+ send_git_repository_modified_notifications(repository, event)
@implementer(IGitRepository, IHasOwner, IPrivacy, IInformationType)
@@ -1165,9 +1165,11 @@
"""See `IGitRepository`."""
return Store.of(self).find(GitGrant, GitGrant.repository_id == self.id)
- def getActivity(self):
+ def getActivity(self, changed_after=None):
"""See `IGitRepository`."""
clauses = [GitActivity.repository_id == self.id]
+ if changed_after is not None:
+ clauses.append(GitActivity.date_changed > changed_after)
return Store.of(self).find(GitActivity, *clauses).order_by(
Desc(GitActivity.date_changed), Desc(GitActivity.id))
=== modified file 'lib/lp/code/model/tests/test_gitjob.py'
--- lib/lp/code/model/tests/test_gitjob.py 2017-10-04 01:29:35 +0000
+++ lib/lp/code/model/tests/test_gitjob.py 2018-09-07 16:10:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2017 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for `GitJob`s."""
@@ -13,6 +13,8 @@
)
import hashlib
+from lazr.lifecycle.event import ObjectModifiedEvent
+from lazr.lifecycle.snapshot import Snapshot
import pytz
from testtools.matchers import (
ContainsDict,
@@ -21,9 +23,16 @@
MatchesSetwise,
MatchesStructure,
)
+import transaction
+from zope.event import notify
+from zope.interface import providedBy
from zope.security.proxy import removeSecurityProxy
-from lp.code.enums import GitObjectType
+from lp.code.adapters.gitrepository import GitRepositoryDelta
+from lp.code.enums import (
+ GitGranteeType,
+ GitObjectType,
+ )
from lp.code.interfaces.branchmergeproposal import (
BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG,
)
@@ -33,6 +42,7 @@
IReclaimGitRepositorySpaceJob,
)
from lp.code.model.gitjob import (
+ describe_repository_delta,
GitJob,
GitJobDerived,
GitJobType,
@@ -46,6 +56,7 @@
from lp.services.job.runner import JobRunner
from lp.services.webapp import canonical_url
from lp.testing import (
+ person_logged_in,
TestCaseWithFactory,
time_counter,
)
@@ -338,5 +349,110 @@
self.assertEqual([(path,)], hosting_fixture.delete.extract_args())
+class TestDescribeRepositoryDelta(TestCaseWithFactory):
+ """Tests for `describe_repository_delta`."""
+
+ layer = ZopelessDatabaseLayer
+
+ def assertDeltaDescriptionEqual(self, expected, snapshot, repository):
+ repository_delta = GitRepositoryDelta.construct(
+ snapshot, repository, repository.owner)
+ self.assertEqual(
+ "\n".join(" %s" % line for line in expected),
+ describe_repository_delta(repository_delta))
+
+ def test_change_basic_properties(self):
+ repository = self.factory.makeGitRepository(name="foo")
+ transaction.commit()
+ snapshot = Snapshot(repository, providing=providedBy(repository))
+ with person_logged_in(repository.owner):
+ repository.setName("bar", repository.owner)
+ expected = [
+ "Name: foo => bar",
+ "Git identity: lp:~{person}/{project}/+git/foo => "
+ "lp:~{person}/{project}/+git/bar".format(
+ person=repository.owner.name, project=repository.target.name),
+ ]
+ self.assertDeltaDescriptionEqual(expected, snapshot, repository)
+
+ def test_add_rule(self):
+ repository = self.factory.makeGitRepository()
+ transaction.commit()
+ snapshot = Snapshot(repository, providing=providedBy(repository))
+ with person_logged_in(repository.owner):
+ repository.addRule("refs/heads/*", repository.owner)
+ self.assertDeltaDescriptionEqual(
+ ["Added protected ref: refs/heads/*"], snapshot, repository)
+
+ def test_change_rule(self):
+ repository = self.factory.makeGitRepository()
+ rule = self.factory.makeGitRule(
+ repository=repository, ref_pattern="refs/heads/foo")
+ transaction.commit()
+ snapshot = Snapshot(repository, providing=providedBy(repository))
+ rule_snapshot = Snapshot(rule, providing=providedBy(rule))
+ with person_logged_in(repository.owner):
+ rule.ref_pattern = "refs/heads/bar"
+ notify(ObjectModifiedEvent(rule, rule_snapshot, ["ref_pattern"]))
+ self.assertDeltaDescriptionEqual(
+ ["Changed protected ref: refs/heads/foo => refs/heads/bar"],
+ snapshot, repository)
+
+ def test_remove_rule(self):
+ repository = self.factory.makeGitRepository()
+ rule = self.factory.makeGitRule(
+ repository=repository, ref_pattern="refs/heads/*")
+ transaction.commit()
+ snapshot = Snapshot(repository, providing=providedBy(repository))
+ with person_logged_in(repository.owner):
+ rule.destroySelf(repository.owner)
+ self.assertDeltaDescriptionEqual(
+ ["Removed protected ref: refs/heads/*"], snapshot, repository)
+
+ def test_add_grant(self):
+ repository = self.factory.makeGitRepository()
+ rule = self.factory.makeGitRule(
+ repository=repository, ref_pattern="refs/heads/*")
+ transaction.commit()
+ snapshot = Snapshot(repository, providing=providedBy(repository))
+ with person_logged_in(repository.owner):
+ rule.addGrant(
+ GitGranteeType.REPOSITORY_OWNER, repository.owner,
+ can_create=True, can_push=True, can_force_push=True)
+ self.assertDeltaDescriptionEqual(
+ ["Added access for repository owner to refs/heads/*: "
+ "create, push, and force-push"],
+ snapshot, repository)
+
+ def test_change_grant(self):
+ repository = self.factory.makeGitRepository()
+ grant = self.factory.makeGitGrant(
+ repository=repository, ref_pattern="refs/heads/*",
+ can_create=True)
+ transaction.commit()
+ snapshot = Snapshot(repository, providing=providedBy(repository))
+ grant_snapshot = Snapshot(grant, providing=providedBy(grant))
+ with person_logged_in(repository.owner):
+ grant.can_push = True
+ notify(ObjectModifiedEvent(grant, grant_snapshot, ["can_push"]))
+ self.assertDeltaDescriptionEqual(
+ ["Changed access for ~{grantee} to refs/heads/*: "
+ "create => create and push".format(grantee=grant.grantee.name)],
+ snapshot, repository)
+
+ def test_remove_grant(self):
+ repository = self.factory.makeGitRepository()
+ grant = self.factory.makeGitGrant(
+ repository=repository, ref_pattern="refs/heads/*",
+ grantee=GitGranteeType.REPOSITORY_OWNER, can_push=True)
+ transaction.commit()
+ snapshot = Snapshot(repository, providing=providedBy(repository))
+ with person_logged_in(repository.owner):
+ grant.destroySelf(repository.owner)
+ self.assertDeltaDescriptionEqual(
+ ["Removed access for repository owner to refs/heads/*: push"],
+ snapshot, repository)
+
+
# XXX cjwatson 2015-03-12: We should test that the jobs work via Celery too,
# but that isn't feasible until we have a proper turnip fixture.
=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py 2018-09-07 16:10:28 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py 2018-09-07 16:10:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2017 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for Git repositories."""
@@ -838,6 +838,7 @@
# Attribute modifications send mail to subscribers.
self.assertEqual(0, len(stub.test_emails))
repository = self.factory.makeGitRepository(name="foo")
+ transaction.commit()
repository_before_modification = Snapshot(
repository, providing=providedBy(repository))
with person_logged_in(repository.owner):
@@ -848,6 +849,7 @@
CodeReviewNotificationLevel.NOEMAIL,
repository.owner)
repository.setName("bar", repository.owner)
+ repository.addRule("refs/heads/stable/*", repository.owner)
notify(ObjectModifiedEvent(
repository, repository_before_modification, ["name"],
user=repository.owner))
@@ -857,12 +859,13 @@
self.assertEqual(1, len(stub.test_emails))
message = email.message_from_string(stub.test_emails[0][2])
body = message.get_payload(decode=True)
- self.assertIn("Name: foo => bar\n", body)
- self.assertIn(
- "Git identity: lp:~{person}/{project}/+git/foo => "
- "lp:~{person}/{project}/+git/bar\n".format(
+ self.assertEqual(
+ " Name: foo => bar\n"
+ " Git identity: lp:~{person}/{project}/+git/foo => "
+ "lp:~{person}/{project}/+git/bar\n"
+ " Added protected ref: refs/heads/stable/*".format(
person=repository.owner.name, project=repository.target.name),
- body)
+ body.split("\n\n--\n")[0])
# XXX cjwatson 2015-02-04: This will need to be expanded once Launchpad
# actually notices any interesting kind of repository modifications.
Follow ups
