Ticketing support #279

Merged
merged 13 commits into from Mar 6, 2014

Projects

None yet

5 participants

@anasouma
Contributor

No description provided.

@beittenc beittenc commented on an outdated diff Feb 20, 2014
SoftLayer/CLI/modules/ticket.py
+from SoftLayer import TicketManager
+from SoftLayer.CLI import CLIRunnable, Table, resolve_id, NestedDict, KeyValueTable
+
+""" Global variables for the status of the ticket - either open or closed """
+OPEN = 'open'
+CLOSED = 'closed'
+
+
+class ListTickets(CLIRunnable):
+ """
+ usage: sl ticket list [--status=open,closed]
+
+ List tickets
+
+ Options:
+ --status=open or closed Display only opened or closed tickets, otherwise display both
@beittenc
beittenc Feb 20, 2014 Contributor

I think there's different docopt syntax that can be used here to better indicate the possible options. Have you tried this?

--status=open|closed

@beittenc beittenc commented on an outdated diff Feb 20, 2014
SoftLayer/CLI/modules/ticket.py
+ usage: sl ticket list [--status=open,closed]
+
+ List tickets
+
+ Options:
+ --status=open or closed Display only opened or closed tickets, otherwise display both
+
+ """
+ action = 'list'
+
+ def execute(self, args):
+ ticket_mgr = TicketManager(self.client)
+
+ mask = 'id,accountId,title,createDate,lastEditDate,assignedUser[firstName, lastName]'
+
+ """Determine what needs to be returned, either open tickets, closed tickets, or both"""
@beittenc
beittenc Feb 20, 2014 Contributor

I'm confused by the string literal here. Based on contents and context, I think this should be a comment instead.

@beittenc beittenc commented on an outdated diff Feb 20, 2014
SoftLayer/CLI/modules/ticket.py
+ List tickets
+
+ Options:
+ --status=open or closed Display only opened or closed tickets, otherwise display both
+
+ """
+ action = 'list'
+
+ def execute(self, args):
+ ticket_mgr = TicketManager(self.client)
+
+ mask = 'id,accountId,title,createDate,lastEditDate,assignedUser[firstName, lastName]'
+
+ """Determine what needs to be returned, either open tickets, closed tickets, or both"""
+ neither = True
+ if (args.get('--status') == OPEN) or (args.get('--status') == CLOSED):
@beittenc
beittenc Feb 20, 2014 Contributor

I'm not a fan of extra parentheses when they're not needed. I don't think it improves readability and just increases line length. I'd probably also lean towards something like:

if args.get in [OPEN, CLOSED]:

But let's see if @sudorandom has another idea.

@beittenc beittenc commented on an outdated diff Feb 20, 2014
SoftLayer/CLI/modules/ticket.py
+ Options:
+ --body=BODY The entry that will be appended to the ticket
+
+ """
+ action = 'update'
+ options = ['--body']
+
+ def execute(self, args):
+ mgr = TicketManager(self.client)
+
+ ticket_id = resolve_id(
+ mgr.resolve_ids, args.get('<identifier>'), 'ticket')
+
+ body = args.get('--body')
+ if (body is None):
+ body = TicketUtils.open_editor(beg_msg="***** Softlayer Ticket Content ******")
@beittenc
beittenc Feb 20, 2014 Contributor

I see this header used multiple times. Could we make it a constant so it's easier to change in the future should the need arise?

@beittenc beittenc commented on an outdated diff Feb 20, 2014
SoftLayer/managers/ticket.py
+ self.ticket = self.client['Ticket']
+
+ def list_tickets(self, status='open', title=None, userId=None, **kwargs):
+ """ List all tickets
+
+ :param string status: status of tickets to retrieve, Open by default
+ :param string title: filter based on title
+ :param dict \*\*kwargs: response-level arguments (limit, offset, etc.)
+ """
+ TICKET_MASK = ('id', 'accountId', 'title', 'createDate', 'lastEditDate', 'assignedUser[firstName, lastName]')
+ if 'mask' not in kwargs:
+ kwargs['mask'] = TICKET_MASK
+
+ _filter = NestedDict(kwargs.get('filter') or {})
+ if title:
+ _filter['ticket']['title'] = \
@beittenc
beittenc Feb 20, 2014 Contributor

I don't think there's a need to split this line; I'm pretty sure it will still be under 80 characters.

@beittenc beittenc commented on an outdated diff Feb 20, 2014
SoftLayer/managers/ticket.py
+
+ """
+ if 'mask' not in kwargs:
+ items = set([
+ 'id',
+ 'title',
+ 'assignedUser[firstName, lastName]',
+ 'createDate',
+ 'lastEditDate',
+ 'updates[entry]',
+ 'updateCount',
+ ])
+ kwargs['mask'] = "mask[%s]" % ','.join(items)
+ return self.ticket.getObject(id=ticket_id, **kwargs)
+
+ def create_ticket(self, title=None, body=None, hardware=None, rootPassword=None, subject=None, **kwargs):
@beittenc
beittenc Feb 20, 2014 Contributor

Purely a question: Are there any valid kwargs here? The normal response-level arguments of limit, offset, etc. shouldn't apply when creating a ticket. So what would someone use?

@beittenc beittenc commented on an outdated diff Feb 20, 2014
SoftLayer/managers/ticket.py
+ """
+
+ currentUser = self.account.getCurrentUser()
+ new_ticket = {
+ 'subjectId': subject,
+ 'contents': body,
+ 'assignedUserId': currentUser['id'],
+ 'title': title,
+ }
+ if (hardware is None):
+ created_ticket = self.ticket.createStandardTicket(new_ticket, body, **kwargs)
+ else:
+ created_ticket = self.ticket.createStandardTicket(new_ticket, body, hardware, rootPassword, **kwargs)
+ return created_ticket
+
+ def update_ticket(self, t_id=None, body=None, **kwargs):
@beittenc
beittenc Feb 20, 2014 Contributor

If you're going to allow **kwargs, please make sure to put it in the docblock.

@beittenc
Contributor

Looks like you also need to add some unit tests for the new manager. I'd also encourage you to try some integration and functional tests for the CLI if you've got the time. Otherwise, I'm really liking what I see here and most of my notes are more stylistic than anything else.

@anasouma
Contributor

Hey Nathan, thanks for your comments, I will try to work on them today or tomorrow.
Wissam

@underscorephil underscorephil referenced this pull request Feb 27, 2014
Closed

CLI Create Ticket #267

@anasouma
Contributor
anasouma commented Mar 6, 2014

Sorry it took a while, this should be fine now. I need to figure out how to take care of those pep8 issues on my local machine.

@sudorandom sudorandom commented on an outdated diff Mar 6, 2014
SoftLayer/CLI/modules/ticket.py
+
+ # mask = 'id,accountId,title,createDate,lastEditDate,'\
+ # 'assignedUser[firstName, lastName]'
+
+ tickets = ticket_mgr.list_tickets(
+ open=args.get('--open'),
+ closed=args.get('--closed'))
+
+ t = Table(['id', 'assigned user', 'title',
+ 'creation date', 'last edit date'])
+
+ for ticket in tickets:
+ if ticket['assignedUser']:
+ t.add_row([
+ ticket['id'],
+ ticket['assignedUser']['firstName'] +
@sudorandom
sudorandom Mar 6, 2014 Contributor

String concatenation isn't typically preferred in Python for the sake of readability. Usually you would do something like this in Python:

"%s %s" % (ticket['assignedUser']['firstName'],
           ticket['assignedUser']['lastName'])
@sudorandom sudorandom commented on an outdated diff Mar 6, 2014
SoftLayer/CLI/modules/ticket.py
+
+ Options:
+ --body=BODY The entry that will be appended to the ticket
+
+ """
+ action = 'update'
+ options = ['--body']
+
+ def execute(self, args):
+ mgr = TicketManager(self.client)
+
+ ticket_id = resolve_id(
+ mgr.resolve_ids, args.get('<identifier>'), 'ticket')
+
+ body = args.get('--body')
+ if (body is None):
@sudorandom
sudorandom Mar 6, 2014 Contributor

Parens are not needed and not preferred in this case for Pythonic code. :)

@sudorandom sudorandom commented on an outdated diff Mar 6, 2014
SoftLayer/CLI/modules/ticket.py
+ mgr.update_ticket(t_id=ticket_id, body=body)
+ return "Ticket Updated!"
+
+
+class TicketsSummary(CLIRunnable):
+ """
+ usage: sl ticket summary
+
+ Give summary info about tickets
+
+ """
+ action = 'summary'
+
+ def execute(self, args):
+ account = self.client['Account']
+ mask = 'mask[openTicketCount, closedTicketCount, '\
@sudorandom
sudorandom Mar 6, 2014 Contributor

Here's an alternative to backslashes:

mask = ('mask[openTicketCount,closedTicketCount,'
        'openBillingTicketCount,openOtherTicketCount,'
        'openSalesTicketCount,openSupportTicketCount,'
        'openAccountingTicketCount]')
@sudorandom sudorandom commented on an outdated diff Mar 6, 2014
SoftLayer/CLI/modules/ticket.py
+ nested.add_row(['Sales', accountObject['openSalesTicketCount']])
+ nested.add_row(['Support', accountObject['openSupportTicketCount']])
+ nested.add_row(['Other', accountObject['openOtherTicketCount']])
+ nested.add_row(['Total', accountObject['openTicketCount']])
+ t.add_row(['Open', nested])
+ t.add_row(['Closed', accountObject['closedTicketCount']])
+
+ return t
+
+
+class TicketUtils:
+ """
+ TicketUtils class that is a helper for common methods.
+ """
+
+ @staticmethod
@sudorandom
sudorandom Mar 6, 2014 Contributor

The @staticmethod is not required for this case. I recommend this be turned into a normal function, since this is used in other classes and relies on no implicit state.

This applies to every other static method in this file.

@sudorandom sudorandom commented on an outdated diff Mar 6, 2014
SoftLayer/CLI/modules/ticket.py
+ update = TicketUtils.wrap_string(result['updates'][i - 1]['entry'])
+ t.add_row(['Update %s' % (i), update])
+
+ return t
+
+ @staticmethod
+ def open_editor(beg_msg, ending_msg=None):
+ """
+
+ :param beg_msg: generic msg to be appended at the end of the file
+ :param ending_msg: placeholder msg to append at the end of the file,
+ like filesystem info, etc, not being used now
+ :returns: the content the user has entered
+
+ """
+ import tempfile
@sudorandom
sudorandom Mar 6, 2014 Contributor

Danger! Don't import things within a method! This is less of a performance concern than it is a style issue.

@sudorandom sudorandom commented on an outdated diff Mar 6, 2014
SoftLayer/CLI/modules/ticket.py
+ ticket_id = resolve_id(
+ mgr.resolve_ids, args.get('<identifier>'), 'ticket')
+
+ count = args.get('--updateCount')
+ return TicketUtils.get_ticket_results(mgr, ticket_id, int(count))
+
+
+class CreateTicket(CLIRunnable):
+ """
+ usage: sl ticket create --title=TITLE --subject=<subjectID> [options]
+
+ Create a support ticket.
+
+ Required:
+ --title=TITLE The title of the ticket
+ --subject=xxx The id of the subject to use for the ticket,
@sudorandom
sudorandom Mar 6, 2014 Contributor

Make sure you follow this style guide for these doc blocks: https://softlayer-python.readthedocs.org/en/latest/dev/cli.html#defining-a-module

Speficially the two-space rule and the alignment for continuing lines.

@sudorandom sudorandom commented on an outdated diff Mar 6, 2014
SoftLayer/managers/ticket.py
+ hardware=None, rootPassword=None, subject=None):
+ """ Create a new ticket
+
+ :param string title: title for the new ticket
+ :param string body: body for the new ticket
+ :param string hardware: id of the hardware to be assigned to the ticket
+ """
+
+ currentUser = self.account.getCurrentUser()
+ new_ticket = {
+ 'subjectId': subject,
+ 'contents': body,
+ 'assignedUserId': currentUser['id'],
+ 'title': title,
+ }
+ # if (hardware is None):
@sudorandom
sudorandom Mar 6, 2014 Contributor

Could this commented out code be cleaned up? :)

@anasouma
Contributor
anasouma commented Mar 6, 2014

Thanks for all your comments, you can tell im new to this. Will get those fixed soon.

@anasouma
Contributor
anasouma commented Mar 6, 2014

I "believe" I captured all your comments.

wissam

@sudorandom sudorandom commented on an outdated diff Mar 6, 2014
SoftLayer/CLI/modules/ticket.py
+ ticket_id = resolve_id(
+ mgr.resolve_ids, args.get('<identifier>'), 'ticket')
+
+ count = args.get('--updateCount')
+ return get_ticket_results(mgr, ticket_id, int(count))
+
+
+class CreateTicket(CLIRunnable):
+ """
+usage: sl ticket create --title=TITLE --subject=<subjectID> [options]
+
+Create a support ticket.
+
+Required:
+ --title=TITLE The title of the ticket
+ --subject=xxx The id of the subject to use for the ticket,
@sudorandom
sudorandom Mar 6, 2014 Contributor

Usually, tags are all capitalized and describe what's required. For this case I think ID fits.

usage: sl ticket create --title=TITLE --subject=ID [options]

and

--subject=ID   The id of the subject to use for the ticket,
@sudorandom sudorandom commented on an outdated diff Mar 6, 2014
SoftLayer/CLI/modules/ticket.py
+ create Create a new ticket
+ detail Output details about an ticket
+ list List tickets
+ update Update an existing ticket
+ subjects List the subject IDs that can be used for ticket creation
+ summary Give summary info about tickets
+"""
+# :license: MIT, see LICENSE for more details.
+
+import textwrap
+import tempfile
+import os
+from subprocess import call
+
+from SoftLayer import TicketManager
+from SoftLayer.CLI import CLIRunnable, Table, \
@sudorandom
sudorandom Mar 6, 2014 Contributor

I'm getting down to nit-picks now. This would fit better with the style of other modules by doing this:

from SoftLayer.CLI import (CLIRunnable, Table, resolve_id, NestedDict,
                           KeyValueTable)
@sudorandom sudorandom commented on an outdated diff Mar 6, 2014
SoftLayer/managers/ticket.py
+from SoftLayer.utils import IdentifierMixin
+
+
+class TicketManager(IdentifierMixin, object):
+ """
+ Manages account Tickets
+
+ :param SoftLayer.API.Client client: an API client instance
+ """
+
+ def __init__(self, client):
+ self.client = client
+ self.account = self.client['Account']
+ self.ticket = self.client['Ticket']
+
+ def list_tickets(self, openStatus=True, closedStatus=True, userId=None):
@sudorandom
sudorandom Mar 6, 2014 Contributor

Camel-cased for normal variables and arguments names are against PEP-8. these should be open_status, closed_status and user_id

@sudorandom sudorandom commented on an outdated diff Mar 6, 2014
SoftLayer/managers/ticket.py
+ """
+ mask = ('mask[id, title, assignedUser[firstName, lastName],'
+ 'createDate,lastEditDate,accountId]')
+
+ call = 'getTickets'
+ if not all([openStatus, closedStatus]):
+ if openStatus:
+ call = 'getOpenTickets'
+ elif closedStatus:
+ call = 'getClosedTickets'
+
+ func = getattr(self.account, call)
+ return func(mask=mask)
+
+ def list_subjects(self):
+ """ List all tickets
@sudorandom
sudorandom Mar 6, 2014 Contributor

Go ahead and close the string and keep this on one line. :)

@sudorandom sudorandom commented on an outdated diff Mar 6, 2014
SoftLayer/managers/ticket.py
+ :param string title: title for the new ticket
+ :param string body: body for the new ticket
+ :param string hardware: id of the hardware to be assigned to the ticket
+ """
+
+ currentUser = self.account.getCurrentUser()
+ new_ticket = {
+ 'subjectId': subject,
+ 'contents': body,
+ 'assignedUserId': currentUser['id'],
+ 'title': title,
+ }
+ created_ticket = self.ticket.createStandardTicket(new_ticket, body)
+ return created_ticket
+
+ def update_ticket(self, t_id=None, body=None):
@sudorandom
sudorandom Mar 6, 2014 Contributor

t_id should be ticket_id for clarity

@sudorandom
Contributor

@anasouma this is looking really great. After addressing my last round of comments I think it'll be good to pull.

@sudorandom sudorandom commented on an outdated diff Mar 6, 2014
SoftLayer/managers/ticket.py
+
+ :param integer id: the ticket ID
+ :returns: A dictionary containing a large amount of information about
+ the specified ticket.
+
+ """
+ mask = ('mask[id, title, assignedUser[firstName, lastName],'
+ 'createDate,lastEditDate,updates[entry],updateCount]')
+ return self.ticket.getObject(id=ticket_id, mask=mask)
+
+ def create_ticket(self, title=None, body=None, subject=None):
+ """ Create a new ticket
+
+ :param string title: title for the new ticket
+ :param string body: body for the new ticket
+ :param string hardware: id of the hardware to be assigned to the ticket
@sudorandom
sudorandom Mar 6, 2014 Contributor

this should match the parameter list. It's subject, and I'm guessing this could apply to CCI or hardware? I'm unsure.

@anasouma
Contributor
anasouma commented Mar 6, 2014

Thanks for your comments, I made all the changes you requested.

@sudorandom
Contributor

LGTM, +1; one more person (@beittenc or @jasonjohnson)? will need to +1 this before merging according to the process we have.

@briancline
Member

+1, looks good to me as well. Thanks for all the work on this module.

@briancline briancline merged commit d8e4d7d into softlayer:master Mar 6, 2014

1 check passed

default The Travis CI build passed
Details
@anasouma anasouma deleted the anasouma:TicketingSupport branch Mar 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment