Fix for issue #60 by t8y8 · Pull Request #61 · tableau/server-client-python

t8y8 · Oct 14, 2016

Make Sign Out a no-op when you are already signed out.

Here is a repro script that passes after and fails before the change:

import argparse
import getpass
import logging

import tableauserverclient as TSC


def main():

    parser = argparse.ArgumentParser(description='Publish a workbook to server.')
    parser.add_argument('--server', '-s', required=True, help='server address')
    parser.add_argument('--username', '-u', required=True, help='username to sign into server')
    parser.add_argument('--logging-level', '-l', choices=['debug', 'info', 'error'], default='error',
                        help='desired logging level (set to error by default)')

    args = parser.parse_args()

    password = getpass.getpass("Password: ")

    # Set logging level based on user input, or error by default
    logging_level = getattr(logging, args.logging_level.upper())
    logging.basicConfig(level=logging_level)

    # Step 1: Sign in to server.
    tableau_auth = TSC.TableauAuth(args.username, password)
    server = TSC.Server(args.server)

    with server.auth.sign_in(tableau_auth):
        new_site = TSC.SiteItem(name='blorp', content_url='blorp')
        yay = server.sites.create(new_site)

    tableau_auth = TSC.TableauAuth(args.username, password, site='blorp')

    with server.auth.sign_in(tableau_auth):

        sites = server.sites.get()
        print([i for i in sites])
        server.sites.delete(yay.id)

if __name__ == '__main__':
    main()

…signed out

LGraber · Oct 14, 2016

tableauserverclient/server/endpoint/sites_endpoint.py

+        # Deleting the site also signs you out on the Server.
+        # Here we check that you're deleting the site you are signed in to
+        # (Which is true now but may not always be) and then clear auth tokens
+        if site_id == self.parent_srv._site_id:


we have a property getter on the parent_srv object. Why not use that instead of the internal member?

The getter checks if you have a site_id, which we don't because we remove it in the DELETE call when we make it, and then complains that you aren't signed in.

@property def site_id(self): if self._site_id is None: error = 'Missing site ID. You must sign in first.' raise NotSignedInError(error) return self._site_id

We clear the auth info (token and site_id) after this check. This code is in the delete method.

LGraber · Oct 14, 2016

tableauserverclient/server/endpoint/auth_endpoint.py

    def sign_out(self):
        url = "{0}/{1}".format(self.baseurl, 'signout')
+        # If there are no auth tokens you're already signed out. No-op
+        if not self.parent_srv._auth_token:


This feels like it should be a method on the server object to indicate if it is signed in. Looking at what you wrote, I understand that there is no "private" in python but we should honor things when they make sense. A method on the server liked signed_in() which returns a boolean would feel cleaner.

When you say "signed in" are you just defining that as having a token?
The token isn't guaranteed to be valid just because it exists (the crux of this issue) which means you aren't actually 'signed in' -- but if that's ok I can have something like that

That is what I am saying. Our logic right now is that we assume if we have a token we are signed in. You still might fail because the token expired or something else, but from the client's standpoint, we think we are signed in. The exact same check you are doing ... just move it so we are not accessing private member variables.

LGraber · Oct 14, 2016

tableauserverclient/server/endpoint/sites_endpoint.py

-        logger.info('Deleted single site (ID: {0})'.format(site_id))
+        # Deleting the site also signs you out on the Server.
+        # Here we check that you're deleting the site you are signed in to
+        # (Which is true now but may not always be) and then clear auth tokens


This comment doesn't make sense, "Which is true now". Our code does not enforce this. It might be true that this only works in this case but I think you can leave that out. Simply put if you delete the site you are signed into, you are automatically signed out.

Tableau Server enforces this, you MUST be signed into the site you are deleting or you get a 403 that says "You can't delete a resource on another site"

Okay. But the comment is confusing given where it is in the code and that level of detail is not needed unless we were doing the check before we called delete. Then it would make sense. As it is, our check is doing "If we deleted the site we are logged into, then we are automatically logged out". That is what this comment should say. If you want to add a check before we call the server, you could but I don't think it is needed right now. That check could have this comment

Got it, will update

…d fix a bug in test setup

LGraber · Oct 15, 2016

tableauserverclient/server/endpoint/sites_endpoint.py

+        # If we deleted the site we are logged into
+        # then we are automatically logged out
+        if site_id == self.parent_srv.site_id:
            self.parent_srv._clear_auth()


Add a log statement here.

LGraber · Oct 15, 2016

🚀 Just the one request for an extra line of logging. Looks good to me

Fix for issie tableau#60. Make Sign Out a no-op when you are already …

0ce4fa3

…signed out

LGraber reviewed Oct 14, 2016

View reviewed changes

Responding to CR feedback. Make a method for exposing is_signed_in an…

c8d1116

…d fix a bug in test setup

LGraber reviewed Oct 15, 2016

View reviewed changes

Adding another log statement

c186fd8

t8y8 merged commit 03d26cd into tableau:development Oct 15, 2016

t8y8 deleted the 60-delete-sign-out branch October 15, 2016 18:57

t8y8 mentioned this pull request Oct 15, 2016

Deleting a site results in a signout error #60

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue #60#61

Fix for issue #60#61
t8y8 merged 3 commits intotableau:developmenttableau/server-client-python:developmentfrom
t8y8:60-delete-sign-outt8y8/server-client-python:60-delete-sign-outCopy head branch name to clipboard

t8y8 commented Oct 14, 2016 •

edited

Loading

Uh oh!

LGraber Oct 14, 2016

Uh oh!

t8y8 Oct 14, 2016

Uh oh!

LGraber Oct 14, 2016

Uh oh!

LGraber Oct 14, 2016

Uh oh!

t8y8 Oct 14, 2016

Uh oh!

LGraber Oct 14, 2016

Uh oh!

LGraber Oct 14, 2016

Uh oh!

t8y8 Oct 14, 2016

Uh oh!

LGraber Oct 14, 2016

Uh oh!

t8y8 Oct 14, 2016

Uh oh!

LGraber Oct 15, 2016

Uh oh!

LGraber commented Oct 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

Conversation

t8y8 commented Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

LGraber commented Oct 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

t8y8 commented Oct 14, 2016 •

edited

Loading