Review: Approve code
Diff comments: > > === modified file 'lib/lp/app/webservice/marshallers.py' > --- lib/lp/app/webservice/marshallers.py 2012-01-01 02:58:52 +0000 > +++ lib/lp/app/webservice/marshallers.py 2018-09-25 18:05:37 +0000 > @@ -31,3 +44,49 @@ > if (value is not None and getUtility(ILaunchBag).user is None): > return obfuscate_email(value) > return value > + > + > +class InlineObjectFieldMarshaller(SimpleFieldMarshaller): > + """A marshaller that represents an object as a dict. > + > + lazr.restful represents objects as URL references by default, but that > + isn't what we want in all cases. > + > + To use this marshaller to read JSON input data, you must register an > + adapter from the expected top-level type of the loaded JSON data > + (usually `dict`) to the `InlineObject` field's schema. The adapter will > + be called with the deserialised input data, with all inner fields > + already converted as indicated by the schema. > + """ > + > + def unmarshall(self, entry, value): > + """See `IFieldMarshaller`.""" > + result = {} > + for name in self.field.schema.names(all=True): > + field = self.field.schema[name] > + if IField.providedBy(field): > + marshaller = getMultiAdapter( > + (field, self.request), IFieldMarshaller) > + sub_value = getattr(value, name, field.default) > + try: > + sub_entry = getMultiAdapter( > + (sub_value, self.request), IEntry) > + except ComponentLookupError: > + sub_entry = entry > + result[name] = marshaller.unmarshall(sub_entry, sub_value) > + return result > + > + def _marshall_from_json_data(self, value): > + """See `SimpleFieldMarshaller`.""" > + template = {} > + for name in self.field.schema.names(all=True): > + field = self.field.schema[name] > + if IField.providedBy(field): > + if field.required and name not in value: > + raise RequiredMissing(name) > + if name in value: > + marshaller = getMultiAdapter( > + (field, self.request), IFieldMarshaller) > + template[name] = marshaller.marshall_from_json_data( > + value[name]) > + return self.field.schema(template) Should these both use representation_name from the marshaller rather than the normal field name, so they end up as foo_link rather than just foo for object references? > > === modified file 'lib/lp/app/webservice/tests/test_marshallers.py' > --- lib/lp/app/webservice/tests/test_marshallers.py 2012-01-01 02:58:52 > +0000 > +++ lib/lp/app/webservice/tests/test_marshallers.py 2018-09-25 18:05:37 > +0000 > @@ -128,3 +149,56 @@ > webservice = LaunchpadWebServiceCaller() > etag_logged_out = webservice(ws_url(bug)).getheader('etag') > self.assertNotEqual(etag_logged_in, etag_logged_out) > + > + > +class IInlineExample(Interface): > + > + person = PersonChoice(vocabulary="ValidPersonOrTeam") > + > + status = Choice(vocabulary=JobStatus) > + > + > +@implementer(IInlineExample) > +class InlineExample: > + > + def __init__(self, person, status): > + self.person = person > + self.status = status > + > + > +@adapter(dict) > +@implementer(IInlineExample) > +def inline_example_from_dict(template): > + return InlineExample(**template) > + > + > +class TestInlineObjectFieldMarshaller(TestCaseWithFactory): > + > + layer = DatabaseFunctionalLayer > + > + def test_unmarshall(self): > + field = InlineObject(schema=IInlineExample) > + request = WebServiceTestRequest() > + request.setVirtualHostRoot(names=["devel"]) > + marshaller = InlineObjectFieldMarshaller(field, request) > + obj = InlineExample(self.factory.makePerson(), JobStatus.WAITING) > + result = marshaller.unmarshall(None, obj) > + self.assertThat(result, MatchesDict({ > + "person": Equals(canonical_url(obj.person, request=request)), e.g. this seems like it should be person_link. > + "status": Equals("Waiting"), > + })) > + > + def test_marshall_from_json_data(self): > + self.useFixture(ZopeAdapterFixture(inline_example_from_dict)) > + field = InlineObject(schema=IInlineExample) > + request = WebServiceTestRequest() > + request.setVirtualHostRoot(names=["devel"]) > + marshaller = InlineObjectFieldMarshaller(field, request) > + person = self.factory.makePerson() > + data = { > + "person": canonical_url(person, request=request), > + "status": "Running", > + } > + obj = marshaller.marshall_from_json_data(data) > + self.assertThat(obj, MatchesStructure.byEquality( > + person=person, status=JobStatus.RUNNING)) > > === modified file 'lib/lp/code/interfaces/gitref.py' > --- lib/lp/code/interfaces/gitref.py 2018-08-20 23:33:01 +0000 > +++ lib/lp/code/interfaces/gitref.py 2018-09-25 18:05:37 +0000 > @@ -392,6 +389,36 @@ > """ > > > +class IGitRefEdit(Interface): > + """IGitRef methods that require launchpad.Edit permission.""" > + > + @export_read_operation() > + @operation_for_version("devel") > + def getGrants(): > + """Get the access grants for this reference.""" > + > + @operation_parameters( > + grants=List( > + title=_("Grants"), > + # Really IGitNascentRuleGrant, patched in > + # _schema_circular_imports.py. > + value_type=InlineObject(schema=Interface))) > + @call_with(user=REQUEST_USER) > + @export_write_operation() > + @operation_for_version("devel") > + def setGrants(grants, user): > + """Set the access grants for this reference.""" May be worth calling out that other grants may apply via wildcards. > + > + > +class IGitRef(IGitRefView, IGitRefEdit): > + """A reference in a Git repository.""" > + > + # XXX cjwatson 2015-01-19 bug=760849: "beta" is a lie to get WADL > + # generation working. Individual attributes must set their version to > + # "devel". > + export_as_webservice_entry(as_of="beta") > + > + > class IGitRefBatchNavigator(ITableBatchNavigator): > pass > > > === modified file 'lib/lp/code/model/gitref.py' > --- lib/lp/code/model/gitref.py 2018-08-23 12:34:24 +0000 > +++ lib/lp/code/model/gitref.py 2018-09-25 18:05:37 +0000 > @@ -453,7 +475,10 @@ > > @property > def commit_message_first_line(self): > - return self.commit_message.split("\n", 1)[0] > + if self.commit_message is not None: > + return self.commit_message.split("\n", 1)[0] > + else: > + return None Is this meant to be here? > > @property > def has_commits(self): > > === modified file 'lib/lp/code/model/gitrule.py' > --- lib/lp/code/model/gitrule.py 2018-09-25 18:05:37 +0000 > +++ lib/lp/code/model/gitrule.py 2018-09-25 18:05:37 +0000 > @@ -117,6 +132,54 @@ > getUtility(IGitActivitySet).logGrantAdded(grant, grantor) > return grant > > + def _validateGrants(self, grants): > + """Validate a new iterable of access grants.""" > + for grant in grants: > + if grant.grantee_type == GitGranteeType.PERSON: > + if grant.grantee is None: > + raise ValueError( > + "Permission grant for %s has grantee_type 'Person' " > + "but no grantee" % self.ref_pattern) > + else: > + if grant.grantee is not None: > + raise ValueError( > + "Permission grant for %s has grantee_type '%s', " > + "contradicting grantee ~%s" % > + (self.ref_pattern, grant.grantee_type, > + grant.grantee.name)) > + > + def setGrants(self, grants, user): > + """See `IGitRule`.""" > + self._validateGrants(grants) > + existing_grants = { > + (grant.grantee_type, grant.grantee): grant > + for grant in self.grants} > + new_grants = OrderedDict( > + ((grant.grantee_type, grant.grantee), grant) > + for grant in grants) > + > + for grant_key, grant in existing_grants.items(): > + if grant_key not in new_grants: > + grant.destroySelf(user) > + > + for grant_key, new_grant in new_grants.items(): > + grant = existing_grants.get(grant_key) > + if grant is None: > + new_grantee = ( > + new_grant.grantee > + if new_grant.grantee_type == GitGranteeType.PERSON > + else new_grant.grantee_type) > + grant = self.addGrant(new_grantee, user) Should this set the permissions from the start, rather than firing a creation event and then a modification one? > + grant_before_modification = Snapshot( > + grant, providing=providedBy(grant)) > + edited_fields = [] > + for field in ("can_create", "can_push", "can_force_push"): > + if getattr(grant, field) != getattr(new_grant, field): > + setattr(grant, field, getattr(new_grant, field)) > + edited_fields.append(field) > + notify(ObjectModifiedEvent( > + grant, grant_before_modification, edited_fields)) > + > def destroySelf(self, user): > """See `IGitRule`.""" > getUtility(IGitActivitySet).logRuleRemoved(self, user) > @@ -213,8 +276,41 @@ > return "<GitRuleGrant [%s] to %s> for %r" % ( > ", ".join(permissions), grantee_name, self.rule) > > + def toDataForJSON(self, media_type): > + """See `IJSONPublishable`.""" > + if media_type != "application/json": > + raise ValueError("Unhandled media type %s" % media_type) > + request = get_current_browser_request() > + return { > + "grantee_type": self.grantee_type, > + "grantee": ( > + canonical_url(self.grantee, request=request) > + if self.grantee is not None else None), Similar to above, should this be grantee_link to match normal links? > + "can_create": self.can_create, > + "can_push": self.can_push, > + "can_force_push": self.can_force_push, > + } > + Does InlineObjectFieldMarshaller.unmarshall not work here? > def destroySelf(self, user=None): > """See `IGitRuleGrant`.""" > if user is not None: > getUtility(IGitActivitySet).logGrantRemoved(self, user) > Store.of(self).remove(self) > + > + > +@implementer(IGitNascentRuleGrant) > +class GitNascentRuleGrant: > + > + def __init__(self, grantee_type, grantee=None, can_create=False, > + can_push=False, can_force_push=False): > + self.grantee_type = grantee_type > + self.grantee = grantee > + self.can_create = can_create > + self.can_push = can_push > + self.can_force_push = can_force_push > + > + > +@adapter(dict) > +@implementer(IGitNascentRuleGrant) > +def nascent_rule_grant_from_dict(template): > + return GitNascentRuleGrant(**template) -- https://code.launchpad.net/~cjwatson/launchpad/git-permissions-webservice-ref/+merge/355608 Your team Launchpad code reviewers is subscribed to branch lp:launchpad. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp