Add list of 'parent' tickets to ticket description table, solves #1184 #1185
Conversation
'parent' tickets are all tickets on which the current ticket depends.
The parent tickets can only be resolved if the current tickets is closed.
This patch splits the current |Dependencies|Total time spent| table row
in two rows. The table layout is now
|Status |Total time spent|
|Depends|Resolves |
Status and Total time only have one line. Status was chosen as filler
field, because I believe it should be moved out of the title to reduce
visual distraction.
Depends and Resolve will have a variable number of lines and are closely
related.
Depends was chosen instead of "Dependencies", to indicate directly what
the listed tickets do and because it is shorter.
The commit also simplifies the wording of the tooltips and harmonizes with
the other tooltips in the same table: It indicates directly what happens
when the button is clicked (or touched!).
Notes:
- The complete row could be omitted if now Depends or Resolves exists for
a ticket. This would reduce visual distraction but was not implemented
because it could lead to misled expectations.
- No button for removing/adding parent tickets was added. If considered
important this can be added any time. Currently a user has to go to
the parent ticket and delete the dependency (easy) or remember the ticket
id/title, search for the parent and add the ticket as dependency (difficult)
|
Hi @jorge-leon, thanks for your contribution! Do you think you can provide screenshots to better see the rendering? Buttons to add/remove dependencies could be a nice feature but it's up to you. And it could be done later in another PR. Just a side note: when changing the wording in |
Sure! With parent links:
With dependency link:
|
When I did the following, starting from the git checkout root directory:
I got a Don't know if a) I did it the right way and b) if I should commit such big changes. I prefer somebody more knowledgeable to create and commit the .po's. I'll do the English, German and Spanish translations then - as soon as I get access on Transifex. |
Will do that. I even propose to remove the status completely from the title: a user clicking on a ticket link does this normally based on a decision which includes evaluating the status shown in the ticket list where she/he found the ticket in the first place. The status is not an information that is primarily looked up by opening a ticket and looking at the title. Instead, the ticket title is the main information. As you point out, it's already quite verbose, IMHO it need not be repeated at all in this place. |
Next iteration:
New screenshots:Without dependencies:
With dependencies:Note: it is possible to make circular dependencies. See the DH-4 ticket, which is
|
helpdesk/views/staff.py
Outdated
| dependencies = [d for d in itertools.chain( | ||
| open_dependencies, | ||
| ticket.ticketdependency.all().difference(open_dependencies) | ||
| )] |
There was a problem hiding this comment.
I don't understand why you need to do this. Isn't it possible to simply do :
| dependencies = [d for d in itertools.chain( | |
| open_dependencies, | |
| ticket.ticketdependency.all().difference(open_dependencies) | |
| )] | |
| dependencies = ticket.ticketdependency.all() |
Maybe you wish to keep open statuses at the beginning? As ticket.status is an integer field, it should be possible to add ordering :
| dependencies = [d for d in itertools.chain( | |
| open_dependencies, | |
| ticket.ticketdependency.all().difference(open_dependencies) | |
| )] | |
| dependencies = ticket.ticketdependency.order_by('depends_on__status') |
There was a problem hiding this comment.
Yes: It's about ordering. I use helpdesk (also) for tracking software development/maintenance. With the ticket dependencies I can model milestones, but I want to trickle resolved/closed tickets down the list, so I can pick always from top.
I thought about ordering by status first, just like you point it out. But since ticket status can be configured / named freely ordering might not
in all cases put open tickets on top. In contrast, using the TICKET_OPEN_STATUSES settings is a stable approach.
The itertools trick allowed me to keep the template code tidy, since there is only one loop to go through - or an empty list. The implementation is based on the assumption, that the number of dependent ticktes is not huge. I would have preferred a QueryList, but could not came up with an order preserving expression.
Another side topic would be to sort the two lists (open/closed tickets) individually before creating the list, but then I did not have a specific preference and left it out.
There was a problem hiding this comment.
Given that status values can be overridden and there is no directive to specify status values to specifically accommodate ordering so that the open statuses appear first, it may be better to enhance the operators available in the ORM to include "NOT IN" to retrieve the unopened dependencies like this:
ticket.ticketdependency.filter(depends_on__status__not_in=Ticket.OPEN_STATUSES)
Then you simplify the code to this:
open_dependencies = ticket.ticketdependency.filter(depends_on__status__in=Ticket.OPEN_STATUSES)
dependencies = open_dependencies + ticket.ticketdependency.filter(depends_on__status__not_in=Ticket.OPEN_STATUSES)
To do this we add this class to a module helpdesk/orm_operators.py:
from django.db.models.lookups import In
class NotIn(In):
lookup_name = "not_in"
def get_rhs_op(self, connection, rhs):
return "NOT IN %s" % rhs
Then make the new operator available in the ORM by adding this to the ready method in helpdesk/apps.py:
from django.db.models.fields import Field
from helpdesk.orm_operators import NotIn
Field.register_lookup(NotIn)
There was a problem hiding this comment.
The + does not work on querysets, but it seems to work with |
There was a problem hiding this comment.
| is not order preserving. The current implementation still uses NotIn, but concatenates via list()+list()
Any update on .po's and Transifex access? |
@jorge-leon I am unaware of anyone with access to Trnsifex. I suggest you do the translations you can and we will see if we can get other users of the project who need the other supported languages to update the relevant language files via a PR. |
Oh! the |
I was unaware of that... I have requested access to the Helpdesk project in Transifex and will provide you with access as soon as I get access (I ssume you have requested access after creating an open source user ID in Transifex) |
When adding a dependent or parent ticket, the choice field in the ticket selection form excludes: - All existing dependencies of the current ticket. - All existing parent tickets of the current ticket. - The current ticket itself. The first two prevent immediate circular references: A dependency cannot be a parent and vice versa. Deeper circular references are not covered by this: a ticket can still be its own grandchild. They also prevent current behavior of throwing an `IntegrityException` when selecting a dependency or parent. The third one prevents also a quirky behavior: until now, specifying the ticket itself as parent or dependency just does not save the dependency and does not issue a warning either.
It makes no sense to make a ticket depend on an already closed ticket, or to make a closed ticket depend on another. The only case I can think of is to create such a relationship in hindsight for documentation purposes. This can be implemented by adding an admin interface or giving the administrator more choices.
helpdesk/views/staff.py
Outdated
| ticketdependency.depends_on = depends_on | ||
| if ticketdependency.ticket != ticketdependency.depends_on: | ||
| ticketdependency.save() | ||
| return HttpResponseRedirect(reverse('helpdesk:view', args=[depends_on.id])) |
There was a problem hiding this comment.
You use shortcut like this I believe:
| return HttpResponseRedirect(reverse('helpdesk:view', args=[depends_on.id])) | |
| return redirect(depends_on.get_absolute_url()) |
There was a problem hiding this comment.
Not my code. Shall I change it anyway? ... Oh: I see: that's bad code, I'll update
There was a problem hiding this comment.
As you want, maybe this could be done for each occurrence in another MR. It's been a while since I'm thinking about refactoring/modernizing the code base (especially views and better formatting templates).
There was a problem hiding this comment.
Changed in two places, checked in, test are running, the first test already passed.
| @@ -422,8 +423,13 @@ def view_ticket(request, ticket_id): | |||
|
|
|||
| return redirect('helpdesk:edit_ticket_checklist', ticket.id, checklist.id) | |||
|
|
|||
| # Open tickets on top | |||
| dependencies = list(ticket.ticketdependency.filter(depends_on__status__in=Ticket.OPEN_STATUSES)) \ | |||
| + list(ticket.ticketdependency.filter(depends_on__status__not_in=Ticket.OPEN_STATUSES)) | |||
There was a problem hiding this comment.
not_in lookup is more readable so no need to change it, but I think ticket.ticketdependency.exclude(depends_on__status__in=Ticket.OPEN_STATUSES) would do the same
There was a problem hiding this comment.
You are right. If you followed the related discussion you might see how we got here. The primary goal - a 'fast' way to concatentate two QuerySets in order - cannot be achieved anyway. So I might as well remove the NotIn ORM operator, since this is the only place where it is used to remove code complexity.
There might be a way to do the chaining via annotations, as show here for SQL.
There was a problem hiding this comment.
from django.db.models import Value
open_deps = ticket.ticketdependency.filter(depends_on__status__in=Ticket.OPEN_STATUSES)).annotate(rank=Value(1))
not_open_deps = ticket.ticketdependency.exclude(depends_on__status__in=Ticket.OPEN_STATUSES)).annotate(rank=Value(2))
dependencies=open_deps.union(not_open_deps).order_by('rank')
What do you think?
There was a problem hiding this comment.
Yep, this could be a readable solution! Too bad there isn't an easy one for that king of thing
'parent' tickets are all tickets on which the current ticket depends.
The parent tickets can only be resolved if the current tickets is closed.
This patch splits the current |Dependencies|Total time spent| table row in two rows. The table layout is now
Status and Total time only have one line. Status was chosen as filler field, because I believe it should be moved out of the title to reduce visual distraction.
Depends and Resolve will have a variable number of lines and are closely related.
Depends was chosen instead of "Dependencies", to indicate directly what the listed tickets do and because it is shorter.
The commit also simplifies the wording of the tooltips and harmonizes with the other tooltips in the same table: It indicates directly what happens when the button is clicked (or touched!).
Notes: