Issue 194113: Support System first upload (Closed)
|
Created:
14 years, 2 months ago by sharoonthomas Modified:
14 years, 1 month ago Visibility:
Public. |
Patch Set 1 #
Total comments: 46
Patch Set 2 : Chnages from udono's comments #Patch Set 3 : Updates from udono #
Total comments: 2
Patch Set 4 : Changed DB schema according to suggestions by Bechamel #
Total comments: 7
Patch Set 5 : Changes proposed by yangoon updated #
Total comments: 17
MessagesTotal messages: 12
Hi All, This is the support system code. Its working. Please review this for me.Thanks for the reviews in the sales opportunity module. Sharoon
Sign in to reply to this message.
looks good so far. Some comments from my side attached. Cheers Udo http://codereview.appspot.com/194113/diff/1/5 File support.py (right): http://codereview.appspot.com/194113/diff/1/5#newcode7 support.py:7: PRIORITY = [('vhigh', 'Very High'), very_high ... very_low http://codereview.appspot.com/194113/diff/1/5#newcode15 support.py:15: ('onhold', 'On Hold'), on_hold http://codereview.appspot.com/194113/diff/1/5#newcode27 support.py:27: help="Eg. Technical Support - Level 1") In help I would not put examples. Better is explanation. http://codereview.appspot.com/194113/diff/1/5#newcode36 support.py:36: ('id', If(In('company', Eval('context', {})), '=', '!='), '=', '!=') is this right? http://codereview.appspot.com/194113/diff/1/5#newcode40 support.py:40: children = fields.One2Many('support.queue', 'parent', 'Lower Level Queues') Why using Adjacency List Model? Tryton has also Modified Preorder Tree Traversal implemented, which is read optimized. hint: grep for 'left' and 'right' http://codereview.appspot.com/194113/diff/1/5#newcode69 support.py:69: LOCK_SOLVED = { Don't understand this name http://codereview.appspot.com/194113/diff/1/5#newcode80 support.py:80: ticket_no = fields.Char('Ticket No', readonly=True, select=1,) code is usually used in Tryton for a char field like this. http://codereview.appspot.com/194113/diff/1/5#newcode81 support.py:81: name = fields.Char('Title', required=True, select=1, name != Title use better the same http://codereview.appspot.com/194113/diff/1/5#newcode87 support.py:87: queue = fields.Many2One('support.queue', 'Department', select=1, Department? http://codereview.appspot.com/194113/diff/1/5#newcode93 support.py:93: assigned_to = fields.Many2One('company.employee', 'Assignee', select=2, assigned_to != Assignee, better the same http://codereview.appspot.com/194113/diff/1/5#newcode101 support.py:101: escalated = fields.Boolean('Escalated ?', readonly=True, select=2, I like on boolean the naming is_xyz or has_xyz. But this is maybe personal... http://codereview.appspot.com/194113/diff/1/5#newcode103 support.py:103: state = fields.Selection(STATES, 'Status', readonly=True, select=1,) State sounds better for me then Status http://codereview.appspot.com/194113/diff/1/5#newcode162 support.py:162: #If the ticket's queue has a parent comment unusefull and wrong place http://codereview.appspot.com/194113/diff/1/5#newcode167 support.py:167: self.write(cursor, user, ticket_id, {'queue':ticket.queue.parent.id, 'state':'escalated', 'escalated':True}, context=context) I find better: args = { 'queue':ticket.queue.parent.id, 'state':'escalated', 'escalated':True, } self.write(cursor, user, ticket_id, args, context=context) http://codereview.appspot.com/194113/diff/1/5#newcode204 support.py:204: ticket = fields.Many2One('support.ticket', 'Support Ticket') s/Support Ticket/Ticket http://codereview.appspot.com/194113/diff/1/5#newcode251 support.py:251: ticket_no = fields.Char('Ticket #') code ... Code http://codereview.appspot.com/194113/diff/1/5#newcode252 support.py:252: name = fields.Char('Title') name != Title http://codereview.appspot.com/194113/diff/1/5#newcode255 support.py:255: queue = fields.Many2One('support.queue', 'Department',) queue != Department http://codereview.appspot.com/194113/diff/1/5#newcode258 support.py:258: assigned_to = fields.Many2One('company.employee', 'Assignee',) assigned_to != Assignee http://codereview.appspot.com/194113/diff/1/5#newcode261 support.py:261: state = fields.Selection(STATES, 'Status',) State
Sign in to reply to this message.
http://codereview.appspot.com/194113/diff/1/6 File support.xml (right): http://codereview.appspot.com/194113/diff/1/6#newcode5 support.xml:5: <menuitem id="menu_main_support" name="Support System" icon="tryton-users" /> isn't a closer name to the functionality better, like support tickets or so? But Iam unsure http://codereview.appspot.com/194113/diff/1/6#newcode22 support.xml:22: <form string="Edit Queue"> I think Iam not a friend of this Queue word. I do not understand this queue naming at all... isn't there a simpler name? http://codereview.appspot.com/194113/diff/1/6#newcode168 support.xml:168: <button name="hold" string="Put On Hold" only 'Hold' or On Hold http://codereview.appspot.com/194113/diff/1/6#newcode171 support.xml:171: <button name="reopen" string="Re-open" Re-Open
Sign in to reply to this message.
http://codereview.appspot.com/194113/diff/1/5 File support.py (right): http://codereview.appspot.com/194113/diff/1/5#newcode7 support.py:7: PRIORITY = [('vhigh', 'Very High'), On 2010/01/27 20:33:03, udo.spallek wrote: > very_high > ... > very_low Done. http://codereview.appspot.com/194113/diff/1/5#newcode15 support.py:15: ('onhold', 'On Hold'), On 2010/01/27 20:33:03, udo.spallek wrote: > on_hold Done. http://codereview.appspot.com/194113/diff/1/5#newcode27 support.py:27: help="Eg. Technical Support - Level 1") On 2010/01/27 20:33:03, udo.spallek wrote: > In help I would not put examples. Better is explanation. Done. http://codereview.appspot.com/194113/diff/1/5#newcode40 support.py:40: children = fields.One2Many('support.queue', 'parent', 'Lower Level Queues') On 2010/01/27 20:33:03, udo.spallek wrote: > Why using Adjacency List Model? Tryton has also Modified Preorder Tree Traversal > implemented, which is read optimized. > > hint: grep for 'left' and 'right' Done. http://codereview.appspot.com/194113/diff/1/5#newcode69 support.py:69: LOCK_SOLVED = { On 2010/01/27 20:33:03, udo.spallek wrote: > Don't understand this name Done. http://codereview.appspot.com/194113/diff/1/5#newcode80 support.py:80: ticket_no = fields.Char('Ticket No', readonly=True, select=1,) On 2010/01/27 20:33:03, udo.spallek wrote: > code is usually used in Tryton for a char field like this. Done. http://codereview.appspot.com/194113/diff/1/5#newcode81 support.py:81: name = fields.Char('Title', required=True, select=1, On 2010/01/27 20:33:03, udo.spallek wrote: > name != Title use better the same Done. http://codereview.appspot.com/194113/diff/1/5#newcode87 support.py:87: queue = fields.Many2One('support.queue', 'Department', select=1, On 2010/01/27 20:33:03, udo.spallek wrote: > Department? Done. http://codereview.appspot.com/194113/diff/1/5#newcode93 support.py:93: assigned_to = fields.Many2One('company.employee', 'Assignee', select=2, On 2010/01/27 20:33:03, udo.spallek wrote: > assigned_to != Assignee, better the same Done. http://codereview.appspot.com/194113/diff/1/5#newcode101 support.py:101: escalated = fields.Boolean('Escalated ?', readonly=True, select=2, On 2010/01/27 20:33:03, udo.spallek wrote: > I like on boolean the naming is_xyz or has_xyz. But this is maybe personal... Done. http://codereview.appspot.com/194113/diff/1/5#newcode103 support.py:103: state = fields.Selection(STATES, 'Status', readonly=True, select=1,) On 2010/01/27 20:33:03, udo.spallek wrote: > State sounds better for me then Status Done. http://codereview.appspot.com/194113/diff/1/5#newcode162 support.py:162: #If the ticket's queue has a parent On 2010/01/27 20:33:03, udo.spallek wrote: > comment unusefull and wrong place Done. http://codereview.appspot.com/194113/diff/1/5#newcode167 support.py:167: self.write(cursor, user, ticket_id, {'queue':ticket.queue.parent.id, 'state':'escalated', 'escalated':True}, context=context) On 2010/01/27 20:33:03, udo.spallek wrote: > I find better: > args = { > 'queue':ticket.queue.parent.id, > 'state':'escalated', > 'escalated':True, > } > > self.write(cursor, user, ticket_id, args, context=context) > Done. http://codereview.appspot.com/194113/diff/1/5#newcode251 support.py:251: ticket_no = fields.Char('Ticket #') On 2010/01/27 20:33:03, udo.spallek wrote: > code ... Code Done. http://codereview.appspot.com/194113/diff/1/5#newcode252 support.py:252: name = fields.Char('Title') On 2010/01/27 20:33:03, udo.spallek wrote: > name != Title Done. http://codereview.appspot.com/194113/diff/1/5#newcode255 support.py:255: queue = fields.Many2One('support.queue', 'Department',) On 2010/01/27 20:33:03, udo.spallek wrote: > queue != Department Done. http://codereview.appspot.com/194113/diff/1/5#newcode258 support.py:258: assigned_to = fields.Many2One('company.employee', 'Assignee',) On 2010/01/27 20:33:03, udo.spallek wrote: > assigned_to != Assignee Done. http://codereview.appspot.com/194113/diff/1/5#newcode261 support.py:261: state = fields.Selection(STATES, 'Status',) On 2010/01/27 20:33:03, udo.spallek wrote: > State Done.
Sign in to reply to this message.
http://codereview.appspot.com/194113/diff/1/6 File support.xml (right): http://codereview.appspot.com/194113/diff/1/6#newcode5 support.xml:5: <menuitem id="menu_main_support" name="Support System" icon="tryton-users" /> Can somebody else also suggest abt this? On 2010/01/28 09:23:51, udono wrote: > isn't a closer name to the functionality better, like support tickets or so? But > Iam unsure http://codereview.appspot.com/194113/diff/1/6#newcode22 support.xml:22: <form string="Edit Queue"> Its fairly standard. again lets go with names more people are comfortable with? Somebody pls comment. On 2010/01/28 09:23:51, udono wrote: > I think Iam not a friend of this Queue word. I do not understand this queue > naming at all... isn't there a simpler name? http://codereview.appspot.com/194113/diff/1/6#newcode168 support.xml:168: <button name="hold" string="Put On Hold" On 2010/01/28 09:23:51, udono wrote: > only 'Hold' or On Hold Done. http://codereview.appspot.com/194113/diff/1/6#newcode171 support.xml:171: <button name="reopen" string="Re-open" On 2010/01/28 09:23:51, udono wrote: > Re-Open Done.
Sign in to reply to this message.
Thx! Just some comments on first reading: http://codereview.appspot.com/194113/diff/16/20 File support.py (right): http://codereview.appspot.com/194113/diff/16/20#newcode77 support.py:77: STATE_LOCK_SOLVED = { Predefinitions at top of file. STATE_SOLVED would be enough for me. LOCKS in tryton code often refer to database locks. http://codereview.appspot.com/194113/diff/16/20#newcode121 support.py:121: 'no_escalate_q': 'You cannot escalate any further!', 'no_escalate' http://codereview.appspot.com/194113/diff/2004/31#newcode7 support.py:7: PRIORITY = [('very_high', 'Very High'), PRIORITIES http://codereview.appspot.com/194113/diff/2004/31#newcode161 support.py:161: :param cursor: the database cursor For what is it needed? AFAIS escalation is already documented through states and history. http://codereview.appspot.com/194113/diff/2004/31#newcode307 support.py:307: two lines between classes http://codereview.appspot.com/194113/diff/2004/31#newcode309 support.py:309: 'Support Ticket History Lines' http://codereview.appspot.com/194113/diff/2004/31#newcode312 support.py:312: better single quotes http://codereview.appspot.com/194113/diff/2004/31#newcode315 support.py:315: Why different naming from Support Ticket? code = fields.Char('Ticket No' ? http://codereview.appspot.com/194113/diff/2004/32 File support.xml (right): http://codereview.appspot.com/194113/diff/2004/32#newcode5 support.xml:5: <menuitem id="menu_main_support" name="Support System" icon="tryton-users" /> My favourites: 'Issue Tracking System' or 'Ticketing System' http://en.wikipedia.org/wiki/Issue_tracking_system http://en.wikipedia.org/wiki/Comparison_of_issue_tracking_systems http://de.wikipedia.org/wiki/Trouble-Ticket-System
Sign in to reply to this message.
Hey sharoon, putting my additional questions and remarks. I' am unsure if I understand your aim and the requirements of the implementation. So please be patient with me ;-) Regards Udo http://codereview.appspot.com/194113/diff/36/40 File support.py (right): http://codereview.appspot.com/194113/diff/36/40#newcode41 support.py:41: right = fields.Integer('Right', required=True, select=1) 1. How are the parent and children queues are interacting? 2. What happens with a ticket which income in a root queue which has two or more children? 3. I understand queue as a tunnel, which has one input and one output. The tickets in a queue are sorted by a sequence. New tickets come to the top, older tickets behind. It is possible to perform on the ticket queue methods like First-In-First-Out or Last-In-Last-Out or preferable in this case Last-In-First-Out. 3.1 Isn't it a level to low to have on queue parent/children? For me it looks like you wantet to put different entities together: the ticket queues and the organisation structure. 3.2 Is there a super object missing, which handles the collections of queues and their paren/child relationships and the rules, which child is when used? 4. Is it not better to have a flat queue without self recursive relationships (without parent/children) for later simply attach or rebuild a mailbox? http://codereview.appspot.com/194113/diff/36/40#newcode44 support.py:44: solution_time = fields.Float('Solution Time', select=2,) For me, these attributes are ticket attributes: response_time solution_time On queue level there can be a time_limit to solve or react on tickets, if the company has guidelines/contracts for this. But this functionality is a good addon module. http://codereview.appspot.com/194113/diff/36/40#newcode66 support.py:66: 'Support Teams' Here I would organize the support structure with parent and child to be open for the specific organizational requirements. * Channel-Entrance * Level 1 * Calm team for the outraged customers * Lawyer * Level 2 * Level 3 * Development * Feedback Team But there must also be a path for the flexible handling of escallations. http://codereview.appspot.com/194113/diff/36/40#newcode139 support.py:139: _history = True Histor on ticket I like! http://codereview.appspot.com/194113/diff/36/40#newcode147 support.py:147: states=STATE_LOCK_SOLVED, depends=['active', 'state']) initiator_party, Initiator Party ? http://codereview.appspot.com/194113/diff/36/40#newcode260 support.py:260: class SupportTicketFollowup(ModelSQL, ModelView): Followups are flat, too. Isn't it an idea, to store them as a nested list with parent/children. With this it is easily to attach a later Email conversation adequate with nested replies...
Sign in to reply to this message.
http://codereview.appspot.com/194113/diff/36/40 File support.py (right): http://codereview.appspot.com/194113/diff/36/40#newcode44 support.py:44: solution_time = fields.Float('Solution Time', select=2,) On 2010/01/28 14:02:13, udono wrote: > For me, these attributes are ticket attributes: > response_time > solution_time > > On queue level there can be a time_limit to solve or react on tickets, if the > company has guidelines/contracts for this. But this functionality is a good > addon module. It could be average if so it must be computed fields http://codereview.appspot.com/194113/diff/36/40#newcode65 support.py:65: class SupportTeam(ModelSQL, ModelView): Why not use Tryton groups? http://codereview.appspot.com/194113/diff/36/40#newcode76 support.py:76: manager = fields.Many2One('company.employee', 'Team Manager', select=2, required=True, I'm not sure we must use employee here. I find more logical to use user because I don't see cases where tickets will be encoded for non-user employees. http://codereview.appspot.com/194113/diff/36/40#newcode76 support.py:76: manager = fields.Many2One('company.employee', 'Team Manager', select=2, required=True, What is the role of manager? http://codereview.appspot.com/194113/diff/36/40#newcode150 support.py:150: active = fields.Boolean('Active', readonly=True, select=2, Not sure it is useful. http://codereview.appspot.com/194113/diff/36/40#newcode152 support.py:152: priority = fields.Selection(PRIORITIES, 'Priority', required=True, select=1, I always found that fixed priorities are not working. Because a ticket can start to be low priority but with time if nothing is done it must increase in priority otherwise it will never be fixed. I have no magic answer (for now :-)) but I think it will be good to think about it. It will be a computed field perhaps but search could become difficult. Or a field that will be updated by a cron task. Also I think limited selection doesn't scale well, I will prefer a integer (like for sequence) http://codereview.appspot.com/194113/diff/36/40#newcode227 support.py:227: 'state':'escalated', I don't understand the state escalated. It seems to be like a one shot and doesn't have the same meaning then other state. I would prefer to have the queue way the tick have done. http://codereview.appspot.com/194113/diff/36/40#newcode248 support.py:248: code = sequence_obj.get(cursor, user, 'support.ticket', For information, bechamel is working on changing this sequence way. http://codereview.appspot.com/194113/diff/36/40#newcode260 support.py:260: class SupportTicketFollowup(ModelSQL, ModelView): I will name it messages. http://codereview.appspot.com/194113/diff/36/40#newcode260 support.py:260: class SupportTicketFollowup(ModelSQL, ModelView): On 2010/01/28 14:02:13, udono wrote: > Followups are flat, too. Isn't it an idea, to store them as a nested list with > parent/children. With this it is easily to attach a later Email conversation > adequate with nested replies... Good idea. http://codereview.appspot.com/194113/diff/36/40#newcode269 support.py:269: private = fields.Boolean('Private update?', Why not internal instead of private?
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
