Storage Policies v2 #327

carycheng · 2018-08-21T01:31:17Z

No description provided.

boxcla · 2018-08-21T01:31:19Z

Hi @carycheng, thanks for the pull request. Before we can merge it, we need you to sign our Contributor License Agreement. You can do so electronically here: http://opensource.box.com/cla

Once you have signed, just add a comment to this pull request saying, "CLA signed". Thanks!

mattwiller · 2018-09-11T19:45:57Z

boxsdk/client/client.py

+            return_full_pages=False,
+        )
+
+    def storage_policy_assignments(self, resolved_for_type, resolved_for_id):


Do you still need this method? It looks like this is going to be on User instead of Client

This method should probably be removed or renamed now that the user.get_storage_policy_assignment() method has been added

mattwiller · 2018-09-11T19:50:37Z

boxsdk/object/storage_policy.py

+        :param assignee:
+            The `user` or `enterprise` to assign the storage policy to
+        :type:
+            :class:User


Missing backticks around User

boxsdk/object/storage_policy.py

mattwiller · 2018-09-11T19:52:09Z

boxsdk/object/storage_policy_assignment.py

+
+    _item_type = 'storage_policy_assignment'
+
+    def get_url(self, *args):


You don't need to override this method; the default pluralization (adding an s to the end of the type) works in this case.

mattwiller · 2018-09-11T19:53:07Z

boxsdk/object/user.py


    _item_type = 'user'
+
+    def storage_policy_assignments(self):


This method name should be updated; this doesn't return multiple objects. Maybe get_storage_policy_assignment()?

mattwiller · 2018-09-11T19:53:24Z

boxsdk/object/user.py

+        :returns:
+            An iterator of the entries in the storage policy assignment
+        :rtype:
+            :class:`BoxObjectCollection`


Return value information needs to be updated

mattwiller · 2018-09-11T19:53:58Z

boxsdk/object/user.py

+        return self.translator.translate('storage_policy_assignment')(
+            session=self._session,
+            object_id=response['entries'][0]['id'],
+            response_object=response['entries'][0],


Consider just grabbing response['entries'][0] once and using it on this line and the one above

mattwiller · 2018-09-11T19:55:56Z

docs/storage_policy.md

+```python
+storage_policy_id = '5678'
+user = client.user('1234')
+storage_policy_assignment = client.storage_policy(storage_policyid).assign(user)


storage_policyid should be storage_policy_id

mattwiller · 2018-09-11T19:58:20Z

docs/storage_policy.md

+storage_policy_assignment_id = '1234'
+new_storage_policy_id = '5678'
+updated_item = {'type': 'storage_policy', 'id': new_storage_policy_id}
+updated_storage_policy_assignment = client.storage_policy_assignment(storage_policy_assignment).update_info(updated_item)


Does this example do what you expect? It looks to me like this would try to reassign type and id on the assignment object itself, not the storage_policy subobject. Could you double-check?

mattwiller · 2018-09-11T20:02:55Z

Left some comments and feedback. Also, remember to add the new object types to object/init.py

mattwiller · 2018-09-14T23:08:31Z

docs/storage_policy.md

-new_storage_policy_id = '5678'
-updated_item = {'type': 'storage_policy', 'id': new_storage_policy_id}
-updated_storage_policy_assignment = client.storage_policy_assignment(storage_policy_assignment).update_info(updated_item)
+new__storage_policy_id = '5678'


This variable has an extra underscore after "new", and is also never used in this example

mattwiller · 2018-09-14T23:11:35Z

docs/storage_policy.md

+storage_policy_assignment_id = '1234'
+new__storage_policy_id = '5678'
+new_storage_policy = client.storage_policy('5678')
+updated_storage_policy_assignment = client.storage_policy_assignment(storage_policy_assignment).update_info(new_storage_policy)


This example doesn't work; storage_policy_assignment should be storage_policy_assignment_id. Also, does passing a StoragePolicy object to update_info() really work? It doesn't seem like that would do the right thing.

mattwiller · 2018-09-14T23:12:00Z

docs/storage_policy.md

+Updating a storage policy assignment is done by calling `storage_policy.update_info(update_item)`.
+
+```python
+storage_policy_assignment_id = '1234'


Can we update the sample assignment IDs to look like a real assignment ID?

mattwiller · 2018-09-14T23:12:45Z

docs/storage_policy.md

+Get Assignment for User
+-------------------------
+
+Calling `user.get_storage_policy_assignment(fields=None)` will return a storage policy assignment object


This explanation could use some beefing up; we might want to explain which assignments could be returned etc.

mattwiller · 2018-09-14T23:13:55Z

boxsdk/client/client.py

+            return_full_pages=False,
+        )
+
+    def storage_policy_assignments(self, resolved_for_type, resolved_for_id):


This method should probably be removed or renamed now that the user.get_storage_policy_assignment() method has been added

test/unit/object/test_storage_policy_assignment.py

mattwiller · 2018-09-14T23:18:36Z

boxsdk/client/client.py

