#3296 comments for grant/revoke permissions
Opened by tkopecek. Modified
tkopecek/koji issue3295  into  master

Download 3296.patch

Related: https://pagure.io/koji/issue/3295

My first thought is that this is a proverbial "can of worms". This patch only adds comments for two operations, but hints at comments for all events.

I'm not sure event_labels is the right thing. We've never used it, and I don't think we need to be constrained by it. This is a significant feature to add and it's worth getting right. In particular I'm not sure about:

  • the 255 character limit
  • the multiple labels per event
  • the term "label"

I have a couple notes about the particulars here, but frankly they might not matter if we need to take a different approach.

  • print_histline seems to handle create_event_label but not revoke_event_label
  • how this affects event caching in get_event (what if get_event is called with different labels in the same transaction)
  • what if we pass both an event_id and a label to make_create/make_revoke?

I think event labels were envisioned to be more like short names for events (analogous to tags in source control systems), hence the limited character length. The term comment suggests a bit more room for freeform text. Note that labels are unique in the db. Admins would not be able to make the same comment twice.

    label VARCHAR(255) UNIQUE NOT NULL

Do we have a sense of what limits we might want, if any, on the length of a comment? Perhaps it might work like a git commit message where the first line is considered a subject for short display purposes, but long messages are allowed.

I was wondering also if this is the right approach. Do we want (in future) really comments to all events? In such case it would be nice to have generic API (addEventComment/editEventComment/deleteEventComment, getEvent(comment=True)) with some permissions - e.g. add can anybody, edit/delete only owner/admin? Separate calls would solve those issues with reusing same event_id in make_create/make_revoke, etc. It will be simply separate function called after successful event creation/selection.

I would allow multiple notes per event (as is in the PR) than to modifying the same one. Of course other possibility is simply to modify existing comment (another addEventNote would simply add lines to existing one). But I think it is more counter-intuitive behaviour (e.g. when searching for some concrete comment with specific first line (when/if API will be available).

New table could look like

CREATE TABLE event_comments (
   event_id INTEGER NOT NULL REFERENCES events(id),
   author NOT NULL REFERENCES users(id),
   created_ts TIMESTAMPTZ NOT NULL DEFAULT NOW(),
   title VARCHAR(255) NOT NULL,
   description TEXT,
) WITHOUT OIDS

Not sure about splitting title/description or taking first line (or simply something like SELECT LEFT(description, 80) could be enough?)
Default ordering by id should be sufficient to get reasonable order for multiple comments.

Metadata