Metadata template endpoints #353

mattwiller · 2018-10-25T06:33:13Z

No description provided.

…template_endpoints

boxcla · 2018-10-25T06:33:26Z

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

coveralls · 2018-10-25T06:39:21Z

Pull Request Test Coverage Report for Build 1308

104 of 106 (98.11%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.04%) to 97.056%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
boxsdk/object/metadata_template.py	79	81	97.53%

Totals
Change from base Build 1307:	0.04%
Covered Lines:	2637
Relevant Lines:	2717

💛 - Coveralls

Jeff-Meadows · 2018-10-25T16:53:30Z

boxsdk/util/translator.py

+        `bool`
+    """
+
+    return 'displayName' in obj and obj['type'] != 'metadata_template'


This is a pretty ugly hack, and could easily break in the future, if the API ever returns another object with displayName as a field.

Perhaps we could solve this issue a different way.

Add an optional parent object parameter to the translate method so that it could determine if the parent is a metadata_template.

Add blacklist fields to object classes. When we come across a metadata_template, perhaps we know not to translate its fields.

Jeff-Meadows · 2018-10-25T19:55:48Z

boxsdk/util/translator.py

    :return:
    """
-    if obj.get('type', '') == 'event':
+    obj_type = obj.get('type')


This logic probably belongs in object classes. I know this method isn't new as of this review, but it's becoming more obvious that this is the wrong place for this logic.

I agree. Once we know the type, we can call a staticmethod on the class to get the ID.

But I hate that we have to do this. As I said below, I think we can get rid of the event_id lookup, because we end up throwing it away anyway.

That leaves only metadata_template which requires this logic. And I wish they would standardize all of their APIs around PKs instead of scope/template. Can we at least, in the SDK, standardize around PK?

mattwiller · 2018-10-26T03:29:02Z

@Jeff-Meadows I added the field exclusion mechanism so object classes can specify which fields should not be translated. I wasn't certain how we might want to decentralize the logic for finding an ID, so I left it in the translator for now.

boxsdk/object/metadata_template.py

boxsdk/object/base_api_json_object.py

boxsdk/util/translator.py

boxsdk/object/metadata_template.py

jmoldow · 2018-10-31T16:35:12Z

boxsdk/client/client.py

+        )
+
+    @api_call
+    def create_metadata_template(self, display_name, fields, template_key=None, hidden=False, scope='enterprise'):


Personal taste, I feel like scope should come before the other optional parameters.

scope is logically the most important part of this operation. And in the APIs, scope always proceeds templateKey.

I don't mind moving it, but I put it last since at this time only enterprise scope templates can be created. Nobody calling this method today (and likely for the near future) will be setting that parameter, whereas they will likely want to set the others.

boxsdk/object/metadata_template.py

boxsdk/object/base_api_json_object.py

boxsdk/object/metadata_template.py

boxsdk/util/translator.py

jmoldow · 2018-10-31T16:51:57Z

boxsdk/util/translator.py

    :return:
    """
-    if obj.get('type', '') == 'event':
+    obj_type = obj.get('type')


I agree. Once we know the type, we can call a staticmethod on the class to get the ID.

But I hate that we have to do this. As I said below, I think we can get rid of the event_id lookup, because we end up throwing it away anyway.

That leaves only metadata_template which requires this logic. And I wish they would standardize all of their APIs around PKs instead of scope/template. Can we at least, in the SDK, standardize around PK?

jmoldow · 2018-10-31T16:56:53Z

boxsdk/client/client.py

+        """
+        return self.translator.get('metadata_template')(
+            session=self._session,
+            object_id='{0}/{1}'.format(scope, template_key),


I really don't like doing this.

Can we flip things around here?

object_id is always the primary key id. For the template_id constructor, we pass it in as-is, without any GET requests.

For the scope, template_key constructor, we do the GET call in order to get the id. And we also pass the response_object, so that we keep the scope and template_key around. Also, we pass in an extra boolean flag (which is normally defaulted to False, but we would pass True here) specifying that the client wants to use the scope/templateKey, not the ID.

For APIs that only work with scope/templateKey, always use scope/templateKey

For APIs that work with either id or scope/templateKey, default to using id. But use scope/templateKey if that optional boolean was set to True, indicating that scope/templateKey is the desired client behavior.

Thinking about this more, I feel even more strongly that we should make this change.

I think the current approach is going to cause us problems, now and in the future.

You can already see one problem forming in this PR, where there's a bunch of places in the code where we need to manually concat the scope and templateKey to form the object_id.

Another place we're going to run into problems is with POST and PUT operations where the body expects metadata template. If we naively do template.object_id, that might be incorrect if the API expects the actual id (I know of at least one API that was supposed to be doing this in the future). If we do template.id, that field might not actually be available, if it wasn't constructed with one.

For simplicity, we could make it so that if we're constructed with the id, we fetch the scope and templateKey, and also vice-versa, that way we always have all of those pieces of information available.

boxsdk/util/translator.py

jmoldow · 2018-10-31T23:35:53Z

boxsdk/client/client.py

+        )
+
+    @api_call
+    def get_metadata_template_by_id(self, template_id):


Should we get rid of the .get() behavior here? (and if so, rename the method)

jmoldow · 2018-10-31T23:40:47Z

test/unit/util/test_translator.py

+
+    if metadata_template_response.get('scope') and metadata_template_response.get('templateKey'):
+        assert metadata_template.scope == metadata_template_response['scope']
+        assert metadata_template.template_key == metadata_template_response['templateKey']


I think you have this in a different test too, but just for completeness, can we assert that metadata_template.object_id is None if metadata_template_response.get('id') is None

Matt Willer added 8 commits October 24, 2018 15:17

WIP: Add metadata template endpoints

1b8d23a

Merge branch 'master' of github.com:box/box-python-sdk into metadata_…

d0ae639

…template_endpoints

Finish metadata template endpoints implementation

7792591

Add tests for metadata template endpoints

d2d96ec

Fix lint errors

54a8e66

Add documentation

7f83a3f

Document paging params

4633ccf

Add scope and template_key properties

40e4fbe

mattwiller requested review from Jeff-Meadows, carycheng and jmoldow October 25, 2018 06:33

Add table of contents to docs

f6c4e83

Jeff-Meadows reviewed Oct 25, 2018

View reviewed changes

Allow object classes to exclude fields from translation

5eb4453

Jeff-Meadows reviewed Oct 26, 2018

View reviewed changes

Fixes from code review feedback

519f060

jmoldow reviewed Oct 31, 2018

View reviewed changes

Matt Willer added 3 commits October 31, 2018 12:24

Fixes from code review

3d88adb

Fix metadata template object ID kludge

4e208a9

Merge branch 'master' into metadata_template_endpoints

44928c4

jmoldow approved these changes Oct 31, 2018

View reviewed changes

Matt Willer added 2 commits October 31, 2018 16:53

Fix code review comments

dec4a0b

Merge branch 'master' into metadata_template_endpoints

1f96206

mattwiller merged commit 7fb1f16 into master Nov 1, 2018

mattwiller deleted the metadata_template_endpoints branch November 1, 2018 02:05

Metadata template endpoints #353

Metadata template endpoints #353

Uh oh!

Conversation

mattwiller commented Oct 25, 2018

Uh oh!

boxcla commented Oct 25, 2018

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 1308

💛 - Coveralls

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 Oct 26, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

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!

6 participants

coveralls commented Oct 25, 2018 •

edited

Loading