[Issue #441] Add maintainer notes to patches #586

andrepapoti · 2024-03-19T14:40:18Z

Description

Add support for maintainers to add notes to patches.
Notes can be either public or private enabling non maintainers users of a certain patches to see them or not.
The API for this feature allows to create, read, update, and delete notes. Reading can be either a detailed view of a note or a list of notes for a specified patch.
The patch serializer was update to include the notes related to it

Thought on this more over the weekend. Out of curiosity, how many maintainer notes would one expect and do we need them to be surfaced to the top above comments? Put another way, would we expect there to be a single uber-important note on a patch, or many notes like a conversation? I'm thinking that a note is really no different from a comment. Rather than adding a whole new model, we could simply extend the Comment model and API to allow creating comments via the web UI and REST API and marking a comment as private (and maybe also hidden, for spammy replies). We would need a visual way to distinguish between "real" comments (i.e. those sent via the mailing list) and other replies we should allow users to filter between the two via a dropdown filter that allows you to show only comments from e.g. a certain author, submitted via the web UI etc. but it could flow a bit better and avoid adding yet another resource to our database and API. Again, this is taken from experience: tools like Bugzilla, GitHub Issues, and Red Hat's Support Portal all work this way, with private comments interleaved with regular comments. What do you think?

andrepapoti · 2024-04-08T18:15:47Z

@stephenfin

I don't imagine patches having many notes at once (although it's possible in the current implementation).

I think the idea for it is to provide important context for a patch before the reader dives into the patch so it makes sense to give them more importance than comments, they are similar to a commit message, but can be private and edited without the need to submit a new patch. If they were to be in the middle of a comment thread they wouldn't have the same emphasis on their message.

Like you mentioned, notes also have a different way to interact with them, to maintain the privacy they are added through the Web UI / REST API

For these reasons notes, in their current state, are different enough from comments to have their own model.

I think that if we went with private comments the use case for the feature would be a bit different, but very valid as well

kuba-moo · 2024-04-09T17:51:22Z

That's right, FWIW, the intention is small number of important notes / one note which can be edited.

stephenfin · 2024-07-12T17:13:12Z

patchwork/api/note.py

@@ -0,0 +1,143 @@
+# Patchwork - automated patch tracking system
+# Copyright (C) 2018 Red Hat


Probably want a different copyright here.

stephenfin · 2024-07-12T17:15:09Z

patchwork/migrations/0047_timestampmixin_note.py

+                    models.DateTimeField(default=django.utils.timezone.now),
+                ),
+            ],
+        ),


This shouldn't be here. I'm going to guess you missed setting abstract in the TimestampMixin.Meta class?

Added this setting

stephenfin · 2024-07-12T17:16:32Z

patchwork/models.py

+    updated_at = models.DateTimeField(default=tz_utils.now)
+
+    def update_timestamp(self):
+        self.updated_at = tz_utils.now().isoformat()


You need to add the following to ensure this doesn't translate to a real table:

class Meta: abstract = True

stephenfin · 2024-07-12T17:18:45Z

patchwork/models.py

@@ -33,6 +33,14 @@ def validate_regex_compiles(regex_string):
        raise ValidationError('Invalid regular expression entered!')


+class TimestampMixin(models.Model):
+    created_at = models.DateTimeField(default=tz_utils.now)
+    updated_at = models.DateTimeField(default=tz_utils.now)


I think you want auto_now_add and auto_now for created_at and updated_at, respectively. You should be able to drop update_timestamp and the call to same then.

https://docs.djangoproject.com/en/dev/ref/models/fields/#django.db.models.DateField

Nice catch. Changed to use these options

stephenfin · 2024-07-12T17:20:05Z

patchwork/models.py

+    )
+    submitter = models.ForeignKey(User, on_delete=models.CASCADE)
+    content = models.TextField(null=False, blank=True)
+    maintainer_only = models.BooleanField(default=True)


Are these not always maintainer-only? Why would we ever want to make them public?

Not necessarily, it may be used to transmit additional context for reviewers that may not be adequate to have in the commit message, in this context it would make sense to have public notes.

stephenfin · 2024-07-12T17:21:50Z

patchwork/templates/patchwork/submission.html

+<h2>Notes</h2>
+{% endif %}
+<a name="{{ item.id }}"></a>
+<div class="submission-message">


Can we use a different class and set the background to a different colour so it stands out? These are presumably rather important.

I used the same style that we have for commit messages since they have a similar level of importance. Do you have any suggestions on how to style it differently ?

stephenfin · 2024-07-12T17:22:03Z

