Re: [PATCH v4 4/9] models: add addressed field

2021-08-20 Thread Stephen Finucane
On Fri, 2021-08-20 at 04:50 +, Raxel Gutierrez wrote: > Currently, there is no state or status associated with comments. In > particular, knowing whether a comment on a patch or cover letter is > addressed or not is useful for transparency and accountability in the > patch review and contributi

Re: [PATCH v4 3/9] templatetags: add utils template filters and tags

2021-08-20 Thread Stephen Finucane
On Fri, 2021-08-20 at 04:50 +, Raxel Gutierrez wrote: > Add utils.py file to create template filters and tags that can be used > by most if not all objects in Patchwork. In particular, add a template > filter to get the plural verbose name of a model and add a template tag > that returns whethe

Re: [PATCH v4 2/9] api: change parameter to for cover comments endpoint

2021-08-20 Thread Stephen Finucane
On Fri, 2021-08-20 at 04:50 +, Raxel Gutierrez wrote: > Rename cover lookup parameter `pk` to `cover_id` for the cover comments > list endpoints to disambiguate from the lookup parameter `comment_id` in > upcoming patches which introduces the cover comments detail endpoint. > This doesn't affec

Re: [PATCH v4 1/9] patch-detail: left align message headers

2021-08-20 Thread Stephen Finucane
On Fri, 2021-08-20 at 04:50 +, Raxel Gutierrez wrote: > Change both of the message containers in the "Commit Message" and > "Comments" to have their content be left-aligned which improves the > proximity of items which boosts the efficiency of gathering information > and performing actions. Bef

Re: [PATCH] REST: don't 500 if we get a non-int ID in /comment/ views

2021-08-20 Thread Stephen Finucane
On Sat, 2021-08-21 at 01:32 +1000, Daniel Axtens wrote: > The Patch and Cover comment views passed unsanitised patch_id/pk > down to the query set filter, which attempts to convert them to > integers, fails, and propagates a ValueError up to us. > > Rather than propagating it to the user as a 500,

Re: [PATCH 1/2] REST: Don't error if a versioned field we would remove is absent

2021-08-20 Thread Stephen Finucane
On Sat, 2021-08-21 at 00:57 +1000, Daniel Axtens wrote: > We remove fields that shouldn't be seen on old versions of the API. > This was done with `pop(field name)`, which will throw an exception > if the named field is absent from the data. However, sometimes if > a patch request is via an old API

[PATCH] urls: Add missing path converters for REST APIs

2021-08-20 Thread Stephen Finucane
Almost all of the API endpoints expect numerical resource IDs, with '/projects' being the sole exception. However, we were not actually enforcing this anywhere. Instead, we were relying on the custom 'get_object_or_404' implementation used by 'GenericAPIView.retrieve' via 'GenericAPIView.get_object

Re: [PATCH 1/2] REST: Don't error if a versioned field we would remove is absent

2021-08-20 Thread Konstantin Ryabitsev
On Sat, Aug 21, 2021 at 12:57:58AM +1000, Daniel Axtens wrote: > This is odd, but not harmful and we definitely shouldn't 500. I can confirm that it fixes the 500 error and restores previous functionality. Thanks very much! Tested-by: Konstantin Ryabitsev -K

Re: 2.2.5 release api-1.1 500 errors

2021-08-20 Thread Daniel Axtens
Hi Konstantin, Thanks for your bug reports. Fixes to both for upstream are on the list. Fixes for the first (v1.1 errors) should apply directly to stable/2.2; the second one will require a minor backport. It's now very late so I'm going to leave this for now. Stephen feel free to take the wheel,

[PATCH] REST: don't 500 if we get a non-int ID in /comment/ views

2021-08-20 Thread Daniel Axtens
The Patch and Cover comment views passed unsanitised patch_id/pk down to the query set filter, which attempts to convert them to integers, fails, and propagates a ValueError up to us. Rather than propagating it to the user as a 500, catch it explicitly and return a 404. Ideally we'd like to return

Re: 2.2.5 release api-1.1 500 errors

2021-08-20 Thread Daniel Axtens
> Some of the others returning 500 errors are: > > GET /api/patches/{msgid}/comments/ > > I'd be happy to provide tracebacks if I can figure out how to get them. yeah msgid is not a valid argument there, it needs to be a number. If you got a traceback it would look like this: File "/opt/py

[PATCH 2/2] tests: test PATCHing a patch in v1.1

2021-08-20 Thread Daniel Axtens
This has been broken for a long time and we didn't notice. Weird. We fixed it, now add a test. Signed-off-by: Daniel Axtens --- patchwork/tests/api/test_patch.py | 14 ++ 1 file changed, 14 insertions(+) diff --git a/patchwork/tests/api/test_patch.py b/patchwork/tests/api/test_patc

[PATCH 1/2] REST: Don't error if a versioned field we would remove is absent

2021-08-20 Thread Daniel Axtens
We remove fields that shouldn't be seen on old versions of the API. This was done with `pop(field name)`, which will throw an exception if the named field is absent from the data. However, sometimes if a patch request is via an old API version, we hit this line without ever having the field present

Re: 2.2.5 release api-1.1 500 errors

2021-08-20 Thread Daniel Axtens
> Here's the PATCH traceback: It's broken upstream too - we don't have a test for PATCH of a patch with APIv1.1. I can't figure out why 2.2.5 would have exposed it but I don't pretend to understand the fine detail of the patch that went in between 2.2.4 and 2.2.5. I'll send a patch for this then

Re: 2.2.5 release api-1.1 500 errors

2021-08-20 Thread Daniel Axtens
Konstantin Ryabitsev writes: > Hello: > > After the 2.2.2->2.2.5 upgrade, we seem to be getting lots of /api/1.1/ errors > for PATCH calls: > > PATCH /api/1.1/patches/{id}/ > > switching to /api/1.2/ seems to fix it, but requires other client app fixes > for API incompatibility. > > Some of t

2.2.5 release api-1.1 500 errors

2021-08-20 Thread Konstantin Ryabitsev
Hello: After the 2.2.2->2.2.5 upgrade, we seem to be getting lots of /api/1.1/ errors for PATCH calls: PATCH /api/1.1/patches/{id}/ switching to /api/1.2/ seems to fix it, but requires other client app fixes for API incompatibility. Some of the others returning 500 errors are: GET /api

[PATCH v4 9/9] docs: add release note for addressed/unaddressed comments

2021-08-20 Thread Raxel Gutierrez
Signed-off-by: Raxel Gutierrez --- ...ressed-patch-comments-bfe71689b6f35a22.yaml | 18 ++ 1 file changed, 18 insertions(+) create mode 100644 releasenotes/notes/comment-detail-endpoint-for-addressed-unaddressed-patch-comments-bfe71689b6f35a22.yaml diff --git a/releasenotes/no

[PATCH v4 8/9] patch-detail: add label and button for comment addressed status

2021-08-20 Thread Raxel Gutierrez
Add new label to patch and cover comments to show the status of whether they are addressed or not and add an adjacent button to allow users to change the status of the comment. Only users that can edit the patch (i.e. patch author, delegate, project maintainers) as well as comment authors can chang

[PATCH v4 7/9] api: add auto-generated OpenAPI schema files

2021-08-20 Thread Raxel Gutierrez
Signed-off-by: Raxel Gutierrez --- docs/api/schemas/latest/patchwork.yaml | 159 +- docs/api/schemas/v1.3/patchwork.yaml | 2770 2 files changed, 2928 insertions(+), 1 deletion(-) create mode 100644 docs/api/schemas/v1.3/patchwork.yaml diff --git a/docs/api/schemas/l

[PATCH v4 3/9] templatetags: add utils template filters and tags

2021-08-20 Thread Raxel Gutierrez
Add utils.py file to create template filters and tags that can be used by most if not all objects in Patchwork. In particular, add a template filter to get the plural verbose name of a model and add a template tag that returns whether an object is editable by the current user. These utilities will

[PATCH v4 5/9] models: change edit permissions for comments

2021-08-20 Thread Raxel Gutierrez
Change patch comments' edit permissions to match that of the patch associated with the comment (i.e. patch author, project maintainers, and delegate) and add permissions for both patch and cover comment authors to be able to change the `addressed` status of comments as well. For cover comments, add

[PATCH v4 1/9] patch-detail: left align message headers

2021-08-20 Thread Raxel Gutierrez
Change both of the message containers in the "Commit Message" and "Comments" to have their content be left-aligned which improves the proximity of items which boosts the efficiency of gathering information and performing actions. Before [1] and after [2] images for reference. [1] https://i.imgur.c

[PATCH v4 0/9] patch-detail: add unaddressed/addressed status to patch comments

2021-08-20 Thread Raxel Gutierrez
This series is a revision to the previous version of the patch series. The series addresses the review comments from the v3 series [1] and also adds support for cover letter comments. Since v3: - Patch 1: separates left align of comment headers - Patch 2: adds renaming of to - Patch 3: adds a u

[PATCH v4 2/9] api: change parameter to for cover comments endpoint

2021-08-20 Thread Raxel Gutierrez
Rename cover lookup parameter `pk` to `cover_id` for the cover comments list endpoints to disambiguate from the lookup parameter `comment_id` in upcoming patches which introduces the cover comments detail endpoint. This doesn't affect the user-facing API. Signed-off-by: Raxel Gutierrez --- patch

[PATCH v4 4/9] models: add addressed field

2021-08-20 Thread Raxel Gutierrez
Currently, there is no state or status associated with comments. In particular, knowing whether a comment on a patch or cover letter is addressed or not is useful for transparency and accountability in the patch review and contribution process. This patch is backend setup for tracking the state of

[PATCH v4 6/9] api: add comments detail endpoint

2021-08-20 Thread Raxel Gutierrez
Add new endpoint for patch and cover comments at api/.../comments/. This involves updating the API version to v1.3 to reflect the new endpoints as a minor change, following the usual semantic versioning convention. The endpoint will make it possible to use the REST API to update the new `addressed