Repository: ucfopen/canvasapi

Issue:
fetching announcements from a `Canvas` object doesn't assign the `course_id`
# Describe the bug

Retrieving announcements from Canvas happen at the level of the `Canvas` object, not course level.  Getting an announcement and trying to call `a.update()` fails generating this error:
```
ValueError: Discussion Topic does not have a course_id or group_id
```

A workaround is to manually assign the course_id into the announcement, and then update.  The following succeeds:
```
b.course_id = 123456
b.update()
```


 

# To Reproduce

Steps to reproduce the behavior:

```
import canvasapi
canvas = canvasapi.Canvas(API_URL, API_KEY)

course_id = 123456
announcements = canvas.get_announcements(context_codes=['course_{}'.format(course_id)])
a = announcements[0]
a.update()
```

This is the complete error:
```
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-15-667b0a4b45f2> in <module>
      2 announcements = canvas.get_announcements(context_codes=['course_{}'.format(course_id)])
      3 a = announcements[0]
----> 4 a.update()

~/opt/anaconda3/lib/python3.7/site-packages/canvasapi/discussion_topic.py in update(self, **kwargs)
    309             "PUT",
    310             "{}s/{}/discussion_topics/{}".format(
--> 311                 self._parent_type, self._parent_id, self.id
    312             ),
    313             _kwargs=combine_kwargs(**kwargs),

~/opt/anaconda3/lib/python3.7/site-packages/canvasapi/canvas_object.py in __getattribute__(self, name)
     15 
     16     def __getattribute__(self, name):
---> 17         return super(CanvasObject, self).__getattribute__(name)
     18 
     19     def __init__(self, requester, attributes):

~/opt/anaconda3/lib/python3.7/site-packages/canvasapi/discussion_topic.py in _parent_type(self)
     34             return "group"
     35         else:
---> 36             raise ValueError("Discussion Topic does not have a course_id or group_id")
     37 
     38     def delete(self, **kwargs):

ValueError: Discussion Topic does not have a course_id or group_id
```
# Expected behavior

I expect the line
```
a.update()
```
to run cleanly,  and without error.

# Environment information

<!--(please complete the following information)-->

- Python 3.7.4
- CanvasAPI version 2.0.0

# Additional context

My ultimate goal is to be able to programmatically adjust `delayed_post_at` for announcements.


Discussion:
coveralls: 
[![Coverage Status](https://coveralls.io/builds/33866349/badge)](https://coveralls.io/builds/33866349)

Coverage decreased (-0.09%) to 99.914% when pulling **5b89021192ee382c2bfd9e3f9730913bab605ebf on bennettscience:issue/420-fetch-announcements** into **71b2d65a88ae361962860fd2fcd81c8904a542fb on ucfopen:develop**.

bennettscience: OK, here's another question:

This call requires the `context_codes[]` parameter. Calling `canvas.get_announcements()` fails if the user hasn't defined a `course_12345` string. Is it better to document this as a required `**kwarg` or add a required param to the method? There are other optional arguments that can be passed, so I'm tempted to make it a required parameter for the call, but I'll do whatever you think is best.


```python
def get_announcements(self, course_id, **kwargs):
        """
        List announcements.

        :calls: `GET /api/v1/announcements \
        <https://canvas.instructure.com/doc/api/announcements.html#method.announcements_api.index>`_

        :param course_id: Course ID to request announcements from.

        :rtype: :class:`canvasapi.paginated_list.PaginatedList` of
                :class:`canvasapi.discussion_topic.DiscussionTopic`
        """
        from canvasapi.discussion_topic import DiscussionTopic

        return PaginatedList(
            DiscussionTopic,
            self.__requester,
            "GET",
            "announcements",
            context_codes=[f'course_{course_id}'],
            _kwargs=combine_kwargs(**kwargs),
        )
```

bennettscience: Not sure why the build failed...is there any way to get a log or report of what happened?
Thetwam: @bennettscience looks like the `black` check failed:

https://travis-ci.org/github/ucfopen/canvasapi/builds/732295563
Thetwam: > This call requires the `context_codes[]` parameter. Calling `canvas.get_announcements()` fails if the user hasn't defined a `course_12345` string. Is it better to document this as a required `**kwarg` or add a required param to the method? There are other optional arguments that can be passed, so I'm tempted to make it a required parameter for the call, but I'll do whatever you think is best.

In general, we try to make required parameters into required arguments for the function (not sure how we missed this argument previously! 😅).

Since `context_codes[]` is required by Canvas, I'd say add a required positional parameter called `context_codes` of type `list`.

However, this would require the user to build their own context string which, while not difficult, is mildly annoying. I like your approach of taking in a list of course IDs instead (especially if we leverage `util.obj_or_id()` and accept `Course` objects as well).

I'm honestly torn on which solution is better. Just making `context_codes` required with no additional processing would be more consistent with what we usually do, but we'd lose out on extra usability. Alternatively, adding the extra functionality would be super useful, but potentially confusing for those users who expect to be able to pass a list of `course_xxxxx` strings. At the risk of overcomplicating things, maybe there's a hybrid option where we can have both work?

Thoughts? @jessemcbride @bennettscience 
jessemcbride: > I'm honestly torn on which solution is better. Just making `context_codes` required with no additional processing would be more consistent with what we usually do, but we'd lose out on extra usability. Alternatively, adding the extra functionality would be super useful, but potentially confusing for those users who expect to be able to pass a list of `course_xxxxx` strings. At the risk of overcomplicating things, maybe there's a hybrid option where we can have both work?

I think a hybrid approach would be alright here in the spirit of more usability. If Canvas ever changes the endpoint (to allow other context code types, for example), we can rethink the approach, but for now, I think this works:

* Take in a list of one or more `courses` as a required argument.
* Check to see if courses is a list of context codes (by seeing if the first element in `courses` is a string containing `course_`). If it is, skip the `obj_or_id` branch.
* If it isn't, use `obj_or_id` to enable users to pass a Course object or an id for convenience.
* Format the list of context_codes appropriately.
* Send off the request.


bennettscience: Sorry this PR was such a saga. I need to add automatic formatting on save...

Patch:
diff --git a/.gitignore b/.gitignore
index f6b23d7..c2f3444 100644
--- a/.gitignore
+++ b/.gitignore
@@ -15,9 +15,10 @@
 /docs/_build/
 /env/
 /env3/
+venv/
 /htmlcov/
 \#*#
 .vscode/settings.json
 setup.sh
 .python-version
-.*.swp
+.*.swp
\ No newline at end of file
diff --git a/canvasapi/canvas.py b/canvasapi/canvas.py
index 9ffcf6a..dc8fae6 100644
--- a/canvasapi/canvas.py
+++ b/canvasapi/canvas.py
@@ -437,23 +437,39 @@ class Canvas(object):
         )
         return response.json()
 
-    def get_announcements(self, **kwargs):
+    def get_announcements(self, courses, **kwargs):
         """
         List announcements.
 
         :calls: `GET /api/v1/announcements \
         <https://canvas.instructure.com/doc/api/announcements.html#method.announcements_api.index>`_
 
+        :param courses: Course ID(s) or <Course> objects to request announcements from.
+        :type courses: list
+
         :rtype: :class:`canvasapi.paginated_list.PaginatedList` of
                 :class:`canvasapi.discussion_topic.DiscussionTopic`
         """
         from canvasapi.discussion_topic import DiscussionTopic
 
+        if type(courses) is not list or len(courses) == 0:
+            raise RequiredFieldMissing("Course IDs need to be passed as a list")
+
+        # The type of object in `courses` is taken care of by obj_or_id, extracting the couse
+        # ID from a <Course> object or by returning plain strings.
+        course_ids = [
+            obj_or_id(course_id, "course_id", (Course,)) for course_id in courses
+        ]
+
+        # Set the **kwargs object vaue so it can be combined with others passed by the user.
+        kwargs["context_codes"] = [f"course_{course_id}" for course_id in course_ids]
+
         return PaginatedList(
             DiscussionTopic,
             self.__requester,
             "GET",
             "announcements",
+            context_codes=course_ids,
             _kwargs=combine_kwargs(**kwargs),
         )
 
diff --git a/canvasapi/discussion_topic.py b/canvasapi/discussion_topic.py
index 2a7dd50..9fa5dd2 100644
--- a/canvasapi/discussion_topic.py
+++ b/canvasapi/discussion_topic.py
@@ -18,6 +18,13 @@ class DiscussionTopic(CanvasObject):
             return self.course_id
         elif hasattr(self, "group_id"):
             return self.group_id
+        elif hasattr(self, "context_code"):
+            if self.context_code.startswith("course_"):
+                self.course_id = self.context_code.split("_")[1]
+                return self.course_id
+            elif self.context_code.startswith("group_"):
+                self.group_id = self.context_code.split("_")[1]
+                return self.group_id
         else:
             raise ValueError("Discussion Topic does not have a course_id or group_id")
 
@@ -32,6 +39,11 @@ class DiscussionTopic(CanvasObject):
             return "course"
         elif hasattr(self, "group_id"):
             return "group"
+        elif hasattr(self, "context_code"):
+            if self.context_code.startswith("course"):
+                return "course"
+            elif self.context_code.startswith("group"):
+                return "group"
         else:
             raise ValueError("Discussion Topic does not have a course_id or group_id")
 


Test patch:
diff --git a/tests/fixtures/announcements.json b/tests/fixtures/announcements.json
index b637050..684540d 100755
--- a/tests/fixtures/announcements.json
+++ b/tests/fixtures/announcements.json
@@ -10,7 +10,8 @@
           "display_name": "John Doe"
         },
         "message": "Announcement Message #1",
-        "subscription_hold": "topic_is_announcement"
+        "subscription_hold": "topic_is_announcement",
+        "context_code": "course_1"
       },
       {
         "id": 2,
@@ -20,7 +21,30 @@
           "display_name": "John Smith"
         },
         "message": "Announcement Message #2",
-        "subscription_hold": "topic_is_announcement"
+        "subscription_hold": "topic_is_announcement",
+        "context_code": "course_1"
+      },
+      {
+        "id": 3,
+        "title": "Annoucement 3",
+        "author": {
+          "id": 3,
+          "display_name": "John Wick"
+        },
+        "message": "Announcement Message #3",
+        "subscription_hold": "topic_is_announcement",
+        "context_code": "group_1"
+      },
+      {
+        "id": 4,
+        "title": "Annoucement 4",
+        "author": {
+          "id": 3,
+          "display_name": "Jon Voight"
+        },
+        "message": "Announcement Message #3",
+        "subscription_hold": "topic_is_announcement",
+        "context_code": "course_2"
       }
     ],
     "status_code": 200
diff --git a/tests/test_canvas.py b/tests/test_canvas.py
index d27aa03..d99bc47 100644
--- a/tests/test_canvas.py
+++ b/tests/test_canvas.py
@@ -702,13 +702,52 @@ class TestCanvas(unittest.TestCase):
         self.assertEqual(progress.id, 1)
 
     # get_announcements()
-    def test_get_announcements(self, m):
+    def test_get_single_course_announcements(self, m):
         register_uris({"announcements": ["list_announcements"]}, m)
-        announcements = self.canvas.get_announcements()
+        announcements = self.canvas.get_announcements([1])
         announcement_list = [announcement for announcement in announcements]
+
         self.assertIsInstance(announcements, PaginatedList)
         self.assertIsInstance(announcement_list[0], DiscussionTopic)
-        self.assertEqual(len(announcement_list), 2)
+        self.assertEqual(len(announcement_list), 4)
+
+    def test_get_course_announcements_from_object(self, m):
+        register_uris(
+            {"course": ["get_by_id"], "announcements": ["list_announcements"]}, m
+        )
+        course = self.canvas.get_course(1)
+        announcements = self.canvas.get_announcements([course])
+
+        self.assertIsInstance(announcements, PaginatedList)
+
+    def test_get_course_announcements_from_mixed_list(self, m):
+        register_uris(
+            {"course": ["get_by_id"], "announcements": ["list_announcements"]}, m
+        )
+        course = self.canvas.get_course(1)
+        course_ids = [course, 2]
+        announcements = self.canvas.get_announcements(course_ids)
+
+        self.assertIsInstance(announcements, PaginatedList)
+
+    def test_get_announcements_fail(self, m):
+        with self.assertRaises(RequiredFieldMissing):
+            self.canvas.get_announcements([])
+        with self.assertRaises(RequiredFieldMissing):
+            self.canvas.get_announcements(1)
+
+    def test_multiple_course_announcements(self, m):
+        register_uris({"announcements": ["list_announcements"]}, m)
+        announcements = self.canvas.get_announcements([1, 2])
+        announcement_list = [announcement for announcement in announcements]
+
+        self.assertEqual(announcement_list[1].context_code, "course_1")
+        self.assertEqual(announcement_list[1]._parent_type, "course")
+        self.assertEqual(announcement_list[1]._parent_id, "1")
+
+        self.assertEqual(announcement_list[2].context_code, "group_1")
+        self.assertEqual(announcement_list[2]._parent_type, "group")
+        self.assertEqual(announcement_list[2]._parent_id, "1")
 
     # get_epub_exports()
     def test_get_epub_exports(self, m):
diff --git a/tests/test_discussion_topic.py b/tests/test_discussion_topic.py
index 7c1dea4..1371505 100644
--- a/tests/test_discussion_topic.py
+++ b/tests/test_discussion_topic.py
@@ -47,7 +47,6 @@ class TestDiscussionTopic(unittest.TestCase):
         discussion = self.discussion_topic.update()
         self.assertIsInstance(discussion, DiscussionTopic)
         self.assertTrue(hasattr(discussion, "course_id"))
-        self.assertEqual(discussion.course_id, 1)
 
     # post_entry()
     def test_post_entry(self, m):