[Issue #441] Add maintainer notes to patches by andrepapoti · Pull Request #586 · getpatchwork/patchwork · GitHub

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?

@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

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

Probably want a different copyright here.

Fixed

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

Added this setting

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

    class Meta:
        abstract = True

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

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.

Any broader discussion should happen on the mailing list, really.
If we're risking splintering the discussion I'd rather not have this feature at all :S

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 ?

Wrong license header?

Fixed

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

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

Removed

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.

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

Leftover

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':

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

Add create public note for tests

Signed-off-by: andrepapoti <andrepapoti@gmail.com>

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 <andrepapoti@gmail.com>

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 <andrepapoti@gmail.com>

Bump latest API version to 1.4 Update patchwork.j2 with new note endpoints Add note endpoints to django urls Signed-off-by: andrepapoti <andrepapoti@gmail.com>

Signed-off-by: andrepapoti <andrepapoti@gmail.com>

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 <andrepapoti@gmail.com>

Signed-off-by: andrepapoti <andrepapoti@gmail.com>

I've thought about this more, and I'd really like us to re-use the Comment model rather than adding a new Note model. I think this will be much simpler: there's no new models, no new APIs, and no new Admin views. All we need is a private field on the PatchComment and CoverComment models (and ideally a hidden field too, but that can be done separately) and a way to filter by type in the both the REST API and web UI. We'll also need to make some changes to the EmailMixin model (like making msgid nullable) but it's not complicated. Because we avoid the new model, we also avoid any new JOINs, meaning it should also be more performant.

I did a quick mock-up of how I'd expect this to look via the web UI:

@victor-accarini @andrepapoti Would you be able to rework this to suit?

Per last reply.

@stephenfin PR #632 is based on your reply. Since there are quite a few differences I pushed into another branch.

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

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

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

victor-accarini force-pushed the i441 branch from 15a756c to 43c752b Compare January 21, 2025 12:55

andrepapoti added 9 commits January 24, 2025 19:38

models: Add Note model

caf054a

Signed-off-by: andrepapoti <andrepapoti@gmail.com>

migrations: add migration for Note

cb13754

Signed-off-by: andrepapoti <andrepapoti@gmail.com>

api: Add Note view and serializer

a9541d6

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 <andrepapoti@gmail.com>

api: Add Note to Patch Serializer

df9ce0e

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 <andrepapoti@gmail.com>

urls: add endpoints for Notes

13f5fd5

Bump latest API version to 1.4 Update patchwork.j2 with new note endpoints Add note endpoints to django urls Signed-off-by: andrepapoti <andrepapoti@gmail.com>

tests: Add tests for Note related endpoints

a87fd13

Signed-off-by: andrepapoti <andrepapoti@gmail.com>

admin: Add Note admin

cafc714

Signed-off-by: andrepapoti <andrepapoti@gmail.com>

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

ded8739

Signed-off-by: andrepapoti <andrepapoti@gmail.com>

victor-accarini force-pushed the i441 branch from 43c752b to ded8739 Compare January 24, 2025 22:49

stephenfin requested changes Mar 11, 2025

View reviewed changes

stephenfin added enhancement awaiting-feedback labels Mar 11, 2025

victor-accarini mentioned this pull request May 15, 2025

[Issue #441] Add maintainer notes to patches - alternative solution #632

Open

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

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

+                  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.

+                  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

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

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

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

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

		@@ -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()

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

	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

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

[Issue #441] Add maintainer notes to patches by andrepapoti · Pull Request #586 · getpatchwork/patchwork · GitHub

Uh oh!

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

[Issue #441] Add maintainer notes to patches#586andrepapoti wants to merge 9 commits intogetpatchwork:maingetpatchwork/patchwork:mainfrom profusion:i441profusion/patchwork:i441Copy head branch name to clipboard

Conversation

andrepapoti commented Mar 19, 2024

Description

Related

Uh oh!

stephenfin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephenfin commented Apr 8, 2024

Uh oh!

andrepapoti commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kuba-moo commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hero24 commented Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrepapoti commented Nov 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenfin commented Mar 11, 2025

Uh oh!

stephenfin left a comment

Choose a reason for hiding this comment

[Issue #441] Add maintainer notes to patches#586
andrepapoti wants to merge 9 commits into
getpatchwork:mainfrom
profusion:i441

andrepapoti commented Apr 8, 2024 •
edited
Loading

kuba-moo commented Apr 9, 2024 •
edited
Loading

hero24 commented Oct 31, 2024 •
edited
Loading