Repository: ucfopen/canvasapi

Issue:
Deprecate /api/v1 from Canvas constructor
Currently we require `/api/v1/` to be appended to the end of an institution's Canvas URL when initializing the `Canvas` object.

We don't check if the URL is malformed in anyway, and as a result, sometimes unexpected behavior can present itself. We recently ran into this issue with one of our own applications where the base URL was set to `instructure.edu//api/v1/`.

We should implement a fix for this in two ways:

* Remove the `/api/version` requirement in the URL as we don't currently support targeting more than one API version
* Force URLs to be formatted properly (i.e. `*.someurl.com` with no trailing slash, `https://` or `http://` allowed) and manually append the `/api/v1/` string ourselves afterward

We should also warn the user that `/api/v1/` has been deprecated and is no longer required if they include it in their base URL.

Discussion:

Patch:
diff --git a/canvasapi/canvas.py b/canvasapi/canvas.py
index 687649c..13193ea 100644
--- a/canvasapi/canvas.py
+++ b/canvasapi/canvas.py
@@ -1,5 +1,7 @@
 from __future__ import absolute_import, division, print_function, unicode_literals
 
+import warnings
+
 from canvasapi.account import Account
 from canvasapi.course import Course
 from canvasapi.exceptions import RequiredFieldMissing
@@ -10,7 +12,10 @@ from canvasapi.paginated_list import PaginatedList
 from canvasapi.requester import Requester
 from canvasapi.section import Section
 from canvasapi.user import User
-from canvasapi.util import combine_kwargs, obj_or_id
+from canvasapi.util import combine_kwargs, get_institution_url, obj_or_id
+
+
+warnings.simplefilter('always', DeprecationWarning)
 
 
 class Canvas(object):
@@ -25,6 +30,16 @@ class Canvas(object):
         :param access_token: The API key to authenticate requests with.
         :type access_token: str
         """
+        new_url = get_institution_url(base_url)
+
+        if 'api/v1' in base_url:
+            warnings.warn(
+                "`base_url` no longer requires an API version be specified. "
+                "Rewriting `base_url` to {}".format(new_url),
+                DeprecationWarning
+            )
+        base_url = new_url + '/api/v1/'
+
         self.__requester = Requester(base_url, access_token)
 
     def create_account(self, **kwargs):
diff --git a/canvasapi/util.py b/canvasapi/util.py
index 88c469c..86d6ee4 100644
--- a/canvasapi/util.py
+++ b/canvasapi/util.py
@@ -122,3 +122,19 @@ def obj_or_id(parameter, param_name, object_types):
         obj_type_list = ",".join([obj_type.__name__ for obj_type in object_types])
         message = 'Parameter {} must be of type {} or int.'.format(param_name, obj_type_list)
         raise TypeError(message)
+
+
+def get_institution_url(base_url):
+    """
+    Trim '/api/v1' from a given root URL.
+
+    :param base_url: The base URL of the API.
+    :type base_url: str
+    :rtype: str
+    """
+    index = base_url.find('/api/v1')
+
+    if index != -1:
+        return base_url[0:index]
+
+    return base_url


Test patch:
diff --git a/tests/settings.py b/tests/settings.py
index 789a25c..e67c5f6 100644
--- a/tests/settings.py
+++ b/tests/settings.py
@@ -1,6 +1,7 @@
 from __future__ import absolute_import, division, print_function, unicode_literals
 
-BASE_URL = 'http://example.com/api/v1/'
+BASE_URL = 'http://example.com'
+BASE_URL_WITH_VERSION = 'http://example.com/api/v1/'
 API_KEY = '123'
 
 INVALID_ID = 9001
diff --git a/tests/test_canvas.py b/tests/test_canvas.py
index dc5ec34..3d75228 100644
--- a/tests/test_canvas.py
+++ b/tests/test_canvas.py
@@ -1,5 +1,7 @@
 from __future__ import absolute_import, division, print_function, unicode_literals
+
 import unittest
+import warnings
 from datetime import datetime
 
 import pytz
@@ -30,6 +32,12 @@ class TestCanvas(unittest.TestCase):
     def setUp(self):
         self.canvas = Canvas(settings.BASE_URL, settings.API_KEY)
 
+    # Canvas()
+    def test_init_deprecate_url_contains_version(self, m):
+        with warnings.catch_warnings(record=True) as w:
+            Canvas(settings.BASE_URL_WITH_VERSION, settings.API_KEY)
+            self.assertTrue(issubclass(w[0].category, DeprecationWarning))
+
     # create_account()
     def test_create_account(self, m):
         register_uris({'account': ['create']}, m)
diff --git a/tests/test_course.py b/tests/test_course.py
index 415f426..fb6e76c 100644
--- a/tests/test_course.py
+++ b/tests/test_course.py
@@ -176,14 +176,16 @@ class TestCourse(unittest.TestCase):
         register_uris({'course': ['upload', 'upload_final']}, m)
 
         filename = 'testfile_course_{}'.format(uuid.uuid4().hex)
-        with open(filename, 'w+') as file:
-            response = self.course.upload(file)
 
-        self.assertTrue(response[0])
-        self.assertIsInstance(response[1], dict)
-        self.assertIn('url', response[1])
+        try:
+            with open(filename, 'w+') as file:
+                response = self.course.upload(file)
 
-        cleanup_file(filename)
+            self.assertTrue(response[0])
+            self.assertIsInstance(response[1], dict)
+            self.assertIn('url', response[1])
+        finally:
+            cleanup_file(filename)
 
     # reset()
     def test_reset(self, m):
diff --git a/tests/test_group.py b/tests/test_group.py
index 526eb23..e948c44 100644
--- a/tests/test_group.py
+++ b/tests/test_group.py
@@ -152,13 +152,14 @@ class TestGroup(unittest.TestCase):
         register_uris({'group': ['upload', 'upload_final']}, m)
 
         filename = 'testfile_group_{}'.format(uuid.uuid4().hex)
-        with open(filename, 'w+') as file:
-            response = self.group.upload(file)
-        self.assertTrue(response[0])
-        self.assertIsInstance(response[1], dict)
-        self.assertIn('url', response[1])
-
-        cleanup_file(filename)
+        try:
+            with open(filename, 'w+') as file:
+                response = self.group.upload(file)
+            self.assertTrue(response[0])
+            self.assertIsInstance(response[1], dict)
+            self.assertIn('url', response[1])
+        finally:
+            cleanup_file(filename)
 
     # preview_processed_html()
     def test_preview_processed_html(self, m):
diff --git a/tests/test_submission.py b/tests/test_submission.py
index 8a083e0..2ccc29c 100644
--- a/tests/test_submission.py
+++ b/tests/test_submission.py
@@ -37,14 +37,16 @@ class TestSubmission(unittest.TestCase):
         register_uris({'submission': ['upload_comment', 'upload_comment_final']}, m)
 
         filename = 'testfile_submission_{}'.format(uuid.uuid4().hex)
-        with open(filename, 'w+') as file:
-            response = self.submission_course.upload_comment(file)
 
-        self.assertTrue(response[0])
-        self.assertIsInstance(response[1], dict)
-        self.assertIn('url', response[1])
+        try:
+            with open(filename, 'w+') as file:
+                response = self.submission_course.upload_comment(file)
 
-        cleanup_file(filename)
+            self.assertTrue(response[0])
+            self.assertIsInstance(response[1], dict)
+            self.assertIn('url', response[1])
+        finally:
+            cleanup_file(filename)
 
     def test_upload_comment_section(self, m):
         # Sections do not support uploading file comments
diff --git a/tests/test_uploader.py b/tests/test_uploader.py
index a72ecd2..6cea063 100644
--- a/tests/test_uploader.py
+++ b/tests/test_uploader.py
@@ -22,7 +22,6 @@ class TestUploader(unittest.TestCase):
 
     def tearDown(self):
         self.file.close()
-
         cleanup_file(self.filename)
 
     # start()
diff --git a/tests/test_user.py b/tests/test_user.py
index fa5d5b7..d2ed67c 100644
--- a/tests/test_user.py
+++ b/tests/test_user.py
@@ -203,14 +203,16 @@ class TestUser(unittest.TestCase):
         register_uris({'user': ['upload', 'upload_final']}, m)
 
         filename = 'testfile_user_{}'.format(uuid.uuid4().hex)
-        with open(filename, 'w+') as file:
-            response = self.user.upload(file)
 
-        self.assertTrue(response[0])
-        self.assertIsInstance(response[1], dict)
-        self.assertIn('url', response[1])
+        try:
+            with open(filename, 'w+') as file:
+                response = self.user.upload(file)
 
-        cleanup_file(filename)
+            self.assertTrue(response[0])
+            self.assertIsInstance(response[1], dict)
+            self.assertIn('url', response[1])
+        finally:
+            cleanup_file(filename)
 
     # list_groups()
     def test_list_groups(self, m):
diff --git a/tests/test_util.py b/tests/test_util.py
index 71677d0..472b4f1 100644
--- a/tests/test_util.py
+++ b/tests/test_util.py
@@ -6,7 +6,9 @@ import requests_mock
 from canvasapi import Canvas
 from canvasapi.course import CourseNickname
 from canvasapi.user import User
-from canvasapi.util import combine_kwargs, is_multivalued, obj_or_id
+from canvasapi.util import (
+    combine_kwargs, get_institution_url, is_multivalued, obj_or_id
+)
 from itertools import chain
 from six import integer_types, iterkeys, itervalues, iteritems
 from six.moves import zip
@@ -408,3 +410,8 @@ class TestUtil(unittest.TestCase):
 
         with self.assertRaises(TypeError):
             obj_or_id(nick, 'nickname_id', (CourseNickname,))
+
+    # get_institution_url()
+    def test_get_institution_url(self, m):
+        base_url = 'https://my.canvas.edu/api/v1'
+        self.assertEqual(get_institution_url(base_url), 'https://my.canvas.edu')
diff --git a/tests/util.py b/tests/util.py
index 3b92514..89a2616 100644
--- a/tests/util.py
+++ b/tests/util.py
@@ -4,6 +4,7 @@ import os
 
 import requests_mock
 
+from canvasapi.util import get_institution_url
 from tests import settings
 
 
@@ -17,7 +18,6 @@ def register_uris(requirements, requests_mocker):
     :param requests_mocker: requests_mock.mocker.Mocker
     """
     for fixture, objects in requirements.items():
-
         try:
             with open('tests/fixtures/{}.json'.format(fixture)) as file:
                 data = json.loads(file.read())
@@ -40,7 +40,7 @@ def register_uris(requirements, requests_mocker):
             if obj['endpoint'] == 'ANY':
                 url = requests_mock.ANY
             else:
-                url = settings.BASE_URL + obj['endpoint']
+                url = get_institution_url(settings.BASE_URL) + '/api/v1/' + obj['endpoint']
 
             try:
                 requests_mocker.register_uri(