Repository: ucfopen/canvasapi

Issue:
Enroll User Incorrectly Requires Enrollment Type Argument
# Describe the bug

`Course.enroll_user()` incorrectly requires `enrollment[type]` to be passed (as the positional arg `enrollment_type`).

Although the [Canvas docs for Enroll a User](https://canvas.instructure.com/doc/api/enrollments.html#method.enrollments_api.create) list `enrollment[type]` as "required", the detailed description says this:

> If no value is given, the type will be inferred by `enrollment` if supplied, otherwise 'StudentEnrollment' will be used.

This implies to me that `enrollment[type]` could be empty and valid if another argument, such as `enrollment[role_id]`, were passed.

# Expected behavior

Although it's possible to enroll a user with a custom `role_id` by filling in any valid `enrollment_type`, it shouldn't be a required field at the function level.

A user should be able to define `enrollment[role_id]` without having to define `enrollment_type`/`enrollment[type]`.

# Environment information

- Python version (`python --version`) any
- CanvasAPI version (`pip show canvasapi`) 2.0.0

# Additional context

Removing `enrollment_type` as a positional argument is required to fix this, but is also a breaking change. We might be able to get away with making it as an argument that defaults to `None`, then doing some extra processing afterwards. While this should preserve compatibility with previous versions, I also don't want to keep that "fix" for very long. I recommend making that change and marking it as deprecated, to be removed in a future version.


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

Coverage remained the same at 100.0% when pulling **1fc81c1796a5ecf8f7525243e1dd72ef1081997c on Thetwam:issue/432-enroll-user-enrollment-type-arg** into **9175ff4b43d6bdfb0149fb63c320e41e72b69314 on ucfopen:develop**.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f9fc32d..fe7584a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -16,6 +16,10 @@
 - Course TODO items (Thanks, [@onomou](https://github.com/onomou))
 - Create observer pairing code (Thanks, [@bennettscience](https://github.com/bennettscience))
 
+### Deprecation Warnings
+
+- The `enrollment_type` argument on `Course.enroll_user` is now deprecated. Pass this information to `enrollment[type]` as a keyword argument instead. e.g. `enroll_user(enrollment={'type': 'StudentEnrollment'})`
+
 ## [2.0.0] - 2020-08-14
 
 ### General
diff --git a/canvasapi/course.py b/canvasapi/course.py
index 46fecde..dbb6f04 100644
--- a/canvasapi/course.py
+++ b/canvasapi/course.py
@@ -1,3 +1,5 @@
+import warnings
+
 from canvasapi.assignment import Assignment, AssignmentGroup
 from canvasapi.blueprint import BlueprintSubscription
 from canvasapi.canvas_object import CanvasObject
@@ -641,7 +643,7 @@ class Course(CanvasObject):
 
         return response.status_code == 204
 
-    def enroll_user(self, user, enrollment_type, **kwargs):
+    def enroll_user(self, user, enrollment_type=None, **kwargs):
         """
         Create a new user enrollment for a course or a section.
 
@@ -651,14 +653,25 @@ class Course(CanvasObject):
         :param user: The object or ID of the user to enroll in this course.
         :type user: :class:`canvasapi.user.User` or int
         :param enrollment_type: The type of enrollment.
-        :type enrollment_type: str
+        :type enrollment_type: str, optional
         :rtype: :class:`canvasapi.enrollment.Enrollment`
         """
         from canvasapi.enrollment import Enrollment
         from canvasapi.user import User
 
         kwargs["enrollment[user_id]"] = obj_or_id(user, "user", (User,))
-        kwargs["enrollment[type]"] = enrollment_type
+
+        if enrollment_type:
+            warnings.warn(
+                (
+                    "The `enrollment_type` argument is deprecated and will be "
+                    "removed in a future version.\n"
+                    "Use `enrollment[type]` as a keyword argument instead. "
+                    "e.g. `enroll_user(enrollment={'type': 'StudentEnrollment'})`"
+                ),
+                DeprecationWarning,
+            )
+            kwargs["enrollment[type]"] = enrollment_type
 
         response = self._requester.request(
             "POST",


Test patch:
diff --git a/tests/test_course.py b/tests/test_course.py
index 898f515..96d5e23 100644
--- a/tests/test_course.py
+++ b/tests/test_course.py
@@ -1,6 +1,7 @@
 import unittest
 from urllib.parse import quote
 import uuid
+import warnings
 
 import requests
 import requests_mock
@@ -204,20 +205,52 @@ class TestCourse(unittest.TestCase):
         register_uris(requires, m)
 
         enrollment_type = "TeacherEnrollment"
-        user_by_id = self.canvas.get_user(1)
-        enrollment_by_id = self.course.enroll_user(user_by_id, enrollment_type)
+
+        # by user ID
+        enrollment_by_id = self.course.enroll_user(
+            1, enrollment={"type": enrollment_type}
+        )
 
         self.assertIsInstance(enrollment_by_id, Enrollment)
         self.assertTrue(hasattr(enrollment_by_id, "type"))
         self.assertEqual(enrollment_by_id.type, enrollment_type)
 
-        user_by_obj = self.canvas.get_user(self.user)
-        enrollment_by_obj = self.course.enroll_user(user_by_obj, enrollment_type)
+        # by user object
+        enrollment_by_obj = self.course.enroll_user(
+            self.user, enrollment={"type": enrollment_type}
+        )
 
         self.assertIsInstance(enrollment_by_obj, Enrollment)
         self.assertTrue(hasattr(enrollment_by_obj, "type"))
         self.assertEqual(enrollment_by_obj.type, enrollment_type)
 
+    def test_enroll_user_legacy(self, m):
+        warnings.simplefilter("always", DeprecationWarning)
+
+        requires = {"course": ["enroll_user"], "user": ["get_by_id"]}
+        register_uris(requires, m)
+
+        enrollment_type = "TeacherEnrollment"
+
+        with warnings.catch_warnings(record=True) as warning_list:
+            # by user ID
+            enrollment_by_id = self.course.enroll_user(1, enrollment_type)
+
+            self.assertIsInstance(enrollment_by_id, Enrollment)
+            self.assertTrue(hasattr(enrollment_by_id, "type"))
+            self.assertEqual(enrollment_by_id.type, enrollment_type)
+
+            # by user object
+            enrollment_by_obj = self.course.enroll_user(self.user, enrollment_type)
+
+            self.assertIsInstance(enrollment_by_obj, Enrollment)
+            self.assertTrue(hasattr(enrollment_by_obj, "type"))
+            self.assertEqual(enrollment_by_obj.type, enrollment_type)
+
+        self.assertEqual(len(warning_list), 2)
+        self.assertEqual(warning_list[0].category, DeprecationWarning)
+        self.assertEqual(warning_list[1].category, DeprecationWarning)
+
     # get_recent_students()
     def test_get_recent_students(self, m):
         recent = {"course": ["get_recent_students", "get_recent_students_p2"]}