Skip to content

Add tags to patches #69

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions media/commitfest/js/change_tag.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// An input validator for the color picker. Points out low-contrast tag color
// choices.
const input = document.getElementById("id_color");
input.addEventListener("input", (event) => {
// Don't do anything if the color code doesn't pass default validity.
input.setCustomValidity("");
if (!input.validity.valid) {
return;
}

// Break the #rrggbb color code into RGB components.
color = parseInt(input.value.substr(1), 16);
red = ((color & 0xFF0000) >> 16) / 255.;
green = ((color & 0x00FF00) >> 8) / 255.;
blue = (color & 0x0000FF) / 255.;

// Compare the contrast ratio against white. All the magic math comes from
// Web Content Accessibility Guidelines (WCAG) 2.2, Technique G18:
//
// https://www.w3.org/WAI/WCAG22/Techniques/general/G18.html
//
function l(val) {
if (val <= 0.04045) {
return val / 12.92;
}
return ((val + 0.055) / 1.055) ** 2.4;
}

lum = 0.2126 * l(red) + 0.7152 * l(green) + 0.0722 * l(blue);
contrast = (1 + 0.05) / (lum + 0.05);

// Complain if we're below WCAG 2.2 recommendations.
if (contrast < 4.5) {
input.setCustomValidity(
"Consider choosing a darker color. "
+ "(Tag text is small and white.)\n\n"
+ "Contrast ratio: " + (Math.trunc(contrast * 10) / 10) + " (< 4.5)"
);

// The admin form uses novalidate, so manually display the browser's
// validity popup. (The user can still ignore it if desired.)
input.reportValidity();
}
});
21 changes: 21 additions & 0 deletions pgcommitfest/commitfest/admin.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
from django.contrib import admin
from django.forms import widgets

from .models import (
CfbotBranch,
CfbotTask,
ColorField,
CommitFest,
Committer,
MailThread,
MailThreadAttachment,
Patch,
PatchHistory,
PatchOnCommitFest,
Tag,
TargetVersion,
Topic,
)
Expand Down Expand Up @@ -38,8 +41,26 @@ class MailThreadAttachmentAdmin(admin.ModelAdmin):
)


class ColorInput(widgets.Input):
"""
A color picker widget.
TODO: this will be natively available in Django 5.2.
"""

input_type = "color"


class TagAdmin(admin.ModelAdmin):
# Customize the Tag form with a color picker and soft validation.
change_form_template = "change_tag_form.html"
formfield_overrides = {
ColorField: {"widget": ColorInput},
}


admin.site.register(Committer, CommitterAdmin)
admin.site.register(CommitFest)
admin.site.register(Tag, TagAdmin)
admin.site.register(Topic)
admin.site.register(Patch, PatchAdmin)
admin.site.register(PatchHistory)
Expand Down
39 changes: 39 additions & 0 deletions pgcommitfest/commitfest/migrations/0011_tag_patch_tags.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# Generated by Django 4.2.19 on 2025-05-30 18:09

from django.db import migrations, models
import pgcommitfest.commitfest.models


class Migration(migrations.Migration):
dependencies = [
("commitfest", "0010_add_failing_since_column"),
]

operations = [
migrations.CreateModel(
name="Tag",
fields=[
(
"id",
models.AutoField(
auto_created=True,
primary_key=True,
serialize=False,
verbose_name="ID",
),
),
("name", models.CharField(max_length=50, unique=True)),
("color", pgcommitfest.commitfest.models.ColorField(max_length=7)),
],
options={
"ordering": ("name",),
},
),
migrations.AddField(
model_name="patch",
name="tags",
field=models.ManyToManyField(
blank=True, related_name="patches", to="commitfest.tag"
),
),
]
26 changes: 26 additions & 0 deletions pgcommitfest/commitfest/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,37 @@ def __str__(self):
return self.version


class ColorField(models.CharField):
"""
A small wrapper around a CharField that can hold a #RRGGBB color code. The
primary reason to have this wrapper class is so that the TagAdmin class can
explicitly key off of it to inject a color picker in the admin interface.
"""

def __init__(self, *args, **kwargs):
kwargs["max_length"] = 7 # for `#RRGGBB` format
super().__init__(*args, **kwargs)


class Tag(models.Model):
Copy link
Collaborator

@JelteF JelteF Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@JelteF JelteF Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""Represents a tag/label on a patch."""

name = models.CharField(max_length=50, unique=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

color = ColorField()

class Meta:
ordering = ("name",)

def __str__(self):
return self.name


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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# One patch can be in multiple commitfests, if it has history
commitfests = models.ManyToManyField(CommitFest, through="PatchOnCommitFest")
Expand Down
7 changes: 7 additions & 0 deletions pgcommitfest/commitfest/templates/change_tag_form.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{% extends 'admin/change_form.html' %}
{% load static %}

{% block admin_change_form_document_ready %}
{{ block.super }}
<script src="{% static 'commitfest/js/change_tag.js' %}" async></script>
{% endblock %}
6 changes: 6 additions & 0 deletions pgcommitfest/commitfest/templates/commitfest.html
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ <h3>{{p.is_open|yesno:"Active patches,Closed patches"}}</h3>
<th><a href="#" style="color:#333333;" onclick="return sortpatches(5);">Patch</a>{%if sortkey == 5%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%elif sortkey == -5%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-up"></i></div>{%endif%}</th>
<th><a href="#" style="color:#333333;" onclick="return sortpatches(4);">ID</a>{%if sortkey == 4%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%elif sortkey == -4%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-up"></i></div>{%endif%}</th>
<th>Status</th>
<th>Tags</th>
<th>Ver</th>
<th><a href="#" style="color:#333333;" onclick="return sortpatches(7);">CI status</a>{%if sortkey == 7%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%elif sortkey == -7%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-up"></i></div>{%endif%}</th>
<th><a href="#" style="color:#333333;" onclick="return sortpatches(6);">Stats</a>{%if sortkey == 6%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-down"></i></div>{%elif sortkey == -6%}<div style="float:right;"><i class="glyphicon glyphicon-arrow-up"></i></div>{%endif%}</th>
Expand All @@ -59,6 +60,11 @@ <h3>{{p.is_open|yesno:"Active patches,Closed patches"}}</h3>
<td><a href="/patch/{{p.id}}/">{{p.name}}</a></td>
<td>{{p.id}}</td>
<td><span class="label label-{{p.status|patchstatuslabel}}">{{p.status|patchstatusstring}}</span></td>
<td style="width: min-content;">
{%for t in p.tag_ids%}
<span class="label" style="background: {{all_tags|tagcolor:t}};">{{all_tags|tagname:t}}</span>
{%endfor%}
</td>
<td>{%if p.targetversion%}<span class="label label-default">{{p.targetversion}}</span>{%endif%}</td>
<td class="cfbot-summary">
{%with p.cfbot_results as cfb%}
Expand Down
8 changes: 8 additions & 0 deletions pgcommitfest/commitfest/templates/patch.html
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@
<th>Topic</th>
<td>{{patch.topic}}</td>
</tr>
<tr>
<th>Tags</th>
<td>
{%for tag in patch.tags.all%}
<span class="label" style="background: {{tag|tagcolor}};">{{tag.name}}</span>
{%endfor%}
</td>
</tr>
<tr>
<th>Created</th>
<td>{{patch.created}}</td>
Expand Down
40 changes: 40 additions & 0 deletions pgcommitfest/commitfest/templatetags/commitfest.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from django.utils.translation import ngettext_lazy

import datetime
import string
from uuid import uuid4

from pgcommitfest.commitfest.models import CommitFest, PatchOnCommitFest
Expand Down Expand Up @@ -41,6 +42,45 @@ def patchstatuslabel(value):
return [v for k, v in PatchOnCommitFest._STATUS_LABELS if k == i][0]


@register.filter(name="tagname")
def tagname(value, arg):
"""
Looks up a tag by ID and returns its name. The filter value is the map of
tags, and the argument is the ID. (Unlike tagcolor, there is no
argument-less variant; just use tag.name directly.)

Example:
tag_map|tagname:tag_id
"""
return value[arg].name


@register.filter(name="tagcolor")
def tagcolor(value, key=None):
"""
Returns the color code of a tag. The filter value is either a single tag, in
which case no argument should be given, or a map of tags with the tag ID as
the argument, as with the tagname filter.

Since color codes are injected into CSS, any nonconforming inputs are
replaced with black here. (Prefer `tag|tagcolor` over `tag.color` in
templates, for this reason.)
"""
if key is not None:
code = value[key].color
else:
code = value.color

if (
len(code) == 7
and code.startswith("#")
and all(c in string.hexdigits for c in code[1:])
):
return code

return "#000000"


@register.filter(is_safe=True)
def label_class(value, arg):
return value.label_tag(attrs={"class": arg})
Expand Down
4 changes: 4 additions & 0 deletions pgcommitfest/commitfest/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
Patch,
PatchHistory,
PatchOnCommitFest,
Tag,
)


Expand Down Expand Up @@ -485,6 +486,7 @@ def patchlist(request, cf, personalized=False):
(SELECT string_agg(first_name || ' ' || last_name || ' (' || username || ')', ', ') FROM auth_user INNER JOIN commitfest_patch_authors cpa ON cpa.user_id=auth_user.id WHERE cpa.patch_id=p.id) AS author_names,
(SELECT string_agg(first_name || ' ' || last_name || ' (' || username || ')', ', ') FROM auth_user INNER JOIN commitfest_patch_reviewers cpr ON cpr.user_id=auth_user.id WHERE cpr.patch_id=p.id) AS reviewer_names,
(SELECT count(1) FROM commitfest_patchoncommitfest pcf WHERE pcf.patch_id=p.id) AS num_cfs,
(SELECT array_agg(tag_id) FROM commitfest_patch_tags t WHERE t.patch_id=p.id) AS tag_ids,

branch.needs_rebase_since,
branch.failing_since,
Expand Down Expand Up @@ -531,6 +533,7 @@ def patchlist(request, cf, personalized=False):
)


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

def commitfest(request, cfid):
# Find ourselves
cf = get_object_or_404(CommitFest, pk=cfid)
Expand Down Expand Up @@ -562,6 +565,7 @@ def commitfest(request, cfid):
"form": form,
"patches": patch_list.patches,
"statussummary": statussummary,
"all_tags": {t.id: t for t in Tag.objects.all()},
"has_filter": patch_list.has_filter,
"title": cf.title,
"grouping": patch_list.sortkey == 0,
Expand Down