Repository: box/box-python-sdk (python)

Issue:
BaseObject.__eq__ is incorrect
`BaseObject.__eq__` only compares object ID, not type. So `client.file(id) == client.folder(id)` evaluates to `True`.

It should also return `NotImplemented` if the other object is not a `BaseObject`, but right now it will raise an exception (e.g. #151 ).

This is technically a backwards-incompatible change. We'll include this in the major version bump v2.0.0.

We also need to define `__hash__`.

I've started work on this already.


Pull request description:
Fix API object equality test
Fixes #163 

Patch:
diff --git a/.travis.yml b/.travis.yml
index 4aa3a59..ad6acef 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,6 +1,11 @@
 sudo: false
 language: python
 
+branches:
+    only:
+        - master
+        - "1.5"
+
 cache:
   directories:
     - $HOME/.cache/pip
diff --git a/HISTORY.rst b/HISTORY.rst
index 7c3d85d..e39f426 100644
--- a/HISTORY.rst
+++ b/HISTORY.rst
@@ -40,6 +40,9 @@ Release History
   inside ``file['entries'][0]``. This is a bugfix, but will be a breaking
   change for any clients that have already written code to handle the bug.
 
+- Comparing two objects (e.g. a ``File`` and a ``Folder``) that have the same Box ID but different types with ``==``
+  will now correctly return `False`.
+
 **Features**
 
 - Added more flexibility to the object translation system:
diff --git a/boxsdk/client/client.py b/boxsdk/client/client.py
index fc00146..0594de4 100644
--- a/boxsdk/client/client.py
+++ b/boxsdk/client/client.py
@@ -10,6 +10,7 @@
 from ..util.api_call_decorator import api_call
 from ..object.search import Search
 from ..object.events import Events
+from ..pagination.limit_offset_based_object_collection import LimitOffsetBasedObjectCollection
 from ..pagination.marker_based_object_collection import MarkerBasedObjectCollection
 from ..util.shared_link import get_shared_link_header
 
@@ -158,6 +159,44 @@ def collaboration(self, collab_id):
         """
         return self.translator.translate('collaboration')(session=self._session, object_id=collab_id)
 
+    def collection(self, collection_id):
+        """
+        Initialize a :class:`Collection` object, whose box ID is collection_id.
+
+        :param collection_id:
+            The box id of the :class:`Collection` object.
+        :type collection_id:
+            `unicode`
+        :return:
+            A :class:`Collection` object with the given collection ID.
+        :rtype:
+            :class:`Collection`
+        """
+        return self.translator.translate('collection')(session=self._session, object_id=collection_id)
+
+    @api_call
+    def collections(self, limit=None, offset=0, fields=None):
+        """
+        Get a list of collections for the current user.
+
+        :param limit:
+            The maximum number of users to return. If not specified, the Box API will determine an appropriate limit.
+        :type limit:
+            `int` or None
+        :param offset:
+            The user index at which to start the response.
+        :type offset:
+            `int`
+        """
+        return LimitOffsetBasedObjectCollection(
+            self.session,
+            self._session.get_url('collections'),
+            limit=limit,
+            fields=fields,
+            offset=offset,
+            return_full_pages=False,
+        )
+
     @api_call
     def users(self, limit=None, offset=0, filter_term=None):
         """
diff --git a/boxsdk/config.py b/boxsdk/config.py
index 2676ba2..203cfc2 100644
--- a/boxsdk/config.py
+++ b/boxsdk/config.py
@@ -10,7 +10,7 @@ class API(object):
     BASE_API_URL = 'https://api.box.com/2.0'
     UPLOAD_URL = 'https://upload.box.com/api/2.0'
     OAUTH2_API_URL = 'https://api.box.com/oauth2'  # <https://developers.box.com/docs/#oauth-2>
-    OAUTH2_AUTHORIZE_URL = 'https://app.box.com/api/oauth2/authorize'  # <https://developers.box.com/docs/#oauth-2-authorize>
+    OAUTH2_AUTHORIZE_URL = 'https://account.box.com/api/oauth2/authorize'  # <https://developers.box.com/docs/#oauth-2-authorize>
 
 
 class Client(object):
diff --git a/boxsdk/object/__init__.py b/boxsdk/object/__init__.py
index 39c7a79..3d83fbe 100644
--- a/boxsdk/object/__init__.py
+++ b/boxsdk/object/__init__.py
@@ -7,6 +7,7 @@
 
 __all__ = list(map(str, [
     'collaboration',
+    'collection',
     'comment',
     'events',
     'event',
diff --git a/boxsdk/object/base_object.py b/boxsdk/object/base_object.py
index 2c05501..3862278 100644
--- a/boxsdk/object/base_object.py
+++ b/boxsdk/object/base_object.py
@@ -155,8 +155,19 @@ def delete(self, params=None, headers=None):
         return box_response.ok
 
     def __eq__(self, other):
-        """Equality as determined by object id"""
-        return self._object_id == other.object_id
+        """Equality as determined by object id and type"""
+        if isinstance(other, BaseObject):
+            # Two objects are considered the same if they have the same address in the API
+            return self.get_url() == other.get_url()
+
+        return NotImplemented
+
+    def __ne__(self, other):
+        """Equality as determined by object id and type"""
+        return not self == other
+
+    def __hash__(self):
+        return hash((self._object_id, self._item_type))
 
     def _paging_wrapper(self, url, starting_index, limit, factory=None):
         """
diff --git a/boxsdk/object/collection.py b/boxsdk/object/collection.py
new file mode 100644
index 0000000..bd096da
--- /dev/null
+++ b/boxsdk/object/collection.py
@@ -0,0 +1,44 @@
+# coding: utf-8
+
+from __future__ import unicode_literals
+
+from boxsdk.object.base_object import BaseObject
+from boxsdk.pagination.limit_offset_based_object_collection import LimitOffsetBasedObjectCollection
+from boxsdk.util.api_call_decorator import api_call
+
+
+class Collection(BaseObject):
+    """Box API endpoint for interacting with collections."""
+
+    _item_type = 'collection'
+
+    @api_call
+    def get_items(self, limit=None, offset=0, fields=None):
+        """
+        Get the items in a collection using limit-offset paging.
+
+        :param limit:
+            The maximum number of items to return per page. If not specified, then will use the server-side default.
+        :type limit:
+            `int` or None
+        :param offset:
+            The index at which to start returning items.
+        :type offset:
+            `int`
+        :param fields:
+            List of fields to request.
+        :type fields:
+            `Iterable` of `unicode`
+        :returns:
+            An iterator of the items in the folder.
+        :rtype:
+            :class:`BoxObjectCollection`
+        """
+        return LimitOffsetBasedObjectCollection(
+            self.session,
+            self.get_url('items'),
+            limit=limit,
+            fields=fields,
+            offset=offset,
+            return_full_pages=False,
+        )
diff --git a/boxsdk/object/item.py b/boxsdk/object/item.py
index 3368e78..1050d77 100644
--- a/boxsdk/object/item.py
+++ b/boxsdk/object/item.py
@@ -356,3 +356,45 @@ def metadata(self, scope='global', template='properties'):
             :class:`Metadata`
         """
         return Metadata(self._session, self, scope, template)
+
+    @api_call
+    def add_to_collection(self, collection):
+        """
+        Add the item to a collection.  This method is not currently safe from race conditions.
+
+        :param collection:
+            The collection to add the item to.
+        :type collection:
+            :class:`Collection`
+        :return:
+            This item.
+        :rtype:
+            :class:`Item`
+        """
+        collections = self.get(fields=['collections']).collections  # pylint:disable=no-member
+        collections.append({'id': collection.object_id})
+        data = {
+            'collections': collections
+        }
+        return self.update_info(data)
+
+    @api_call
+    def remove_from_collection(self, collection):
+        """
+        Remove the item from a collection.  This method is not currently safe from race conditions.
+
+        :param collection:
+            The collection to remove the item from.
+        :type collection:
+            :class:`Collection`
+        :return:
+            This item.
+        :rtype:
+            :class:`Item`
+        """
+        collections = self.get(fields=['collections']).collections  # pylint:disable=no-member
+        updated_collections = [c for c in collections if c['id'] != collection.object_id]
+        data = {
+            'collections': updated_collections
+        }
+        return self.update_info(data)


Test patch:
diff --git a/test/unit/object/conftest.py b/test/unit/object/conftest.py
index 9c0264e..021e4d3 100644
--- a/test/unit/object/conftest.py
+++ b/test/unit/object/conftest.py
@@ -7,6 +7,7 @@
 import pytest
 from six import int2byte, PY2
 from boxsdk.object.collaboration import Collaboration
+from boxsdk.object.collection import Collection
 from boxsdk.object.comment import Comment
 from boxsdk.object.file import File
 from boxsdk.object.folder import Folder
@@ -28,6 +29,11 @@ def mock_collaboration_id():
     return 'collab_id1'
 
 
+@pytest.fixture(scope='module')
+def mock_collection_id():
+    return 'collection_id1'
+
+
 @pytest.fixture(scope='module')
 def mock_file_path():
     return os.path.join('path', 'to', 'file')
@@ -104,6 +110,11 @@ def mock_user(mock_box_session, mock_user_id):
     return user
 
 
+@pytest.fixture()
+def mock_collection(mock_box_session, mock_collection_id):
+    return Collection(mock_box_session, mock_collection_id)
+
+
 @pytest.fixture(scope='function')
 def mock_precondition_failed_response(mock_object_id, make_mock_box_request):
     mock_box_response, _ = make_mock_box_request(
diff --git a/test/unit/object/test_base_object.py b/test/unit/object/test_base_object.py
index f0ac36f..b0f84a7 100644
--- a/test/unit/object/test_base_object.py
+++ b/test/unit/object/test_base_object.py
@@ -6,6 +6,9 @@
 import pytest
 from boxsdk.config import API
 from boxsdk.object.base_object import BaseObject
+from boxsdk.object.file import File
+from boxsdk.object.folder import Folder
+from boxsdk.object.user import User
 
 
 @pytest.fixture(params=('file', 'folder', 'user'))
@@ -21,6 +24,21 @@ def test_object_and_response(
     return test_objects_and_responses[request.param]
 
 
+@pytest.fixture(params=('same_file', 'same_folder', 'same_user', 'against_none', 'different_ids', 'different_types'))
+def objects_for_comparison(test_file, test_folder, mock_user, request, mock_box_session):
+    cases = {
+        'identical_object': (test_file, test_file, True),
+        'same_file': (test_file, File(mock_box_session, test_file.object_id), True),
+        'same_folder': (test_folder, Folder(mock_box_session, test_folder.object_id), True),
+        'same_user': (mock_user, User(mock_box_session, mock_user.object_id), True),
+        'against_none': (test_file, None, False),
+        'different_ids': (File(mock_box_session, '1'), File(mock_box_session, '2'), False),
+        'different_types': (File(mock_box_session, '1'), Folder(mock_box_session, '1'), False),
+        'not_api_object': (test_file, {'id': test_file.object_id, 'type': 'file'}, False),
+    }
+    return cases[request.param]
+
+
 @pytest.mark.parametrize('params,headers', product(*([[None, {}, {'foo': 'bar'}, {'foo': 'bar', 'num': 4}]] * 2)))
 def test_update_info(test_object_and_response, mock_box_session, params, headers):
     # pylint:disable=redefined-outer-name, protected-access
@@ -79,3 +97,28 @@ def test_get_type_url(test_object_and_response):
     test_object, _ = test_object_and_response
     url = test_object.get_type_url()
     assert url.endswith('{0}s'.format(test_object._item_type))
+
+
+def test_eq(objects_for_comparison):
+    # pylint:disable=redefined-outer-name
+    obj1, obj2, expected_value = objects_for_comparison
+    assert (obj1 == obj2) == expected_value
+
+
+def test_ne(objects_for_comparison):
+    # pylint:disable=redefined-outer-name
+    obj1, obj2, are_equal = objects_for_comparison
+    expected_value = not are_equal
+    assert (obj1 != obj2) == expected_value
+
+
+def test_hash(objects_for_comparison):
+    # pylint:disable=redefined-outer-name
+    obj1, obj2, are_equal = objects_for_comparison
+
+    # Set a value in the dict such that it will be overridden to True iff the objects hash identically (are equal)
+    test_dict = {}
+    test_dict[obj1] = False
+    test_dict[obj2] = True
+
+    assert test_dict[obj1] == are_equal
diff --git a/test/unit/object/test_collection.py b/test/unit/object/test_collection.py
new file mode 100644
index 0000000..dc43eb2
--- /dev/null
+++ b/test/unit/object/test_collection.py
@@ -0,0 +1,53 @@
+# coding: utf-8
+
+from __future__ import unicode_literals
+
+
+def test_get(mock_collection, mock_box_session):
+    expected_url = mock_collection.get_url()
+    collection_name = 'Favorites'
+
+    mock_box_session.get.return_value.json.return_value = {
+        'type': 'collection',
+        'id': mock_collection.object_id,
+        'name': collection_name,
+        'collection_type': 'favorites'
+    }
+
+    fetched_collection = mock_collection.get()
+
+    mock_box_session.get.assert_called_once_with(expected_url, headers=None, params=None)
+    assert fetched_collection.name == collection_name
+
+
+def test_get_items(mock_collection, mock_box_session):
+    expected_url = mock_collection.get_url('items')
+    item_id1 = '12345'
+    item_id2 = '56789'
+
+    mock_box_session.get.return_value.json.return_value = {
+        'limit': 100,
+        'offset': 0,
+        'total_count': 2,
+        'entries': [
+            {
+                'type': 'folder',
+                'id': item_id1
+            },
+            {
+                'type': 'file',
+                'id': item_id2
+            }
+        ]
+    }
+
+    items = mock_collection.get_items()
+
+    item1 = items.next()
+    mock_box_session.get.assert_called_once_with(expected_url, params={'offset': 0})
+    assert item1.type == 'folder'
+    assert item1.object_id == item_id1
+
+    item2 = items.next()
+    assert item2.type == 'file'
+    assert item2.object_id == item_id2
diff --git a/test/unit/object/test_item.py b/test/unit/object/test_item.py
index 73e5673..2bed02f 100644
--- a/test/unit/object/test_item.py
+++ b/test/unit/object/test_item.py
@@ -12,6 +12,38 @@ def test_item_and_response(test_file, test_folder, mock_file_response, mock_fold
     return test_folder, mock_folder_response
 
 
+@pytest.fixture(params=('empty', 'same', 'other'))
+def test_collections_for_addition(mock_collection_id, request):
+    """Fixture returning a tuple of the expected collections values before and after addition"""
+    other_collection_id = mock_collection_id + '2'
+    if request.param == 'empty':
+        return [], [{'id': mock_collection_id}]
+    elif request.param == 'same':
+        # Adding a second instance of the same collection is handled correctly by the API,
+        # so for simplicity we do not check for an existing copy of the collection and just append
+        return [{'id': mock_collection_id}], [{'id': mock_collection_id}, {'id': mock_collection_id}]
+    elif request.param == 'other':
+        return [{'id': other_collection_id}], [{'id': other_collection_id}, {'id': mock_collection_id}]
+
+    raise NotImplementedError("Forgot to implement {}".format(request.param))
+
+
+@pytest.fixture(params=('empty', 'only_removed', 'only_other', 'other_and_removed'))
+def test_collections_for_removal(mock_collection_id, request):
+    """Fixture returning a tuple of the expected collections values before and after removal"""
+    other_collection_id = mock_collection_id + '2'
+    if request.param == 'empty':
+        return [], []
+    elif request.param == 'only_removed':
+        return [{'id': mock_collection_id}], []
+    elif request.param == 'only_other':
+        return [{'id': other_collection_id}], [{'id': other_collection_id}]
+    elif request.param == 'other_and_removed':
+        return [{'id': mock_collection_id}, {'id': other_collection_id}], [{'id': other_collection_id}]
+
+    raise NotImplementedError("Forgot to implement {}".format(request.param))
+
+
 def test_update_info(test_item_and_response, mock_box_session, etag, if_match_header):
     # pylint:disable=redefined-outer-name, protected-access
     test_item, mock_item_response = test_item_and_response
@@ -141,3 +173,45 @@ def test_get(test_item_and_response, mock_box_session, fields, mock_object_id, e
     mock_box_session.get.assert_called_once_with(expected_url, params=expected_params, headers=if_none_match_header)
     assert isinstance(info, test_item.__class__)
     assert info.id == mock_object_id
+
+
+def test_add_to_collection(test_item_and_response, mock_box_session, mock_collection, test_collections_for_addition):
+    # pylint:disable=redefined-outer-name, protected-access
+    test_item, mock_item_response = test_item_and_response
+    current_collections, expected_collections = test_collections_for_addition
+    expected_url = test_item.get_url()
+    expected_params = {'fields': 'collections'}
+    expected_data = {
+        'collections': expected_collections
+    }
+    mock_response = {
+        'collections': current_collections
+    }
+    mock_box_session.get.return_value.json.return_value = mock_response
+    mock_box_session.put.return_value = mock_item_response
+
+    test_item.add_to_collection(mock_collection)
+
+    mock_box_session.get.assert_called_once_with(expected_url, headers=None, params=expected_params)
+    mock_box_session.put.assert_called_once_with(expected_url, data=json.dumps(expected_data), headers=None, params=None)
+
+
+def test_remove_from_collection(test_item_and_response, mock_box_session, mock_collection, test_collections_for_removal):
+    # pylint:disable=redefined-outer-name, protected-access
+    test_item, mock_item_response = test_item_and_response
+    current_collections, expected_collections = test_collections_for_removal
+    expected_url = test_item.get_url()
+    expected_params = {'fields': 'collections'}
+    expected_data = {
+        'collections': expected_collections
+    }
+    mock_response = {
+        'collections': current_collections
+    }
+    mock_box_session.get.return_value.json.return_value = mock_response
+    mock_box_session.put.return_value = mock_item_response
+
+    test_item.remove_from_collection(mock_collection)
+
+    mock_box_session.get.assert_called_once_with(expected_url, headers=None, params=expected_params)
+    mock_box_session.put.assert_called_once_with(expected_url, data=json.dumps(expected_data), headers=None, params=None)