Repository: ucfopen/canvasapi

Issue:
Course.reorder_pinned_topics cannot be called correctly
In `canvasapi` 0.6.0, the `Course.reorder_pinned_topics` method has two defects that prevent it from being called correctly:

1. When called with a list of discussion topic ids, this method performs a `POST` call with the topic ids given as repeated `order` parameters. For example, an order list of `[1, 2, 3]` turns into a `POST` request body of `order=1,order=2,order=3`. However, the [documentation for the corresponding Canvas API method](https://canvas.instructure.com/doc/api/discussion_topics.html#method.discussion_topics.reorder) indicates that the ordering should actually be given as a single comma-delimited parameter: `order=1,2,3`.

1. To work around the problem described above, one might consider turning the id list into a comma-delimited string before making the `Course.reorder_pinned_topics` call. For example, one might pass the `order` parameter to this call as the string `"1,2,3"` instead of the list `[1, 2, 3]`. However, the implementation of `Course.reorder_pinned_topics` uses `isinstance(order, list)` to check whether `order` is a list or not. If the argument is not a list, this method raises `ValueError("Param order needs to be string or a list.")`. Although the error message explains that either a string or a list would be accepted, the code in question really allows only a list.

Taken together, these effectively prevent one from using `canvasapi` to reorder pinned discussions.

Discussion:

Patch:
diff --git a/canvasapi/course.py b/canvasapi/course.py
index 7cd4f40..d12d049 100644
--- a/canvasapi/course.py
+++ b/canvasapi/course.py
@@ -1,7 +1,7 @@
 from __future__ import absolute_import, division, print_function, unicode_literals
 from warnings import warn
 
-from six import python_2_unicode_compatible
+from six import python_2_unicode_compatible, text_type
 
 from canvasapi.canvas_object import CanvasObject
 from canvasapi.discussion_topic import DiscussionTopic
@@ -899,15 +899,19 @@ class Course(CanvasObject):
         <https://canvas.instructure.com/doc/api/discussion_topics.html#method.discussion_topics.reorder>`_
 
         :param order: The ids of the pinned discussion topics in the desired order.
-            e.g. [104, 102, 103]
-        :type order: list
+            e.g. [104, 102, 103], (104, 102, 103), or "104,102,103"
+        :type order: list, tuple, or string
 
         :rtype: :class:`canvasapi.paginated_list.PaginatedList` of
             :class:`canvasapi.discussion_topic.DiscussionTopic`
         """
+        # Convert list or tuple to comma-separated string
+        if isinstance(order, (list, tuple)):
+            order = ",".join([text_type(topic_id) for topic_id in order])
 
-        if not isinstance(order, list):
-            raise ValueError("Param order needs to be string or a list.")
+        # Check if is a string with commas
+        if not isinstance(order, text_type) or "," not in order:
+            raise ValueError("Param `order` must be a list, tuple, or string.")
 
         response = self._requester.request(
             'POST',
diff --git a/canvasapi/group.py b/canvasapi/group.py
index c6336f0..7376731 100644
--- a/canvasapi/group.py
+++ b/canvasapi/group.py
@@ -1,6 +1,6 @@
 from __future__ import absolute_import, division, print_function, unicode_literals
 
-from six import python_2_unicode_compatible
+from six import python_2_unicode_compatible, text_type
 
 from canvasapi.canvas_object import CanvasObject
 from canvasapi.discussion_topic import DiscussionTopic
@@ -474,8 +474,13 @@ class Group(CanvasObject):
         :rtype: :class:`canvasapi.paginated_list.PaginatedList` of
             :class:`canvasapi.discussion_topic.DiscussionTopic`
         """
-        if not isinstance(order, list):
-            raise ValueError("Param order needs to be string or a list.")
+        # Convert list or tuple to comma-separated string
+        if isinstance(order, (list, tuple)):
+            order = ",".join([text_type(topic_id) for topic_id in order])
+
+        # Check if is a string with commas
+        if not isinstance(order, text_type) or "," not in order:
+            raise ValueError("Param `order` must be a list, tuple, or string.")
 
         response = self._requester.request(
             'POST',


Test patch:
diff --git a/tests/fixtures/course.json b/tests/fixtures/course.json
index 7ee017d..0226b68 100644
--- a/tests/fixtures/course.json
+++ b/tests/fixtures/course.json
@@ -1003,15 +1003,6 @@
 		},
 		"status_code": 200
 	},
-	"reorder_pinned_topics_no_list": {
-		"method": "POST",
-		"endpoint": "courses/1/discussion_topics/reorder",
-		"data": {
-			"reorder": true,
-			"order": "1, 2, 3"
-		},
-		"status_code": 200
-	},
 	"get_course_level_participation_data": {
 		"method": "GET",
 		"endpoint": "courses/1/analytics/activity",
diff --git a/tests/fixtures/group.json b/tests/fixtures/group.json
index 6e542af..fbecbf8 100644
--- a/tests/fixtures/group.json
+++ b/tests/fixtures/group.json
@@ -679,16 +679,7 @@
 		},
 		"status_code": 200
 	},
-	"reorder_pinned_topics_no_list": {
-		"method": "POST",
-		"endpoint": "groups/1/discussion_topics/reorder",
-		"data": {
-			"reorder": true,
-			"order": "1, 2, 3"
-		},
-		"status_code": 200
-	},
-  "delete_external_feed": {
+	"delete_external_feed": {
 		"method": "DELETE",
 		"endpoint": "groups/1/external_feeds/1",
 		"data": {
diff --git a/tests/test_course.py b/tests/test_course.py
index 367d304..ad15005 100644
--- a/tests/test_course.py
+++ b/tests/test_course.py
@@ -3,8 +3,10 @@ import unittest
 import uuid
 import warnings
 
+import requests
 import requests_mock
 from six import text_type
+from six.moves.urllib.parse import quote
 
 from canvasapi import Canvas
 from canvasapi.assignment import Assignment, AssignmentGroup
@@ -491,18 +493,37 @@ class TestCourse(unittest.TestCase):
 
     # reorder_pinned_topics()
     def test_reorder_pinned_topics(self, m):
-        register_uris({'course': ['reorder_pinned_topics']}, m)
+        # Custom matcher to test that params are set correctly
+        def custom_matcher(request):
+            match_text = '1,2,3'
+            if request.text == 'order={}'.format(quote(match_text)):
+                resp = requests.Response()
+                resp._content = b'{"reorder": true, "order": [1, 2, 3]}'
+                resp.status_code = 200
+                return resp
+
+        m.add_matcher(custom_matcher)
 
         order = [1, 2, 3]
+        discussions = self.course.reorder_pinned_topics(order=order)
+        self.assertTrue(discussions)
 
+    def test_reorder_pinned_topics_tuple(self, m):
+        register_uris({'course': ['reorder_pinned_topics']}, m)
+
+        order = (1, 2, 3)
         discussions = self.course.reorder_pinned_topics(order=order)
         self.assertTrue(discussions)
 
-    def test_reorder_pinned_topics_no_list(self, m):
-        register_uris({'course': ['reorder_pinned_topics_no_list']}, m)
+    def test_reorder_pinned_topics_comma_separated_string(self, m):
+        register_uris({'course': ['reorder_pinned_topics']}, m)
 
-        order = "1, 2, 3"
+        order = "1,2,3"
+        discussions = self.course.reorder_pinned_topics(order=order)
+        self.assertTrue(discussions)
 
+    def test_reorder_pinned_topics_invalid_input(self, m):
+        order = "invalid string"
         with self.assertRaises(ValueError):
             self.course.reorder_pinned_topics(order=order)
 
diff --git a/tests/test_group.py b/tests/test_group.py
index f32a529..0ab3687 100644
--- a/tests/test_group.py
+++ b/tests/test_group.py
@@ -2,7 +2,9 @@ from __future__ import absolute_import, division, print_function, unicode_litera
 import unittest
 import uuid
 
+import requests
 import requests_mock
+from six.moves.urllib.parse import quote
 
 from canvasapi import Canvas
 from canvasapi.group import Group, GroupMembership, GroupCategory
@@ -249,18 +251,37 @@ class TestGroup(unittest.TestCase):
 
     # reorder_pinned_topics()
     def test_reorder_pinned_topics(self, m):
-        register_uris({'group': ['reorder_pinned_topics']}, m)
+        # Custom matcher to test that params are set correctly
+        def custom_matcher(request):
+            match_text = '1,2,3'
+            if request.text == 'order={}'.format(quote(match_text)):
+                resp = requests.Response()
+                resp._content = b'{"reorder": true, "order": [1, 2, 3]}'
+                resp.status_code = 200
+                return resp
+
+        m.add_matcher(custom_matcher)
 
         order = [1, 2, 3]
+        discussions = self.group.reorder_pinned_topics(order=order)
+        self.assertTrue(discussions)
 
+    def test_reorder_pinned_topics_tuple(self, m):
+        register_uris({'group': ['reorder_pinned_topics']}, m)
+
+        order = (1, 2, 3)
         discussions = self.group.reorder_pinned_topics(order=order)
         self.assertTrue(discussions)
 
-    def test_reorder_pinned_topics_no_list(self, m):
-        register_uris({'group': ['reorder_pinned_topics_no_list']}, m)
+    def test_reorder_pinned_topics_comma_separated_string(self, m):
+        register_uris({'group': ['reorder_pinned_topics']}, m)
 
-        order = "1, 2, 3"
+        order = "1,2,3"
+        discussions = self.group.reorder_pinned_topics(order=order)
+        self.assertTrue(discussions)
 
+    def test_reorder_pinned_topics_invalid_input(self, m):
+        order = "invalid string"
         with self.assertRaises(ValueError):
             self.group.reorder_pinned_topics(order=order)