Annotations should probably be part of a commitfest entry

Not really an important thing to address, but the fact that annotations are linked only to a mailthread and not to a commitfest entry causes some weird behaviour. As reported by Robbert Haas on Discord:

https://commitfest.postgresql.org/patch/5501/ says that "Created patch record" happened on 2025-01-11, but it also says that Ibrar Ahmed added an annotation on 2022-07-08. even if Ibrar has a time machine, being able to add an annotation to a patch record that won't be created for another 2.5 years is an impressive achievement. 🤔

This seeming time-traveling, is caused by the fact that this emailthread was part of a commitfest entry before: https://commitfest.postgresql.org/patch/3704/

And the old annotation is still present in the new commitfest entry, because there's no link to the actual cf entry from the annotation, only to the mailthread.

pgcommitfest/pgcommitfest/commitfest/models.py

Lines 417 to 435 in d5ee5a8

    
           class MailThreadAnnotation(models.Model): 
        
               mailthread = models.ForeignKey( 
        
                   MailThread, null=False, blank=False, on_delete=models.CASCADE 
        
               ) 
        
               date = models.DateTimeField(null=False, blank=False, auto_now_add=True) 
        
               user = models.ForeignKey(User, null=False, blank=False, on_delete=models.CASCADE) 
        
               msgid = models.CharField(max_length=1000, null=False, blank=False) 
        
               annotationtext = models.TextField(null=False, blank=False, max_length=2000) 
        
               mailsubject = models.CharField(max_length=500, null=False, blank=False) 
        
               maildate = models.DateTimeField(null=False, blank=False) 
        
               mailauthor = models.CharField(max_length=500, null=False, blank=False) 
        
               @property 
        
               def user_string(self): 
        
                   return "%s %s (%s)" % ( 
        
                       self.user.first_name, 
        
                       self.user.last_name, 
        
                       self.user.username, 
        
                   )

One thing to consider is that maybe we want to encourage people to re-open old commitfest entries instead of creating new ones, to preserve history better generally.

Strongly disagree with associating annotations (or really any patch-related data) with a CF entry. I think that history needs to be associated with CF entries directly (i.e., PatchHistory needs to add a cf_id field) so Events that happen to a patch are recorded as happening when that patch was part of a given CF. But the MailThread is strictly tied to a Patch, not a PatchOnCommitFest. We could prohibit associating the same MailThread with multiple Patches if wanted to do something simple (dealing with the probably few existing violations of the rule in some manner) or just check for a multiple-Patch situation when showing threads and being upfront about the existence of other Patches.

In short, the situation described here is perfectly fine and its just a data presentation failure we need to fix. Or prevent the unusual situation from happening in the first place (i..e, make Patch.mailthread_set ManyToOne - or vice-versa...) since it doesn't seem that important a use case to accommodate (going back through years of history manually instead of expecting the author of the resurrected patch to summarize things nicely in a new thread seems like the wrong way of making this work).

Could someone run a query checking for how many violations of "MailThread does not belong to multiple Patch" we have?

Navigation Menu

Repository navigation

Annotations should probably be part of a commitfest entry #40

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	class MailThreadAnnotation(models.Model):
	mailthread = models.ForeignKey(
	MailThread, null=False, blank=False, on_delete=models.CASCADE
	)
	date = models.DateTimeField(null=False, blank=False, auto_now_add=True)
	user = models.ForeignKey(User, null=False, blank=False, on_delete=models.CASCADE)
	msgid = models.CharField(max_length=1000, null=False, blank=False)
	annotationtext = models.TextField(null=False, blank=False, max_length=2000)
	mailsubject = models.CharField(max_length=500, null=False, blank=False)
	maildate = models.DateTimeField(null=False, blank=False)
	mailauthor = models.CharField(max_length=500, null=False, blank=False)

	@property
	def user_string(self):
	return "%s %s (%s)" % (
	self.user.first_name,
	self.user.last_name,
	self.user.username,
	)

Annotations should probably be part of a commitfest entry #40

Description

Activity

polobo commented on Apr 6, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions