-
Notifications
You must be signed in to change notification settings - Fork 12
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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(); | ||
} | ||
}); |
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" | ||
), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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): | ||
"""Represents a tag/label on a patch.""" | ||
|
||
name = models.CharField(max_length=50, unique=True) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||
|
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 %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ | |
Patch, | ||
PatchHistory, | ||
PatchOnCommitFest, | ||
Tag, | ||
) | ||
|
||
|
||
|
@@ -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, | ||
|
@@ -531,6 +533,7 @@ def patchlist(request, cf, personalized=False): | |
) | ||
|
||
|
||
@transaction.atomic # tie the patchlist() query to Tag.objects.all() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Is there some hidden cost to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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) | ||
|
@@ -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, | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.