Add workbook to tasks by williamlang · Pull Request #206 · tableau/server-client-python

williamlang · Jul 13, 2017

According to the docs here:

https://onlinehelp.tableau.com/current/api/rest_api/en-us/help.htm#REST/rest_api_ref.htm#Query_Extract_Refresh_Tasks%3FTocPath%3DAPI%2520Reference%7C_____57

Tasks may or may not have a workbook tag. Added support for this.

t8y8 · Jul 13, 2017

@williamlang is a Tableau employee for those who may not know :)

I'm not super familiar with the Tasks APIs... when does it include a workbook id?

It looks like it could in theory contain a data source as well -- which means we'd want a datasource_id as well OR an 'object_id' and 'object_type' that we handle for the user

I'll take a look at the code in a bit, I reserve the right to propose an alternative ;)

t8y8

The code LGTM.

Still thinking about the API...

williamlang · Jul 13, 2017

@t8y8 you are right though, the second example response does include a datasource id.

williamlang · Jul 13, 2017

@t8y8 @benlower @RussTheAerialist

Here are a few scenarios of how we could incorporate workbook_id and datasource_id

        # case 1: workbook_id and datasource_id
        workbook_tasks = [[task.id, task.schedule_id, task.workbook_id] for task in all_tasks]
        datasource_taks = [[task.id, task.schedule_id, task.datasource_id] for task in all_tasks]

        for workbook_task in workbook_tasks:
            server.workbooks.get(workbook_task.workbook_id)

        for datasource_task in datasource_taks:
            server.datasources.get(datasource_task.datasource_id)

        # case 2: object_name
        workbook_tasks = filter(lambda task : task.object_name == 'workbook', all_tasks)
        datasource_tasks = filter(lambda task : task.object_name == 'datasource', all_tasks)

        # case 2b: object_name
        for task in all_tasks:
            if task.object_name == 'workbook':
                server.workbooks.get(task.object_id)
            else if task.object_name == 'datasource':
                server.datasources.get(task.object_id)

        # case 3: hash map
        workbook_tasks = filter(lambda task : task.content.type == 'workbook', all_tasks)
        datasource_tasks = filter(lambda task : task.content.type == 'datasource', all_tasks)

        # case 3b: hash_map
        for task in all_tasks:
            if task.content.type == 'workbook':
                server.workbooks.get(task.content.id)
            else if task.content.type == 'datasource':
                server.datasources.get(task.content.id)`

t8y8 · Jul 13, 2017

I think the dict case might be the cleanest API:

for task in all_tasks:
    if task.content['type'] == 'workbook':  # Note that dot notation doesn't work for dicts in python
        print(task.content['id'])

Or

wb_tasks = filter(lambda t: t.content.get('type') == 'workbook', all_tasks)

This is actually a pattern we have to solve for permissions APIs. I slightly prefer the dict but I'm also ok with option 1, simply having a datasource_id AND a workbook_id attribute, and maybe some setter magic to make sure both are never set.

graysonarts · Jul 14, 2017

Hey @williamlang -

@t8y8 and I talked about the various approaches outlined. I'm not super keen on any of the three as outlined, but I think merging the ideas of 2 and 3 into a 4th option would be the way I'd like to see it happen.

If we had a class called Target with a type and id attribute, then we'd get the ease of use of 3 with the slightly stronger type of 2 without polluting the Task object with prefixed attributes (e.g. task.object.id reads better to me than task.object_id.)

Does that make sense?

williamlang · Jul 17, 2017

@RussTheAerialist @t8y8

is this what you're thinking?

t8y8

Few comments for cleanup.

The question, do we want it to be target_type for the Target class or .type I think .type is fine since it's namespaced in the class.

t8y8 · Jul 18, 2017

tableauserverclient/models/target.py

+    def __repr__(self):
+        return "<Target#{id}, {target_type}>"
+
+    def get_type(self):


Ahh Java snuck into your Python code ;)

+class Target(): def __init__(self, id, target_type): self.id = id self.target_type = target_type @property def target_type(self): return self.target_type

Or you can just leave them as attributes without any protection since the object is very simple:

class Target(object): def __init__(...): self.id = id_ self.target_type = target_type def __repr__(self): ...

Or, if we go with type:

class Target(object): def __init__(self, id_, target_type) self.id = id_ self.type = target_type @property def type(self): return self.type

I think I'm more of a fan of type rather than target_type because Target is already included in the name of the class, so it ends up being redundant.

t8y8 · Jul 18, 2017

tableauserverclient/models/target.py

@@ -0,0 +1,14 @@
+"""Target class meant to abstract mappings to other objects"""
+class Target():
+    def __init__(self, id, target_type):


Name the function parameter id_ to avoid collision with the built in.

t8y8 · Jul 18, 2017

tableauserverclient/models/target.py

@@ -0,0 +1,14 @@
+"""Target class meant to abstract mappings to other objects"""
+class Target():


Two blank lines here (between docstring and the class def) should fix the style checker
tableauserverclient/models/target.py:2:1: E302 expected 2 blank lines, found 0

graysonarts · Jul 18, 2017

tableauserverclient/models/target.py

+        self.target_type = target_type
+
+    def __repr__(self):
+        return "<Target#{id}, {target_type}>"


I think you'll need a .format(**self) at the end of the repr.

graysonarts · Jul 18, 2017

tableauserverclient/models/task_item.py

        self.task_type = task_type
        self.priority = priority
        self.consecutive_failed_count = consecutive_failed_count
-        self.schedule_id = schedule_id


Why was this line removed?

williamlang · Jul 19, 2017

@RussTheAerialist added a test to prevent schedule_id getting deleted again :)

@t8y8 did the thing.

t8y8

🚀

Two small nits

t8y8 · Jul 19, 2017

tableauserverclient/models/task_item.py

+        if datasource_element is not None:
+            datasource_id = datasource_element.get('id', None)
+            target = Target(datasource_id, "datasource")
+        #


Remove this extra '#' and the space around it (of course still making the style checker happy)

t8y8 · Jul 19, 2017

test/test_task.py

+        self.assertEqual('c7a9327e-1cda-4504-b026-ddb43b976d1d', task.target.id)
+        self.assertEqual('datasource', task.target.type)
+
+    def test_get_tasks_with_workbook_and_datasource(self):


This is technically never possible, right? (though a test for it is fine -- we may do this someday)

t8y8 · Jul 20, 2017

🚀🚀

graysonarts

🚀 👨‍🎤

William Lang added 3 commits July 13, 2017 09:42

add workbook id to tasks and add tests

47f6666

add workbook id to __repr__

8f94e29

remove unused imports

8aec69b

t8y8 approved these changes Jul 13, 2017

View reviewed changes

add target class to abstract references to other objects

9ad2544

William Lang added 2 commits July 17, 2017 16:52

forgot a new line

30c228c

aaaand another new line

aa80e6d

t8y8 suggested changes Jul 18, 2017

View reviewed changes

graysonarts suggested changes Jul 18, 2017

View reviewed changes

add test for schedule id, refactor target

4e107ad

t8y8 approved these changes Jul 19, 2017

View reviewed changes

remove #

b8757f1

graysonarts approved these changes Jul 20, 2017

View reviewed changes

t8y8 merged commit 0d146fa into tableau:development Jul 20, 2017

williamlang deleted the add-workbook-to-tasks branch July 24, 2017 20:30

Search code, repositories, users, issues, pull requests...

Conversation

williamlang commented Jul 13, 2017

Uh oh!

t8y8 commented Jul 13, 2017

Uh oh!

t8y8 left a comment

Choose a reason for hiding this comment

Uh oh!

williamlang commented Jul 13, 2017

Uh oh!

williamlang commented Jul 13, 2017

Uh oh!

t8y8 commented Jul 13, 2017

Uh oh!

graysonarts commented Jul 14, 2017

Uh oh!

williamlang commented Jul 17, 2017

Uh oh!

t8y8 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

williamlang commented Jul 19, 2017

Uh oh!

t8y8 left a comment

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!

t8y8 commented Jul 20, 2017

Uh oh!

graysonarts left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

t8y8 left a comment •

edited

Loading