Skip to content

Conversation

@hugoboos
Copy link
Contributor

No description provided.

@t8y8
Copy link
Collaborator

t8y8 commented Dec 20, 2016

@hugoboos thank you for the submission!

We'll need a few things before we can review:

  1. Please rebase this against the development branch
  2. Please fill out the CLA (linked here http://tableau.github.io/)
  3. We don't currently have any tests that check that the serialization works... Can you include a screenshot of some output that includes the new element? (It's just nice to have a sanity check on record!)

@hugoboos hugoboos changed the base branch from master to development December 21, 2016 08:18
@hugoboos
Copy link
Contributor Author

Hi,

  1. Rebased and changed the PR
  2. Requested e-form to sign
  3. Is it sufficient to show some code that publishes a datasource using the oAuth flag?

@t8y8
Copy link
Collaborator

t8y8 commented Dec 21, 2016

Thank you @hugoboos !

I checked and the CLA stuff is all in motion. Once it's in I'll officially review the code.

For the 3rd item, I think just making a WorkbookItem and calling the method WorkbookRequest._generate_xml(wb_item) and printing the output should show that the serialization works well enough for me (That's how we verified the first incarnation of ConnectionCredentials)

@t8y8 t8y8 removed the CLARequired label Dec 22, 2016
Copy link
Collaborator

@t8y8 t8y8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small requests

(And we've confirmed the CLA is good to go!)

"""

def __init__(self, name, password, embed=True):
def __init__(self, name, password, embed=True, oauth=True):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should default to 'false' since most connections don't need an oAuth parameter (and I'm not sure what they'll do if both are present)

credentials_element.attrib['name'] = connection_credentials.name
credentials_element.attrib['password'] = connection_credentials.password
credentials_element.attrib['embed'] = 'true' if connection_credentials.embed else 'false'
credentials_element.attrib['oAuth'] = 'true' if connection_credentials.oauth else 'false'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with my above comment, this would be including the oauth parameter in all requests, even if it's not an oauth enabled connection... Can you test and see if that causes a problem? If it does I suggest defaulting to None and then doing something like:

if connection_credentials.oauth is not None:
    redentials_element.attrib['oAuth'] = 'true'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will check for that.

@hugoboos
Copy link
Contributor Author

I have looked up the REST API documentation and only when the connection is oAuth, you should set it to true.

So adding oAuth="false" doesn't make any sense. I will update the PR and make the default value set to False.

Screen shot of the generated XML:
screen shot 2016-12-23 at 13 42 16

Copy link
Collaborator

@t8y8 t8y8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Looks great!

@t8y8 t8y8 merged commit f551c0b into tableau:development Dec 23, 2016
@LGraber
Copy link
Contributor

LGraber commented Dec 23, 2016

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants