[Issue #507] Allow non delegate users to declare interest in a patch #588

andrepapoti · 2024-04-01T13:51:54Z

Description

Add the relation between an user and a patch through the Patch's interested_users attribute.
This relations happens through the model PatchInterest that contains a timestamp for when the interest was declared.
Inside a Patch's State there's now an attribute indicating how long an interest can last before it expires.
To remove expired interests async tasks were added using Celery
Serializers and Forms were update to reflect the changes aswell

I am not certain this solves #507. I can see from the code that there have been changes to model and patch structures to store users interested in the patch, but I don't see anywhere changes to actual user interface. I also run this code using docker-compose on my machine and I don't see any way to show interest in the patch? Am I missing something?

andrepapoti · 2024-11-01T05:04:55Z

@hero24 were you unable to declare interest on a patch when running it on a container?

I missed to add a button for it on the UI, I'll add an extra commit with it

hero24 · 2024-11-01T12:26:02Z

@andrepapoti I will retest this later and let you know if it works through API for me, When I was testing this, I focused on web interface. I think most people use patchwork through web interface (it would actually be an interesting metric to conduct, but that beyond of the scope), but yes I could not find an add interest button through web, or any other way to declare interest through web interface.

hero24 · 2024-11-01T21:34:31Z

As I said earlier I gave this a run and tried looking at it through API. The code crashes, like the other PR, when I access through web /api/patches/XXX/ patchwork crashes. Seems like its user permissions, but it can't crash on api call to patch details, and this works without this PR on standard patchwork code. Attaching screenshot.

victor-accarini

Seems to be working didn't check the cronjob though

victor-accarini · 2025-01-25T01:49:32Z

@stephenfin I took a closer look into this feature and from the issue it seems we should only be able to declare interest in a series and not on specific patches, is this correct? Also how the interest should be handled in the case of another series superseding the current one, the review should be moved to the new series or should just be deleted?

stephenfin

This is looking pretty good. There are, however, three main changes I'd like to see:

Rename the models and fields, per comments.
Ensure attention set entries can be "soft deleted", so that maintainers can tell that one existed in the past
Consider dropping the concept stale review requests from Patchwork itself and instead document how one can do this via the API.

stephenfin · 2025-03-11T09:57:46Z

patchwork/models.py

@@ -729,6 +736,15 @@ class Meta:
        ]


+class PatchReviewIntention(models.Model):


I quite like the "attention set" term since it allows us to capture both a review intention and a review request. Can we use it?

Suggested change

class PatchReviewIntention(models.Model):

class PatchAttentionSet(models.Model):

stephenfin · 2025-03-11T10:01:36Z

patchwork/models.py

+class PatchReviewIntention(models.Model):
+    patch = models.ForeignKey(Patch, on_delete=models.CASCADE)
+    user = models.ForeignKey(User, on_delete=models.CASCADE)
+    last_time_marked_for_review = models.DateTimeField(default=tz_utils.now)


Can we rename this field to simply last_updated. I also wonder if we could/should track removals from this set? If we do, I think we likely only need to track the most recent request.

Suggested change

last_time_marked_for_review = models.DateTimeField(default=tz_utils.now)

last_updated = models.DateTimeField(auto_now=True)

removed = models.BooleanField(default=False)

removed_reason = models.CharField(max_length=50)

stephenfin · 2025-03-11T10:04:37Z

patchwork/models.py

+    last_time_marked_for_review = models.DateTimeField(default=tz_utils.now)
+
+    class Meta:
+        unique_together = [('patch', 'user')]


Assuming the above.

Suggested change

unique_together = [('patch', 'user')]

unique_together = [('patch', 'user', 'removed')]

stephenfin · 2025-03-11T10:05:43Z

patchwork/migrations/0048_patch_has_planned_review_and_more.py

Because the GitHub UI won't let me leave comments on the commit message: please wrap commit messages at <= 72 characters.

stephenfin · 2025-03-11T10:06:28Z

patchwork/migrations/0048_patch_has_planned_review_and_more.py

+class Migration(migrations.Migration):
+    dependencies = [
+        ('patchwork', '0047_add_database_indexes'),
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),


I'm guessing this was auto-generated? I didn't see it pop up in the most recent series. Could you regenerate this?

stephenfin · 2025-03-11T10:26:31Z

patchwork/api/patch.py

@@ -428,7 +428,7 @@ def has_object_permission(self, request, view, obj):
        if 'planning_to_review' in data:
            for review_data in data:
                user_id = review_data['user']
-                if request.user.id == reviewing_user:
+                if request.user.id == user_id:


Can you squash this change back into the earlier commit that added this view.

stephenfin · 2025-03-11T10:32:12Z

patchwork/api/patch.py

@@ -172,6 +203,8 @@ class Meta:
            'hash',
            'submitter',
            'delegate',
+            'planning_to_review',


Suggested change

'planning_to_review',

'attention_set',

stephenfin · 2025-03-11T10:32:21Z

patchwork/api/patch.py

@@ -172,6 +203,8 @@ class Meta:
            'hash',
            'submitter',
            'delegate',
+            'planning_to_review',
+            'has_planned_review',


Suggested change

'has_planned_review',

stephenfin · 2025-03-11T10:33:11Z

patchwork/api/patch.py

+        intereted_user = patchreviewintention_set.pop()['user']
+        instance.planning_to_review.add(intereted_user.id)


s/intereted/interested/

stephenfin · 2025-03-11T10:33:58Z

patchwork/api/patch.py

-            return super(PatchDetailSerializer, self).update(
-                instance, validated_data
-            )
+            return


Is this correct? Could you add a comment explaining why we're returning without calling super()?

Patches that needs the attention of a specific user can now be linked to them by the attention set field. Attention flags by default are soft deleted. Signed-off-by: andrepapoti <[email protected]> Signed-off-by: Victor Accarini <[email protected]>

Users can now add/remove themselves from a patch attention list if they want to sinalize that they will look into it. To add themselves to the attention list they must execute a patch with a list with their IDs in the `attention_set` property. To remove themselves they must send a list with their negative IDs or an empty list. Maintainers can also remove anyone from the attention list by sending a list with negative IDs. Signed-off-by: andrepapoti <[email protected]> Signed-off-by: Victor Accarini <[email protected]>

* List view: show number of users interested in a specific patch * Detail view: show a list of users who are interested in the patch * Allow users to add/remove interest in a patch * Allow managers to remove interested user from a patch Signed-off-by: Victor Accarini <[email protected]>

Signed-off-by: Victor Accarini <[email protected]>

victor-accarini · 2025-03-21T20:13:40Z

@stephenfin I executed some of the request changes but also refactored a little @andrepapoti solution.

