Skip to content
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

Tickets updated with priority and severity #220

Merged

Conversation

paulsputer
Copy link
Collaborator

  • TODO: New entries appended to language files require updating

@gitblit
Copy link
Collaborator

gitblit commented Oct 1, 2014

Really Nice! It's like looking at my own code.

Changes I'd like to see:

  1. Drop the additions to the non-English translation files. Gitblit will default to English for missing keys and it makes it more clear to translators what they need to do as all translations are ordered identically.
  2. I'm wondering about the need for RepositoryModel.useSeverity and RepositoryModel.usePriority. I almost think we can drop that too and make the display of the default values more-or-less invisible. If you don't want to use those values, don't change them from the defaults.
  3. Might be a good idea to have visual indicators for priority - maybe severity too. They should be displayed in the tickets list, the ticket page, and perhaps in the selectors for new/edit ticket. We might be able to legitimately harvest icons from Atlassian AUI or from a different source.

@paulsputer
Copy link
Collaborator Author

Thanks, glad it's easy reading

  1. That sounds much better, thought it was a bit awkward as I was adding all those in! Out of interest is it wicket that manages the defaulting to English or is there some additional logic for that?

  2. It would certainly make the page templates more simplistic by avoiding the additional fragments of:

  • Option visibility on new tickets
  • Value visibility on ticket page
  • Filter visibility on ticket list page

The intention was to keep the UI clean for users who don't use them but it does feel as though it complicates the code far more than the value it provides... Lucene indexes them no matter the config, so it is purely for the UI. Happy to take them out to keep the code simple, as you say if they don't use them the defaults are pretty much invisible anyway.

2.1) I had thought of making the priority & severity enumerations project configurable but it looks like that would require far more changes than I wanted to do on this PR. Lucene indexes them as integers to maintain portability so this could be done at a later date if needed.

2.2) Similar to 2.1 but focusing on language, the strings are generated directly from the enum name, is there a way to disconnect the display name from the enum name?

  1. Sure, I was wanting to add some visual indicators especially on the ticket list page but the difficulty is deciding the best way to maintain the clean look and responsive design when doing this. I think icons are great for the ticket types but the severity/priority could be more awkward, especially if (2.1) wants to be introduced at some point later. I had thought of text in a UI lozenge but felt that may add too much clutter. I notice the ticket icons are all grayscale so we could put those to additional use and leverage the fact that they are treated as text and use a simple style mechanism such ascolor:hsla(<SeverityRating>, <saturation>, 50%, <PriorityRating>), This allows the integer ratings to be directly related to the Hue and Alpha:
  • Hue for Severity: Red (0) (Catastrophic) - yellow (Serious) - Green (120) (Negligible)

  • Alpha for Priority: High 1.0 -> Low 0.1

  • Saturation set with (SeverityRating < 0 ? 0% : 80%) to display unrated severity as black

    What do you think?

@paulsputer
Copy link
Collaborator Author

Changes complete, the calculated approach for color indicators didn't display quite as nicely as I was hoping for so I've implemented them as a lookup table for easier tuning to human eyes. The hue range has been widened to include purple at the catastrophic end. Alpha fades out ticket on lower priorities to a minimum of 20%. Default priority/severity settings should look similar to ticket icons in current release.

@paulsputer paulsputer force-pushed the ticketAttributesPrioritySeverity branch from fd23ec1 to 6d16bc6 Compare October 18, 2014 16:14
@paulsputer
Copy link
Collaborator Author

Cleaned pull request into one commit to ease identification of changes. Also updated to display priority icons.

@gitblit
Copy link
Collaborator

gitblit commented Oct 20, 2014

This is looking really nice. There are a few things I'd like addressed before I merge:

  1. I've gotten complaints before from folks with RG-color-blindness about state & use of color. I'm not crazy about the difference between "normal" and "high". I'd advocate that "normal" should have no priority indicator - although it does look pretty. That would make the assignment of a non-normal priority more visually meaningful.
  2. case-drop. In the future I may revisit the whole case issue, but for now we have to stick with all lower-case since the rest of the ui is all lower-case.

@steveno
Copy link
Contributor

steveno commented Oct 20, 2014

@gitblit +1 for color blindness awareness

@paulsputer paulsputer force-pushed the ticketAttributesPrioritySeverity branch from 6d16bc6 to 2c8c5ec Compare October 20, 2014 21:49
@paulsputer
Copy link
Collaborator Author

Ok, hopefully we're getting closer now :)

  • Updated priority & severity color usage to improve visibility for RG-color-blind users
  • Added css controlled severity-:after case for improved visibility to avoid reliance on color
  • Updated priority & severity names to maintain case consistency
  • Removed unused priority & severity strings in en language file
  • updated ticket icons to fixed width for alignment

+ Severity indicated via new character indicator and color of ticket icon on ticket list
+ Priority indicated via new priority icon and color on ticket list
+ Indexed as integers to provide sorting and maintain language neutral
index
+ Colours and indicator text controlled through CSS classes priority-<x> & severity-<x>
+ UITicketTest created to generate tickets of all types to ease debugging
@paulsputer paulsputer force-pushed the ticketAttributesPrioritySeverity branch from 2c8c5ec to f9c78c0 Compare October 20, 2014 21:55
@gitblit
Copy link
Collaborator

gitblit commented Oct 21, 2014

@paulsputer I've made a friendly amendment to your CSS changes which you can find here or at the pr-220 branch in this repository. The general idea is to make red the color for the most important things i.e. ROYGBIV is the guide. I'm also anticipating requests to translate 'C', 'Ca', etc in the CSS so I don't want to introduce that - but I propose a more clear alternative that I think I can defend against translation requests.

I've attached a screenshot of the results of this change. If you are fine with this adjustment I'll go ahead and merge.

screenshot from 2014-10-21 09 29 35

@paulsputer
Copy link
Collaborator Author

@gitblit yes that looks great! Maybe we could remove the text all together and just use the dots to avoid the translation issue? I didn't like using Ca, C etc but couldn't think of a more suitable alternative at the time, but the dots are clear, simple, and language neutral 👍

@gitblit gitblit merged commit f9c78c0 into gitblit-org:develop Oct 21, 2014
@gitblit
Copy link
Collaborator

gitblit commented Oct 21, 2014

I pulled out the S# text and set the tooltip with the severity from TicketsUI. We still have a translation issue - but all the ticket enums have the same problem and that is a rainy day job. The main thing is the translation is not in CSS which would be unmaintainable.

Thanks for taking the time to implement this improvement. It's really great!

@gitblit
Copy link
Collaborator

gitblit commented Oct 21, 2014

@paulsputer I'm giving more thought to the severity levels and wondering what your take is.

Priority = What should be done first?
Severity = How broken is it? What is the bug's impact?

I recognize that Severity is a standard term, but it implies bug or something wrong. That doesn't necessarily correlate well with a task, enhancement, or maintenance. What about labeling this as Impact or Importance, instead?

N Severity Impact
5 catastrophic showstopper?
4 critical high
3 serious medium
2 minor low
1 negligible cosmetic
0 unrated unrated

Language like that might be generally more useful in triaging. A task or an enhancement doesn't really have severity - but they do have importance or impact which might be orthogonal to priority. What do you think?

@paulsputer
Copy link
Collaborator Author

@gitblit yea it gets rather tricky with the wordings. Priority is the easy one, but severity/impact could have very project specific terms. From a technical perspective I had hoped to abstract these further to allow them to be customized on a per-project basis but I felt the change was already on the large side for one PR, and at this stage my focus is on creating a tool to migrate from Trac to Gitblit without losing this vital information. From what I could gather the project settings uses JGit config API, which doesn’t appear to provide access to a group of KVPs so this may be a bit more involved.

The terminology I’ve used here is based on ISO14971 which builds upon the more generic ISO16085 for risk management. Both refer to “probability of occurrence” and “severity of consequence” where the project specific weighting of both together determines the risk and if it is acceptable. All the standards specify that there should be a project specific description for each category, so there is scope for flexibility without going to the extreme of some ticket systems that do away with names all together and just have numbers, though there could be benefits in doing that internally in the code.

Importance is an awkward one as it becomes too similar to priority. My first instinct with severity/impact is that it is the difference between an engineering term vs a business term so I’m not so convinced that renaming provides the answer. I see what you mean though about the different ticket types, I think the main question in either case is how it should be interpreted for the task/ticket being categorized. Maybe the best way forward will become clearer as we use that new feature :)

gitblit added a commit that referenced this pull request Nov 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants