Add new Recents API to client #210

autoreleasefool · 2017-07-05T20:40:06Z

See: https://developer.box.com/v2.0/reference#recent-item-object

Use the Client to retrieve a user's recently accessed items on Box.

RecentItem class allows the translator to correctly instantiate the item returned by the API, which has the field type='recent_item'. This provides some additional fields, and access to the underlying File/Folder as the property RecentItem.item

boxcla · 2017-07-05T20:40:08Z

Verified that @josephroque has signed the CLA. Thanks for the pull request!

Jeff-Meadows · 2017-07-10T23:37:57Z

boxsdk/client/client.py

            response_object=response,
        )

+    @api_call


It appears this endpoint uses marker based paging, not limit/offset.

👍 waiting on #212 to rebase

Jeff-Meadows · 2017-07-10T23:42:35Z

boxsdk/client/client.py

+            `Iterable` of `unicode`
+        :returns:
+            A list of the user's recently accessed items.
+        :rtype:


this returns a list of RecentItem, right?

Jeff-Meadows · 2017-07-10T23:43:29Z

boxsdk/client/client.py

+            The index at which to start returning items.
+        :type offset:
+            `int`
+        :param fields:


Mention that the fields apply to the recent item object, not to the underlying file or folder.

Actually, these do apply to the underlying file or folder. I'll mention that. The recent_item type always has the same fields, but returns the 'mini' file/folder format by default, so this is to request more fields. It's similarly unclear in the API documentation, but that is what it does in practice. I'll update this to be more clear.

Jeff-Meadows · 2017-07-10T23:45:32Z

boxsdk/client/client.py