There were some inconsistencies in how the application behaved in the API and the web, I updated both to have similar behavior.
Now we can add/remove users in the attention set from the admin interface
Fixed a permission bug allowing non-maintainers to update patches if they had 'attention_set' defined in the payload
Moved the permission check for the update function (since is a specific use-case we don't need to make it more complex that needed right now)
Had to adapt a solution for updating the attention_set + soft_delete

Edit:

I also removed the 'soft-delete' attribute from the unique contraints since I'm always updating the existing ones.

andrepapoti marked this pull request as draft April 5, 2024 04:10

andrepapoti force-pushed the i507 branch from 0f7d391 to 58d7360 Compare April 5, 2024 15:35

stephenfin mentioned this pull request Apr 6, 2024

Allow non-delegates to claim interest in a series #507

Open

andrepapoti marked this pull request as ready for review April 8, 2024 16:13

andrepapoti force-pushed the i507 branch from 58d7360 to 62f2ebd Compare April 15, 2024 19:34

andrepapoti marked this pull request as draft April 15, 2024 19:38

andrepapoti force-pushed the i507 branch 3 times, most recently from 4e1c212 to 2eb1ae1 Compare April 17, 2024 15:42

andrepapoti marked this pull request as ready for review April 17, 2024 15:42

andrepapoti force-pushed the i507 branch from 2eb1ae1 to 1001da9 Compare April 17, 2024 15:43

andrepapoti force-pushed the i507 branch from 1001da9 to a0837c0 Compare December 23, 2024 20:00

victor-accarini approved these changes Dec 26, 2024

View reviewed changes

victor-accarini force-pushed the i507 branch 2 times, most recently from 8fccaac to d9abfce Compare January 27, 2025 20:35

victor-accarini force-pushed the i507 branch from d9abfce to bef562f Compare February 24, 2025 14:53

stephenfin requested changes Mar 11, 2025

View reviewed changes

andrepapoti and others added 7 commits March 21, 2025 17:03

models: add attention set to patches

de6b759

Patches that needs the attention of a specific user can now be linked to them by the attention set field. Attention flags by default are soft deleted. Signed-off-by: andrepapoti <[email protected]> Signed-off-by: Victor Accarini <[email protected]>

admin: add attention set information in patches

f1feb60

Signed-off-by: Victor Accarini <[email protected]>

docs: update schema with patch attention set

789f447

Signed-off-by: Victor Accarini <[email protected]>

tests: validate patch attention_set api behavior

a4603b4

Signed-off-by: Victor Accarini <[email protected]>

tests: update patch view tests

2e3da7b

Signed-off-by: Victor Accarini <[email protected]>

victor-accarini force-pushed the i507 branch from bef562f to 2e3da7b Compare March 21, 2025 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue #507] Allow non delegate users to declare interest in a patch #588

[Issue #507] Allow non delegate users to declare interest in a patch #588

andrepapoti commented Apr 1, 2024 •

edited

Loading

stephenfin commented Apr 6, 2024

hero24 commented Oct 23, 2024

andrepapoti commented Nov 1, 2024

hero24 commented Nov 1, 2024 •

edited

Loading

hero24 commented Nov 1, 2024

victor-accarini left a comment

victor-accarini commented Jan 25, 2025

stephenfin left a comment

stephenfin Mar 11, 2025

stephenfin Mar 11, 2025

stephenfin Mar 11, 2025

stephenfin Mar 11, 2025

stephenfin Mar 11, 2025

stephenfin Mar 11, 2025

stephenfin Mar 11, 2025

stephenfin Mar 11, 2025

stephenfin Mar 11, 2025

stephenfin Mar 11, 2025

victor-accarini commented Mar 21, 2025 •

edited

Loading

		@@ -729,6 +736,15 @@ class Meta:
		]


		class PatchReviewIntention(models.Model):

	class PatchReviewIntention(models.Model):
	class PatchAttentionSet(models.Model):

-    last_time_marked_for_review = models.DateTimeField(default=tz_utils.now)
+    last_updated = models.DateTimeField(auto_now=True)
+    removed = models.BooleanField(default=False)
+    removed_reason = models.CharField(max_length=50)

	unique_together = [('patch', 'user')]
	unique_together = [('patch', 'user', 'removed')]

		intereted_user = patchreviewintention_set.pop()['user']
		instance.planning_to_review.add(intereted_user.id)

[Issue #507] Allow non delegate users to declare interest in a patch #588

Are you sure you want to change the base?

[Issue #507] Allow non delegate users to declare interest in a patch #588

Conversation

andrepapoti commented Apr 1, 2024 • edited Loading

Description

Related

stephenfin commented Apr 6, 2024

hero24 commented Oct 23, 2024

andrepapoti commented Nov 1, 2024

hero24 commented Nov 1, 2024 • edited Loading

hero24 commented Nov 1, 2024

victor-accarini left a comment

Choose a reason for hiding this comment

victor-accarini commented Jan 25, 2025

stephenfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victor-accarini commented Mar 21, 2025 • edited Loading

andrepapoti commented Apr 1, 2024 •

edited

Loading

hero24 commented Nov 1, 2024 •

edited

Loading

victor-accarini commented Mar 21, 2025 •

edited

Loading