+        """
+        return self.translator.translate('storage_policy_assignment')(session=self._session, object_id=assignment_id)
+
+    def storage_policies(self, limit=None, marker=None, fields=None):


Along the lines of our previous conversation, we should consider renaming this to something like get_storage_policies()

boxsdk/object/storage_policy.py

mattwiller · 2018-09-17T21:17:57Z

docs/storage_policy.md

-new__storage_policy_id = '5678'
-new_storage_policy = client.storage_policy('5678')
-updated_storage_policy_assignment = client.storage_policy_assignment(storage_policy_assignment).update_info(new_storage_policy)
+updated_storage_policy = {'storage_policy': {'type': 'storage_policy', 'id': '1234'}}


We might want to consider creating an override for this method to enable just passing in a StoragePolicy (since that's the only thing that can be updated)

mattwiller · 2018-09-17T21:25:34Z

boxsdk/client/client.py

+        :type fields:
+            `Iterable` of `unicode`
+        :returns:
+            An iterator of the entries in the storage policy


This description could be a little clearer; this returns the storage policies available for the current enterprise

mattwiller · 2018-09-17T21:27:20Z

boxsdk/object/user.py

+        }
+        box_response = self._session.get(url, params=additional_params)
+        response = box_response.json()['entries'][0]
+        return self.translator.translate('storage_policy_assignment')(


This should use response['type'] rather than hardcoding the string

mattwiller · 2018-09-17T21:30:02Z

docs/storage_policy.md

+
+```python
+assignment_id = '1234'
+policy_assignment = client.storage_policy_assignment(assignment_id).delete()


This example needs to be updated; delete() doesn't return anything

mattwiller · 2018-09-17T21:30:49Z

test/unit/object/test_storage_policy.py

+
+
+def test_assign(test_storage_policy, mock_user, mock_box_session):
+    expected_url = mock_box_session.get_url('storage_policy_assignments')


Expected URL should be hardcoded

…on-sdk into storage_policies_v2

coveralls · 2018-10-25T04:02:23Z

Pull Request Test Coverage Report for Build 1240

36 of 36 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 95.472%

Totals
Change from base Build 1238:	0.07%
Covered Lines:	2298
Relevant Lines:	2407

💛 - Coveralls

…correct translated objects

front porting storage policies

9067f01

carycheng changed the title ~~front porting storage policies~~ Storage Policies v2 Aug 21, 2018

carycheng added 4 commits August 24, 2018 11:09

fixed tests for client and added object_type in base object

b317656

removed extraneous line in storage policy assignment

dcba51c

removed extraneous lines from test_storage_policy_assignment

c196988

switched import order

5654ae0

carycheng requested review from Jeff-Meadows and mattwiller August 24, 2018 20:51

carycheng added 9 commits August 27, 2018 16:28

added docs for storage policy endpoint

51ddeff

fixed docs for storage policy and removed constructors usage

2d80470

fixed updates to storage policies

e6a3e8b

updated docs and moved assignment onto user

55505d8

fixed assign on storage policy

52f6bc8

removedu unused fields

c9fd61b

added limit and marker

5bfc330

removed unused fields

409023d

removed unused import

197bace

mattwiller reviewed Sep 11, 2018

View reviewed changes

fixed from feedback

b5af4fa

mattwiller suggested changes Sep 14, 2018

View reviewed changes

carycheng added 4 commits September 16, 2018 15:04

changes to storage policy

e19babf

fixed docs for storage policy

1c6d8af

removed unused lines

eff576b

removed unused import

072f68a

mattwiller reviewed Sep 17, 2018

View reviewed changes

carycheng added 3 commits September 17, 2018 16:58

fixed from feedback

defb9f1

Merge branch 'master' into storage_policies_v2

f60b81c

updates to storage policies

146284a

carycheng added 6 commits October 24, 2018 16:55

added tests for storage policy assign method

23ba14a

Merge branch 'master' into storage_policies_v2

ed17f80

replace hard coded type

e90bb38

Merge branch 'storage_policies_v2' of https://github.com/box/box-pyth…

f0465dd

…on-sdk into storage_policies_v2

added asserts to test client for storage policies get

81cb159

converted translator to use new one

1ef7425

added test for storage policy and storage policy constructor returns …

afc6ee9

…correct translated objects

mattwiller approved these changes Oct 25, 2018

View reviewed changes

carycheng added 5 commits October 26, 2018 13:24

Merge branch 'master' into storage_policies_v2

2885870

Merge branch 'master' into storage_policies_v2

470b334

added end of paging method

d262b8c

added additional line spacing between tests in test_client

c3d34ce

Merge branch 'master' into storage_policies_v2

8d10cc6

carycheng merged commit 68d8b89 into master Oct 26, 2018

carycheng deleted the storage_policies_v2 branch October 26, 2018 23:05


		_item_type = 'storage_policy_assignment'

		def get_url(self, *args):



		def test_assign(test_storage_policy, mock_user, mock_box_session):
		expected_url = mock_box_session.get_url('storage_policy_assignments')

Storage Policies v2 #327

Storage Policies v2 #327

Uh oh!

Conversation

carycheng commented Aug 21, 2018

Uh oh!

boxcla commented Aug 21, 2018

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!

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!

mattwiller commented Sep 11, 2018

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

coveralls commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1240

💛 - Coveralls

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Oct 25, 2018 •

edited

Loading