+            A list of the user's recently accessed items.
+        :rtype:
+            `list` of :class:`Item`
+        """


this can be self.get_url('recent_items')

Everything else in the file follows the format used here. Should I still change it?

Up to you, I guess. Most of the other things in this file predate being able to use self.get_url

… fields

autoreleasefool · 2017-07-14T21:12:36Z

boxsdk/pagination/page.py

        item = item_class(
            session=self._session,
-            object_id=item_json['id'],
+            object_id=item_json['id'] if 'id' in item_json else None,


Note that RecentItem is not a subclass of BaseObject because most of the functionality provided by BaseObject does not apply to to recent items (o way to GET individual recent items, no way to update). Finally, recent items from the API do not have unique IDs associated with them, so there's no way to get item_json['id']

Maybe we can do something like this:

item_class = self._translator.translate(item_json['type']) kwargs = {} if issubclass(item_class, BaseEndpoint): kwargs['session'] = self._session if issubclass(item_class, BaseObject): kwargs['object_id'] = item_json['id'] if 'id' in item_json else None item = item_class(response_object=item_json, **kwargs)

It's not great, but it should work, for now. Eventually we'll make something nicer than this for #140 .

I think this is what's going to be necessary - solves the **kwargs in RecentItem as well.

jmoldow · 2017-07-19T22:08:51Z

boxsdk/client/client.py

            response_object=response,
        )

+    @api_call


We should add **kwargs, and pass that to MarkerBasedObjectCollection. And then remove return_full_pages=True below, since that's a decision that the client, not the SDK, should make.

Depending on your personal preference, you could remove limit, marker, and fields, and leave them all for **kwargs. Or you can leave it as-is.

jmoldow · 2017-07-19T22:10:51Z

boxsdk/client/client.py

+            limit=limit,
+            fields=fields,
+            marker=marker,
+            return_full_pages=True,


I don't know if this is necessary, I believe it defaults to False.

jmoldow · 2017-07-19T22:14:47Z

boxsdk/object/recent_item.py

+            response_object=item,
+        )
+
+    def get_url(self, *args):


Is this needed? As you said, this isn't a BaseObject, so we'll never be calling API endpoints for this object.

jmoldow · 2017-07-19T22:18:30Z

boxsdk/object/recent_item.py

+
+    _item_type = 'recent_item'
+
+    def __init__(self, session, response_object=None, **kwargs):


We shouldn't accept **kwargs unless we're willing to do something with it.

Doing this is just a hack around the fact that we're passing object_id to it..

We should solve that problem a different way.

And if we remove **kwargs, then you can actually just remove this method override altogether, since it isn't doing anything other than calling super.

jmoldow · 2017-07-19T22:26:40Z

boxsdk/pagination/page.py

        item = item_class(
            session=self._session,
-            object_id=item_json['id'],
+            object_id=item_json['id'] if 'id' in item_json else None,


Maybe we can do something like this:

item_class = self._translator.translate(item_json['type']) kwargs = {} if issubclass(item_class, BaseEndpoint): kwargs['session'] = self._session if issubclass(item_class, BaseObject): kwargs['object_id'] = item_json['id'] if 'id' in item_json else None item = item_class(response_object=item_json, **kwargs)

It's not great, but it should work, for now. Eventually we'll make something nicer than this for #140 .

jmoldow · 2017-07-19T22:28:15Z

test/unit/client/test_client.py

+def marker_id():
+    return 'marker_1'
+
+


Won't this break if it is a module fixture? Because the Mock will retain its call spec between tests if we do this.

This user_response object was pre-existing, git just reports me as adding it. I added the lines above, with the @pytext.fixture from there. So for the comment on scope below, I followed suit from this line you're commenting on here. Additionally, I believe most of these fixtures are used in a read-only sense, so reuse of them in the tests is inconsequential.

jmoldow · 2017-07-19T22:28:32Z

test/unit/client/test_client.py

    return mock_network_response


+@pytest.fixture(scope='module')


Again, this doesn't seem like something that should be module scoped.

jmoldow · 2017-07-19T22:29:32Z

test/unit/client/test_client.py

+        'entries': [
+            {'type': 'recent_item', 'item': {'type': 'file', 'id': file_id}}
+        ],
+        'next_marker': marker_id,


Providing a next_marker implies that there are more results coming. So if you try paging, it'll try to make a second API call, which doesn't seem like what you want.

…tests to match new behavior

jmoldow · 2017-07-20T01:17:07Z

test/unit/object/test_recent_item.py

+            "item": {"type": "file", "id": mock_object_id}
+        })
+    assert recent_item['type'] == 'recent_item'
+    assert recent_item.item.object_id == mock_object_id


Can you also assert recent_item.item.session is mock_box_session?

jmoldow · 2017-07-20T01:18:08Z

boxsdk/client/client.py

            response_object=response,
        )

+    @api_call


In docstring, document that **kwargs are passed to MarkerBasedObjectCollection.

We could also call this **collection_kwargs or **paging_kwargs.

Cool, I was going to do this, but noticed it wasn't done that way somewhere else, so I thought it might be like, a standard.

See: https://developer.box.com/v2.0/reference#recent-item-object Use the Client to retrieve a user's recently accessed items on Box. `RecentItem` class allows the translator to correctly instantiate the item returned by the API, which has the field `type='recent_item'`. This provides some additional fields, and access to the underlying File/Folder as the property `RecentItem.item`

Jeff-Meadows reviewed Jul 10, 2017

View reviewed changes

autoreleasefool added 5 commits July 13, 2017 16:35

Add recent items to client

a91ed99

Add BaseEndpoint as parent for RecentItem, test case for recent_items…

f9431f7

… fields

Explicitly test all params in get_recent_items

b283a7d

Update get_recents to use marker based paging

08b9235

Adjust recents tests for marker based paging

884f557

autoreleasefool commented Jul 14, 2017

View reviewed changes

jmoldow reviewed Jul 19, 2017

View reviewed changes

Remove RecentItem __init__, pass **kwargs directly to parent, update …

e7b396d

…tests to match new behavior

jmoldow approved these changes Jul 20, 2017

View reviewed changes

Document "**kwargs" in get_recent_items, update test case

c2205b3

jmoldow merged commit ea32ba5 into box:master Jul 20, 2017


		_item_type = 'recent_item'

		def __init__(self, session, response_object=None, **kwargs):

Add new Recents API to client #210

Add new Recents API to client #210

Uh oh!

Conversation

autoreleasefool commented Jul 5, 2017

Uh oh!

boxcla commented Jul 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants