Enhance the client to support uploading an attachment to a ticket by joshuakwan · Pull Request #735 · softlayer/softlayer-python · GitHub
Skip to content

Enhance the client to support uploading an attachment to a ticket#735

Merged
sudorandom merged 5 commits into
softlayer:masterfrom
joshuakwan:master
Jul 28, 2016
Merged

Enhance the client to support uploading an attachment to a ticket#735
sudorandom merged 5 commits into
softlayer:masterfrom
joshuakwan:master

Conversation

@joshuakwan

Copy link
Copy Markdown
Contributor

Please review, comment and merge

@coveralls

Copy link
Copy Markdown

Comment thread SoftLayer/CLI/ticket/upload.py Outdated
if name is None:
name = os.path.basename(file)

bytes = None

@camporter camporter Jun 27, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bytes is a type, so this is shadowing it. You'll want to change this to file_bytes or something similar.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the comment, will fix

@coveralls

coveralls commented Jul 6, 2016

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.2%) to 79.642% when pulling 8b0be92 on joshuakwan:master into 62515bd on softlayer:master.

@joshuakwan

Copy link
Copy Markdown
Contributor Author

Any update on this? Thanks.

@sudorandom

Copy link
Copy Markdown
Contributor

It looks like these comments haven't been addressed yet: #735 (comment)

@joshuakwan

Copy link
Copy Markdown
Contributor Author

Code updated upon the comments. But for ticket_id I am just following all the other methods to have it as ticket_id=None.

@coveralls

coveralls commented Jul 25, 2016

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.1%) to 79.652% when pulling 2bb6669 on joshuakwan:master into 62515bd on softlayer:master.

@camporter

Copy link
Copy Markdown
Member

LGTM, though it'd be nice to see the upload stuff have tests. Here's some that should work:

diff --git a/SoftLayer/fixtures/SoftLayer_Ticket.py b/SoftLayer/fixtures/SoftLayer_Ticket.py
index c85deda..ea230f9 100644
--- a/SoftLayer/fixtures/SoftLayer_Ticket.py
+++ b/SoftLayer/fixtures/SoftLayer_Ticket.py
@@ -48,5 +48,17 @@ addAttachedVirtualGuest = {
     "ticketId": 100
 }

+addAttachedFile = {
+    "id": 123,
+    "fileName": "a_file_name",
+    "fileSize": "100",
+    "ticketId": 100,
+    "updateId": 200,
+    "createDate": "2013-08-01T14:14:04-07:00",
+    "modifyDate": "2013-08-01T14:14:04-07:00",
+    "uploaderType": "USER",
+    "uploaderId": "300"
+}
+
 removeAttachedHardware = True
 removeAttachedVirtualGuest = True
diff --git a/tests/CLI/modules/ticket_tests.py b/tests/CLI/modules/ticket_tests.py
index 69674c6..c43f46e 100644
--- a/tests/CLI/modules/ticket_tests.py
+++ b/tests/CLI/modules/ticket_tests.py
@@ -167,3 +167,40 @@ class TicketTests(testing.TestCase):
                                 'removeAttachedVirtualGuest',
                                 args=(100,),
                                 identifier=1)
+
+    def test_ticket_upload_no_path(self):
+        result = self.run_command(['ticket', 'upload', '1'])
+
+        self.assertEqual(result.exit_code, 2)
+        self.assertIsInstance(result.exception, exceptions.ArgumentError)
+
+    def test_ticket_upload_invalid_path(self):
+        result = self.run_command(['ticket', 'upload', '1',
+                                   '--path=tests/resources/nonexistent_file',
+                                   '--name=a_file_name'])
+
+        self.assertEqual(result.exit_code, 2)
+        self.assertIsInstance(result.exception, exceptions.ArgumentError)
+
+    def test_ticket_upload_no_name(self):
+        result = self.run_command(['ticket', 'upload', '1',
+                                   '--path=tests/resources/ticket_upload'])
+
+        self.assert_no_fail(result)
+        self.assert_called_with('SoftLayer_Ticket',
+                                'addAttachedFile',
+                                args=({"filename": "ticket_upload",
+                                       "data": b"ticket upload data"},),
+                                identifier=1)
+
+    def test_ticket_upload(self):
+        result = self.run_command(['ticket', 'upload', '1',
+                                   '--path=tests/resources/ticket_upload',
+                                   '--name=a_file_name'])
+
+        self.assert_no_fail(result)
+        self.assert_called_with('SoftLayer_Ticket',
+                                'addAttachedFile',
+                                args=({"filename": "a_file_name",
+                                       "data": b"ticket upload data"},),
+                                identifier=1)
diff --git a/tests/resources/ticket_upload b/tests/resources/ticket_upload
new file mode 100644
index 0000000..d2e3a7c
--- /dev/null
+++ b/tests/resources/ticket_upload
@@ -0,0 +1 @@
+ticket upload data
\ No newline at end of file

Someone might have a differing opinion about where to put the test upload file though.

@coveralls

coveralls commented Jul 27, 2016

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.2%) to 79.631% when pulling c296a5f on joshuakwan:master into 62515bd on softlayer:master.

@joshuakwan

Copy link
Copy Markdown
Contributor Author

@sudorandom sudorandom merged commit 4932cac into softlayer:master Jul 28, 2016
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.

4 participants