Add tags to patches #69

jchampio · 2025-06-02T18:06:10Z

A Tag is an arbitrary label for a patch in the Commitfest UI. Other than helping users identify patches of interest, it has no other semantic meaning to the CF application. This addresses #11 and #67.

Tags are created using the administrator interface. They consist of a unique name and a background color. The color should be sent from compliant browsers in #rrggbb format, which is stored without backend validation; to avoid later CSS injection, any non-conforming values are replaced with black during rendering.

The second patch helps admins pick colors that aren't completely eye-gouging, by putting up an (ignorable) warning if we don't meet baseline WCAG recommendations on text contrast.

Notes

I've chosen not to put tags into the dashboard, to keep it minimal, but it's easy enough to add if there is disagreement.
To avoid putting a separate name+color copy of every tag into each row of the patchlist query, I instead grab the tag IDs and look them up in a map at render time. I think this requires @transaction.atomic to tie the map and patches together, which I've added across the entire view for simplicity.
Backend validation for the color didn't make a whole lot of sense to me, since only admins can create tags and we escape at time-of-use anyway.
I couldn't find a way to push the background color into a safer data-* attribute, but maybe someone knows of a way to do that? That would get rid of the CSS injection problem altogether.
Eventually I think it would be good to have a selectize-style tag picker, rather than the current multiselect UX.

TODOs

does Django have a better CSS escape function?
check usability of the color picker and soft validation API

Screenshots

Admin interface:

User interface:

A Tag is an arbitrary label for a patch in the Commitfest UI. Other than helping users identify patches of interest, it has no other semantic meaning to the CF application. Tags are created using the administrator interface. They consist of a unique name and a background color. The color should be sent from compliant browsers in #rrggbb format, which is stored without backend validation; to avoid CSS injection, any non-conforming values are replaced with black during templating.

Add front-end soft validation to the TagAdmin form. This uses WCAG 2.2 guidelines to warn the administrator if a color choice is going to be low-contrast when compared to our text/background color of white. (The warning may be ignored.)

polobo · 2025-06-05T02:30:59Z

pgcommitfest/commitfest/views.py

@@ -531,6 +533,7 @@ def patchlist(request, cf, personalized=False):
    )


+@transaction.atomic  # tie the patchlist() query to Tag.objects.all()


I'd be inclined to define Tag as an append/update-only table - soft deletes only. Think PostgreSQL enums in recommended usage (but with a soft delete capability). No real point enforcing a transaction to read its contents in that case. Even if we got an overly fresh read the attributes of Tag that would be valid data.

Until tag usage settles, I think it should be easy for CFMs to add and remove them. (Easily worth a transaction wrap IMO, and the more I look at the complexity of patchlist() the more I think that perhaps it should have been wrapped already...)

Is there some hidden cost to @transaction.atomic that's not in the documentation, that I should be aware of?

Is there some hidden cost to @transaction.atomic that's not in the documentation, that I should be aware of?

Not that I know of. The goal of using soft deletes still stands. Don't show "hidden/deleted" tags in the UI but they are still there and can be re-enabled. With name/description separated from label so long as the purpose is deemed valid the label can be changed to better match community expectations - which I suspect would happen if we removed and then reinstated a tag. It doesn't have to a hard-and-fast rule; especially if we don't functionally depend on it.

polobo · 2025-06-05T02:42:20Z

pgcommitfest/commitfest/models.py

+class Tag(models.Model):
+    """Represents a tag/label on a patch."""
+
+    name = models.CharField(max_length=50, unique=True)


I suppose it might be considered pre-mature but I'd have a separate "label" attribute from the "Name" of the tag (and/or a description) - label being displayed and optimized for length, name and description being documentation. I'd also just add a "tag_category" right up front. I'd tie the colors to the categories and then distinguish within them by name. Given the expanse of the "Product Module" category maybe we should consider a two-color scheme: background for the category and border (and label) to denote the specific element within the category. I'd strongly advise we decide on the initial set of tags before getting to far along in design and development though (lets figure out where to collaborate on that). For categories, at minimum Front-End and Back-End come to mind, probably Protocol. That doesn't include stuff like "Good First Issue" which might fall into a "Meta" category. I'm envisioning stuff like a "CirrusCI" category that flags for stuff like "wants clobber cache enabled" (this is less well formed in my mind but stemmed from the recent discussion on Discord).

Right now it's difficult to get people to agree on what Tags should even be used for, so I'd like to keep it super simple at first. We can add features as requested.

polobo · 2025-06-05T02:48:45Z

pgcommitfest/commitfest/models.py

 class Patch(models.Model, DiffableModel):
    name = models.CharField(
        max_length=500, blank=False, null=False, verbose_name="Description"
    )
    topic = models.ForeignKey(Topic, blank=False, null=False, on_delete=models.CASCADE)
+    tags = models.ManyToManyField(Tag, related_name="patches", blank=True)


Maybe also pre-mature, and likely reasonably added later, but I envisioned an ability for annotations to be added to the tag-patch mapping. Maybe the commit message and email threads are enough places to write stuff but, especially for something like 'Good First Issue', some commentary as to why would be expected to exist somewhere, and a tag annotation seems like a good place.

I'm also thinking we'll use the mailing list for primary discussion while this shakes out, but I think this would be a good thing to add eventually.

polobo · 2025-06-05T03:41:58Z

For the CSS I've a couple of thoughts.

Can we use "LCH" as the color specifier instead of RGB?

Python has cssutils to dynamically build stylesheets. Not sure we can fully delegate value validation but keeping the legibility check in place is nice which naturally checks the inputs.

I'd be more inclined to add data-* attributes to the html markup and build a dynamic stylesheet served from an assets URL. Solving cache busting/invalidation (version code in the virtual file name) should be straight-forward - tags aren't going to be frequently changing. I'm considering whether they should just be compiled in; do away with the creation UI altogether. Point users to an external color picker and just tell them where to copy the value. Need to do more research here though - just some thoughts for now.

jchampio · 2025-06-05T13:26:30Z

Can we use "LCH" as the color specifier instead of RGB?

I like the technical details of LCH, but does it give users something? It's hard to beat "click the button, pick a color, move on" for the price of an <input>. If the system eventually gets the smarts to pick colors for you, then I think LCH starts to make more sense.

polobo · 2025-06-05T16:45:37Z

Can we use "LCH" as the color specifier instead of RGB?

I like the technical details of LCH, but does it give users something? It's hard to beat "click the button, pick a color, move on" for the price of an <input>. If the system eventually gets the smarts to pick colors for you, then I think LCH starts to make more sense.

I would say the people using this form are designers - our users are the people seeing the results on the webpage. It should be generally easier to produce a consistent set of colors for them to see if those colors are specified in LCH, in particular with a agreed-upon "Luminosity component. Also hoping to figure out dark mode at some point which seems easier to do if using LCH. People invented it and got in into the browsers for a reason.

jchampio · 2025-06-05T17:38:30Z

Is it hard to migrate RGB to LCH later, if we eventually get to a place where we'd like to make better use of color?

polobo · 2025-06-05T18:22:54Z

I don’t have any immediate concerns about trying to migrate away from RGB in the future. This is stuff I would need to experiment with myself anyway.

JelteF · 2025-06-05T22:54:02Z

I think storing RGB is fine for now. One thing that would be nice is a "randomize color button" like github has for labels.

JelteF · 2025-06-05T22:57:31Z

pgcommitfest/commitfest/models.py

+        super().__init__(*args, **kwargs)
+
+
+class Tag(models.Model):


Can you add some entries for this to the dummy database dump: https://github.com/postgres/pgcommitfest#regenerating-the-database-dump-files Or maybe even add a few to that we know we definitely want to the migration.

Sure, I'll add some to the dummy data. (I'm hoping the initial tags will come from the mailing list during the preview.)

I think "bug", "needs-user-testing", "needs-bikeshedding", "good-first-review", "mising-docs", "missing-tests" would be a good start.

jchampio added 2 commits June 2, 2025 10:48

Help admins choose tag colors with contrast

02aa97f

Add front-end soft validation to the TagAdmin form. This uses WCAG 2.2 guidelines to warn the administrator if a color choice is going to be low-contrast when compared to our text/background color of white. (The warning may be ignored.)

polobo reviewed Jun 5, 2025

View reviewed changes

JelteF reviewed Jun 5, 2025

View reviewed changes

		@@ -531,6 +533,7 @@ def patchlist(request, cf, personalized=False):
		)


		@transaction.atomic # tie the patchlist() query to Tag.objects.all()

Add tags to patches #69

Are you sure you want to change the base?

Add tags to patches #69

Uh oh!

Conversation

jchampio commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes

TODOs

Screenshots

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!

polobo commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jchampio commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polobo commented Jun 5, 2025

Uh oh!

jchampio commented Jun 5, 2025

Uh oh!

polobo commented Jun 5, 2025

Uh oh!

JelteF commented Jun 5, 2025

Uh oh!

JelteF Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JelteF Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jchampio commented Jun 2, 2025 •

edited

Loading

polobo commented Jun 5, 2025 •

edited

Loading

jchampio commented Jun 5, 2025 •

edited

Loading

JelteF Jun 5, 2025 •

edited

Loading

JelteF Jun 6, 2025 •

edited

Loading