pagure

#1677 Issue 1544 - RFE - Add colored tags
Merged by pingou. Opened by mreynolds.
mreynolds/pagure iss1544  into  master

Download 1677.patch

I only extended the base Tag
object to include color, projectID, and a unique id. Then I removed the
dependency of the issue from the tag for working in the ui. Otherwise
tags were only visible if an issue contained that tag. This also meant
you could not add, edit, or remove a tag unless you had already added
one to an issue. So in my patch it updated the setting.html page to
show all tags (whether or not they existed in an issue). You can add,
edit and delete them. They also automatically update all the issues in
that project with the color/name change. This is also reflected in both
issues.html & issue.html.

I also added a unique identifier to each tag(tag_id) - this allows every
project to use the same name with its own unique color.  There are no
conflicts since the primary key is now the unique identifier(tag_id)
instead of the tag name(tag).
This patch enforces that tags are only created from the settings page.
This patch is missing the complete alembic migration script.

rebased

rebased

rebased

Since we have the tag_id above, I doubt we need to keep this one

This seems wrong as well, the color is a property of the tag itself, so we shouldn't store it again here, otherwise we're implying that the tag can have a color and the one assigned to an issue can have another which iiuc isn't what we want.

We can drop that init, the default value in the DB will take care of the default value if none is provided.

So after spending some time on trying to come up with a migration script and giving it some thoughts, I wonder if the solution isn't to go for new tables.

Basically, create the tags_colored and tags_issues_colored tables have the alembic script migrate the data in them.

Then in a later release we can a) drop the code related to tags and tags_issues and b) drop them from the DB as well. (and, optionally, rename tags_colored to tags but at that point I don't think the gain will be worth the pain).

4 new commits added

  • Add the alembic migration allowing to migrate tags to the new model
  • Adjust unit-tests for the changes made to the colored tags feature
  • Rework the colored tag feature
  • Create the tags_colored and tags_issues_colored tables in the DB

Ok, so I spent sometime reworking bits of this feature, I think this commit lists most of the change:
https://pagure.io/fork/mreynolds/pagure/c/3b03480f827992e652a2aaaae9046f14ca5f0ef8

Most notably, I dropped the hard-coded list of color in favor of an html5 color-picker widget allowing the users to use any color they wish to, giving us much more flexibility.

Unit-tests are passing for me locally, so did the alembic migration script.

More feedback are welcome as usual :)

1 new commit added

  • Include the identifier in the string representation of the TagColored object

if tag_names is a list then why not iterating through like for tag_name in tag_names: ?

why not just check tag_name != "" why use continue ?

We can definitely simplify

Same iteration issues here !

rebased

Looks good to me now :)

Ok, so I spent sometime reworking bits of this feature, I think this commit lists most of the change:
https://pagure.io/fork/mreynolds/pagure/c/3b03480f827992e652a2aaaae9046f14ca5f0ef8
Most notably, I dropped the hard-coded list of color in favor of an html5 color-picker widget allowing the users to use any color they wish to, giving us much more flexibility.

FYi, The reason I did the hardcoded color list over the widget was because most colors make terrible tags. I chose "darker" colors as it's easier to read the the tag name/text inside of the tag icon. That being said I'm totally fine with this change :)

I'm going to do some testing with all these changes you made and make sure it still works as designed. I should have feedback later today.

Unit-tests are passing for me locally, so did the alembic migration script.
More feedback are welcome as usual :)

FYi, The reason I did the hardcoded color list over the widget was because most colors make terrible tags. I chose "darker" colors as it's easier to read the the tag name/text inside of the tag icon. That being said I'm totally fine with this change :)

I agree on this but I changed it for two reasons:
- if it's un-readable it's not our problem it's their's :)
- it gives us a much larger range of choices (that was the main argument for me)

1 new commit added

  • Ensure the provided color match the expected pattern

rebased

rebased

Shouldn't the alembic upgrade script delete the old generic tags, and do it in a way that won't interfere with updating all projects (as the old tags were shared between projects)?

rebased

Looks good! Thanks for helping on this one!!!

Before merging, I'm adding a small test for update_tags (which I think we should move btw, to repo.py likely since this is now project-wide)

1 new commit added

  • Add unit-tests for the update_tags endpoint

9 new commits added

  • Add unit-tests for the update_tags endpoint
  • Ensure the provided color match the expected pattern
  • Rework the logic in update_tags in pagure/ui/issues.py as suggested by @farhaan
  • Include the identifier in the string representation of the TagColored object
  • Add the alembic migration allowing to migrate tags to the new model
  • Adjust unit-tests for the changes made to the colored tags feature
  • Rework the colored tag feature
  • Create the tags_colored and tags_issues_colored tables in the DB
  • Issue 1544 - RFE - Add colored tags

Looks good

Alright, let's rebase one last time and merge then :)

rebased

Pull-Request has been merged by pingou
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.