patchwork/tests/api/test_notes.py

@@ -0,0 +1,231 @@
+# Patchwork - automated patch tracking system
+# Copyright (C) 2018 Red Hat


Wrong license header?

stephenfin · 2024-07-12T17:24:52Z

patchwork/views/patch.py

@@ -122,6 +122,14 @@ def patch_detail(request, project_id, msgid):
        'submitter', 'date', 'id', 'content', 'patch', 'addressed'
    )

+    if (
+        request.user.is_authenticated
+        and patch.project not in request.user.profile.maintainer_projects.all()


Suggested change

and patch.project not in request.user.profile.maintainer_projects.all()

and patch.project in request.user.profile.maintainer_projects.all()

Assuming I'm reading this correctly, surely the project should be in the user's list of projects they maintain?

Assuming this does need updating, I think we need a test for the UI also.

Oh I think I was logged in the wrong account when testing it on my end, you are correct, it should be in the user list of projects. I fexed the issue, made the notes show for admin users and added tests for the UI

stephenfin · 2024-07-12T17:26:10Z

releasenotes/notes/issue-441-8392549b54b1178d.yaml

+    may not have. These can also be public so that any user can read them.
+api:
+  - |
+    The Application version has been updated to v3.2.


You don't need this: we'll announce the version bump separately when we cut a release.

hero24 · 2024-10-31T17:11:34Z

I have run this and I have couple of points to add:

There is no add note button on web interface directly. you have to go through api/patches/XX/notes/
When Ive added a note using above method the author of note is the same with the author of a patch which is not always the case, in bigger projects there are many people committing to certain parts of the project and only one/two maintainers. This is especially important that the author is correct when there are multiple maintainers.
Similarly, the date I see is also empty and set to UTC
Lastly I dont see a way of removing/editing or marking the note as resolved.
I think there should be edit/remove for this.

andrepapoti · 2024-11-01T04:58:21Z

@hero24 I'll investigate / fix the mentioned points over the next week

Signed-off-by: andrepapoti <[email protected]>

Added NoteList api. It allows the user to fetch all notes from a specific test or create a new one Added NoteDetail api. It allows the user to fetch, update and delete notes Signed-off-by: andrepapoti <[email protected]>

Patch serializer returns a fields containing it's notes. Some notes may be filtered out depending on the request's user and on the note maintainer_only attribute Signed-off-by: andrepapoti <[email protected]>

Bump latest API version to 1.4 Update patchwork.j2 with new note endpoints Add note endpoints to django urls Signed-off-by: andrepapoti <[email protected]>

Signed-off-by: andrepapoti <[email protected]>

The submission template now includes a section to display notes, these can be filtered out depending if the request user is a maintainer for the patch's project and on the note maintainer_only attribute Signed-off-by: andrepapoti <[email protected]>

Signed-off-by: andrepapoti <[email protected]>

victor-accarini · 2024-12-26T13:35:36Z

patchwork/views/patch.py

+                create_note_form = CreateNoteForm()
+
+            if is_maintainer and action == 'edit-note':
+                print(edit_cancel)


victor-accarini · 2024-12-26T13:40:23Z

patchwork/views/patch.py

+            if is_maintainer and action == 'add-note':
+                create_note_form = CreateNoteForm()
+
+            if is_maintainer and action == 'edit-note':


You have to make sure the other actions are not evaluated after the first match, without the elif when trying to add a note as superuser but not maintainer we get a 403 error.

Suggested change

if is_maintainer and action == 'edit-note':

elif is_maintainer and action == 'edit-note':

victor-accarini · 2024-12-26T13:48:22Z

patchwork/forms.py

+    form_name = forms.CharField(initial=name, widget=forms.HiddenInput)
+    content = forms.CharField(label='Content', widget=forms.Textarea)
+    maintainer_only = forms.BooleanField(
+        label='Maintainers Only', initial=True, widget=forms.CheckboxInput


We must set required=False for checkboxes to enable public notes creation

Suggested change

label='Maintainers Only', initial=True, widget=forms.CheckboxInput

label='Maintainers Only', initial=True, widget=forms.CheckboxInput, required=False

victor-accarini · 2024-12-26T13:49:45Z

patchwork/tests/api/test_notes.py

+            instance.submitter.id, response_data['submitter']['id']
+        )
+
+    def test_create_note(self):


Add create public note for tests

andrepapoti changed the title ~~Add maintainer notes to patches (#441)~~ [Issue #441] Add maintainer notes to patches Mar 19, 2024

stephenfin requested changes Apr 6, 2024

View reviewed changes

andrepapoti force-pushed the i441 branch 2 times, most recently from 3c68f5d to 0dedae2 Compare April 8, 2024 19:51

stephenfin requested changes Jul 12, 2024

View reviewed changes

andrepapoti force-pushed the i441 branch from 0dedae2 to b62f796 Compare July 22, 2024 16:55

models: Add Note model

9f5baca

Signed-off-by: andrepapoti <[email protected]>

andrepapoti force-pushed the i441 branch from b62f796 to 3ec381d Compare December 3, 2024 13:46

andrepapoti added 8 commits December 19, 2024 00:12

migrations: add migration for Note

1993bb8

Signed-off-by: andrepapoti <[email protected]>

api: Add Note view and serializer

6e6046a

Added NoteList api. It allows the user to fetch all notes from a specific test or create a new one Added NoteDetail api. It allows the user to fetch, update and delete notes Signed-off-by: andrepapoti <[email protected]>

api: Add Note to Patch Serializer

f59e310

Patch serializer returns a fields containing it's notes. Some notes may be filtered out depending on the request's user and on the note maintainer_only attribute Signed-off-by: andrepapoti <[email protected]>

urls: add endpoints for Notes

34c1295

Bump latest API version to 1.4 Update patchwork.j2 with new note endpoints Add note endpoints to django urls Signed-off-by: andrepapoti <[email protected]>

tests: Add tests for Note related endpoints

4947882

Signed-off-by: andrepapoti <[email protected]>

admin: Add Note admin

0343f4e

Signed-off-by: andrepapoti <[email protected]>

release-notes: Add release notes for the patch note feature

15a756c

Signed-off-by: andrepapoti <[email protected]>

andrepapoti force-pushed the i441 branch from 3ec381d to 15a756c Compare December 19, 2024 03:14

victor-accarini reviewed Dec 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue #441] Add maintainer notes to patches #586

[Issue #441] Add maintainer notes to patches #586

andrepapoti commented Mar 19, 2024

stephenfin left a comment

stephenfin commented Apr 8, 2024

andrepapoti commented Apr 8, 2024 •

edited

Loading

kuba-moo commented Apr 9, 2024 •

edited

Loading

stephenfin Jul 12, 2024

andrepapoti Jul 22, 2024

stephenfin Jul 12, 2024

andrepapoti Jul 22, 2024

stephenfin Jul 12, 2024

stephenfin Jul 12, 2024

andrepapoti Jul 22, 2024

stephenfin Jul 12, 2024

andrepapoti Jul 22, 2024

stephenfin Jul 12, 2024

andrepapoti Jul 22, 2024

stephenfin Jul 12, 2024

andrepapoti Jul 22, 2024

stephenfin Jul 12, 2024

andrepapoti Jul 22, 2024

stephenfin Jul 12, 2024

andrepapoti Jul 22, 2024

hero24 commented Oct 31, 2024 •

edited

Loading

andrepapoti commented Nov 1, 2024

victor-accarini Dec 26, 2024

victor-accarini Dec 26, 2024

victor-accarini Dec 26, 2024

victor-accarini Dec 26, 2024

		@@ -0,0 +1,143 @@
		# Patchwork - automated patch tracking system
		# Copyright (C) 2018 Red Hat

		@@ -0,0 +1,231 @@
		# Patchwork - automated patch tracking system
		# Copyright (C) 2018 Red Hat

	and patch.project not in request.user.profile.maintainer_projects.all()
	and patch.project in request.user.profile.maintainer_projects.all()

	if is_maintainer and action == 'edit-note':
	elif is_maintainer and action == 'edit-note':

	label='Maintainers Only', initial=True, widget=forms.CheckboxInput
	label='Maintainers Only', initial=True, widget=forms.CheckboxInput, required=False

[Issue #441] Add maintainer notes to patches #586

Are you sure you want to change the base?

[Issue #441] Add maintainer notes to patches #586

Conversation

andrepapoti commented Mar 19, 2024

Description

Related

stephenfin left a comment

Choose a reason for hiding this comment

stephenfin commented Apr 8, 2024

andrepapoti commented Apr 8, 2024 • edited Loading

kuba-moo commented Apr 9, 2024 • edited Loading

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

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

hero24 commented Oct 31, 2024 • edited Loading

andrepapoti commented Nov 1, 2024

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

andrepapoti commented Apr 8, 2024 •

edited

Loading

kuba-moo commented Apr 9, 2024 •

edited

Loading

hero24 commented Oct 31, 2024 •

edited

Loading