8000 added ability to get usage statistics for views by shinchris · Pull Request #242 · tableau/server-client-python · GitHub
[go: up one dir, main page]
Skip to content

added ability to get usage statistics for views #242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Nov 20, 2017
Merged

added ability to get usage statistics for views #242

merged 7 commits into from
Nov 20, 2017

Conversation

shinchris
Copy link
Contributor

Issue #179.

t8y8
t8y8 suggested changes Oct 24, 2017
View reviewed changes
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.

A nit and add a test :)

tableauserverclient/server/endpoint/workbooks_endpoint.py Outdated
def _get_views_for_workbook(self, workbook_item):
url = "{0}/{1}/views".format(self.baseurl, workbook_item.id)
def _get_views_for_workbook(self, workbook_item, usage):
url = "{0}/{1}/views".format(self.baseurl, workbook_item.id, usage)
Copy link
Collaborator

Choose a reason for hiding this comment

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

usage doesn't need to be in the .format

t8y8
t8y8 approved these changes Oct 25, 2017
View reviewed changes
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.

🚀

These are two small rename proposals. I don't feel so strongly as to demand them, but might be a good idea.

test/test_view.py Outdated
@@ -45,6 +46,28 @@ def test_get(self):
self.assertEqual('6d13b0ca-043d-4d42-8c9d-3f3313ea3a00', all_views[1].workbook_id)
self.assertEqual('5de011f8-5aa9-4d5b-b991-f462c8dd6bb7', all_views[1].owner_id)

def test_get_usage(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

test_get_views_with_usage

test/test_workbook.py Outdated
@@ -220,6 +221,31 @@ def test_populate_views(self):
self.assertEqual('Interest rates', views_list[2].name)
self.assertEqual('RESTAPISample/sheets/Interestrates', views_list[2].content_url)

def test_populate_views_usage(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

test_populate_views_with_usage

@t8y8
Copy link
Collaborator
t8y8 commented Oct 25, 2017

🚀

t8y8
t8y8 suggested changes Oct 25, 2017
View reviewed changes
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.

WAIT

graysonarts
graysonarts suggested changes Oct 25, 2017
View reviewed changes
tableauserverclient/server/endpoint/views_endpoint.py
server_response = self.get_request(self.baseurl, req_options)
url = self.baseurl
if usage:
url += "?includeUsageStatistics=true"
Copy link
Contributor

Choose a reason for hiding this comment

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

This could lead to the url being ?includeUsageStatistics=true?page=2 if you pass in request options. Can you add a test to check that the url being generated in this case would be ?includeUsageStatistics=true&page=2 ?

You'll probably want to amend request options so that it inspects the url for the existence of ? and then uses an & to append options to the query string.

@t8y8
Copy link
Collaborator
t8y8 commented Oct 26, 2017 •
edited
Loading 
def apply_query_params(self, url):
    params = []

    if '?' in url:
        url, existing_params = url.split('?')
        params.append(existing_params)

    if self.page_number:
        params.append('pageNumber={0}'.format(self.page_number))
    if self.page_size:
        params.append('pageSize={0}'.format(self.page_size<
8000
/span>))
    if len(self.sort) > 0:
        sort_options = (str(sort_item) for sort_item in self.sort)
        ordered_sort_options = sorted(sort_options)
        params.append('sort={}'.format(','.join(ordered_sort_options)))
    if len(self.filter) > 0:
        filter_options = (str(filter_item) for filter_item in self.filter)
        ordered_filter_options = sorted(filter_options)
        params.append('filter={}'.format(','.join(ordered_filter_options)))

    return "{0}?{1}".format(url, '&'.join(params))

That works in cases where the url contains an existing parameter, and in cases where the url is normal

t8y8
t8y8 approved these changes Oct 26, 2017
View reviewed changes
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.

🚀 🚀

graysonarts
graysonarts approved these changes Nov 9, 2017
View reviewed changes
Copy link
Contributor
@graysonarts graysonarts left a comment

Choose a reason for hiding this comment

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

🚀

test/assets/view_get_usage.xml Outdated
@@ -2,14 +2,16 @@
<tsResponse xmlns="http://tableau.com/api" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://tableau.com/api http://tableau.com/api/ts-api-2.3.xsd">
<pagination pageNumber="1" pageSize="100" totalAvailable="2" />
<views>
<view id="d79634e1-6063-4ec9-95ff-50acbf609ff5" name="ENDANGERED SAFARI" contentUrl="SafariSample/sheets/ENDANGEREDSAFARI">
<view id="d79634e1-6063-4ec9-95ff-50acbf609ff5" name="foo" contentUrl="SafariSample/sheets/ENDANGEREDSAFARI">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the name change?

@shinchris shinchris merged commit fa7e568 into development Nov 20, 2017
@graysonarts graysonarts deleted the usage-stats branch January 15, 2018 21:40
@graysonarts graysonarts mentioned this pull request Jan 16, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@graysonarts graysonarts graysonarts approved these changes

@t8y8 t8y8 t8y8 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shinchris @t8y8 @graysonarts
0