Made a mistake with making update_patch_relations modular. Tests are
failing. The following changes should be made to this patch to fix:
- patches = set(related_patches) if related_patches else set()
+ patches = set(related_patches['patches']) if related_patches else set()
if patch.related is not None:
patches = patches.union(patch.related.patches.all())
check_user_maintains_all(user, patches)
# handle deletion
- if not related_patches:
+ if not related_patches['patches']:
On Mon, Aug 23, 2021 at 10:58 AM Raxel Gutierrez <ra...@google.com> wrote:
>
> Move function that handles updates for patch relations for REST API
> calls outside of DRF serialized so that it can be reused by the patch
> relations table through form submission. The function is used in an
> upcoming patch that deals with the manual modification of the patch
> relations of a given patch.
>
> Signed-off-by: Raxel Gutierrez <ra...@google.com>
> ---
> patchwork/api/patch.py | 161 +++++++++++++++++++++--------------------
> 1 file changed, 84 insertions(+), 77 deletions(-)
>
> diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> index a97a8820..ff7640b8 100644
> --- a/patchwork/api/patch.py
> +++ b/patchwork/api/patch.py
> @@ -70,6 +70,87 @@ class PatchConflict(APIException):
> )
>
>
> +def check_user_maintains_all(user, patches):
> + maintains = user.maintainer_projects.all()
> + if any(s.project not in maintains for s in patches):
> + detail = (
> + 'At least one patch is part of a project you are not '
> + 'maintaining.'
> + )
> + raise PermissionDenied(detail=detail)
> + return True
> +
> +
> +def update_patch_relations(user, patch, data):
> + # Validation rules
> + # ----------------
> + #
> + # Permissions: to change a relation:
> + # for all patches in the relation, current and proposed,
> + # the user must be maintainer of the patch's project
> + # Note that this has a ratchet effect: if you add a cross-project
> + # relation, only you or another maintainer across both projects can
> + # modify that relationship in _any way_.
> + #
> + # Break before Make: a patch must be explicitly removed from a
> + # relation before being added to another
> + #
> + # No Read-Modify-Write for deletion:
> + # to delete a patch from a relation, clear _its_ related patch,
> + # don't modify one of the patches that are to remain.
> + #
> + # (As a consequence of those two, operations are additive:
> + # if 1 is in a relation with [1,2,3], then
> + # patching 1 with related=[2,4] gives related=[1,2,3,4])
> +
> + # Permissions:
> + # Must be maintainer of:
> + # - current patch
> + check_user_maintains_all(user, [patch])
> + # - all patches currently in relation
> + # - all patches proposed to be in relation
> + errors = []
> + related_patches = data.pop('related')
> + patches = set(related_patches) if related_patches else set()
> + if patch.related is not None:
> + patches = patches.union(patch.related.patches.all())
> + check_user_maintains_all(user, patches)
> +
> + # handle deletion
> + if not related_patches:
> + # do not allow relations with a single patch
> + if patch.related and patch.related.patches.count() == 2:
> + patch.related.delete()
> + patch.related = None
> + patch.save()
> + return errors
> +
> + # break before make
> + relations = {patch.related for patch in patches if patch.related}
> + if len(relations) > 1:
> + return [PatchConflict()]
> + if relations:
> + relation = relations.pop()
> + else:
> + relation = None
> + if relation and patch.related is not None:
> + if patch.related != relation:
> + return [PatchConflict()]
> +
> + # apply
> + if relation is None:
> + relation = PatchRelation()
> + relation.save()
> +
> + for rp in patches:
> + rp.related = relation
> + rp.save()
> + patch.related = relation
> + patch.save()
> +
> + return errors
> +
> +
> class PatchListSerializer(BaseHyperlinkedModelSerializer):
>
> web_url = SerializerMethodField()
> @@ -184,87 +265,13 @@ class PatchDetailSerializer(PatchListSerializer):
> return super(PatchDetailSerializer, self).update(
> instance, validated_data)
>
> - related = validated_data.pop('related')
> -
> - # Validation rules
> - # ----------------
> - #
> - # Permissions: to change a relation:
> - # for all patches in the relation, current and proposed,
> - # the user must be maintainer of the patch's project
> - # Note that this has a ratchet effect: if you add a cross-project
> - # relation, only you or another maintainer across both projects can
> - # modify that relationship in _any way_.
> - #
> - # Break before Make: a patch must be explicitly removed from a
> - # relation before being added to another
> - #
> - # No Read-Modify-Write for deletion:
> - # to delete a patch from a relation, clear _its_ related patch,
> - # don't modify one of the patches that are to remain.
> - #
> - # (As a consequence of those two, operations are additive:
> - # if 1 is in a relation with [1,2,3], then
> - # patching 1 with related=[2,4] gives related=[1,2,3,4])
> -
> - # Permissions:
> - # Because we're in a serializer, not a view, this is a bit clunky
> user = self.context['request'].user.profile
> - # Must be maintainer of:
> - # - current patch
> - self.check_user_maintains_all(user, [instance])
> - # - all patches currently in relation
> - # - all patches proposed to be in relation
> - patches = set(related['patches']) if related else {}
> - if instance.related is not None:
> - patches = patches.union(instance.related.patches.all())
> - self.check_user_maintains_all(user, patches)
> -
> - # handle deletion
> - if not related['patches']:
> - # do not allow relations with a single patch
> - if instance.related and instance.related.patches.count() == 2:
> - instance.related.delete()
> - instance.related = None
> - return super(PatchDetailSerializer, self).update(
> - instance, validated_data)
> -
> - # break before make
> - relations = {patch.related for patch in patches if patch.related}
> - if len(relations) > 1:
> - raise PatchConflict()
> - if relations:
> - relation = relations.pop()
> - else:
> - relation = None
> - if relation and instance.related is not None:
> - if instance.related != relation:
> - raise PatchConflict()
> -
> - # apply
> - if relation is None:
> - relation = PatchRelation()
> - relation.save()
> - for patch in patches:
> - patch.related = relation
> - patch.save()
> - instance.related = relation
> - instance.save()
> -
> + errors = update_patch_relations(user, instance, validated_data)
> + if errors:
> + raise errors.pop()
> return super(PatchDetailSerializer, self).update(
> instance, validated_data)
>
> - @staticmethod
> - def check_user_maintains_all(user, patches):
> - maintains = user.maintainer_projects.all()
> - if any(s.project not in maintains for s in patches):
> - detail = (
> - 'At least one patch is part of a project you are not '
> - 'maintaining.'
> - )
> - raise PermissionDenied(detail=detail)
> - return True
> -
> class Meta:
> model = Patch
> fields = PatchListSerializer.Meta.fields + (
> --
> 2.33.0.rc2.250.ged5fa647cd-goog
>
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
