Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

277 update group feature#279

Merged
t8y8 merged 6 commits intotableau:developmenttableau/server-client-python:developmentfrom
sotnich:277-update_group-featuresotnich/server-client-python:277-update_group-featureCopy head branch name to clipboard
Apr 19, 2018
Merged

277 update group feature#279
t8y8 merged 6 commits intotableau:developmenttableau/server-client-python:developmentfrom
sotnich:277-update_group-featuresotnich/server-client-python:277-update_group-featureCopy head branch name to clipboard

Conversation

@sotnich
Copy link
Contributor

@sotnich sotnich commented Mar 23, 2018

Adding support for update method for groups.

@sotnich sotnich mentioned this pull request Mar 23, 2018
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 pretty good -- can you update the tests as well? A mocked PUT like we do for other updates should be OK.

We'll want @d45 or @aaroncarey to take a look at the doc updates, I think we need more text explaining that for a 'local' group this is a traditional update, but for an AD group it triggers a synchronization job.

logger.info('Deleted single group (ID: {0})'.format(group_id))

@api(version="2.0")
def update(self, group_item, default_site_role=UserItem.Roles.Unlicensed):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pull UserItem.Roles.Unlicensed into a constant UNLICENSED_USER or something a little shorter, it helps this read a little easier for me

@t8y8
Copy link
Collaborator

t8y8 commented Mar 27, 2018

There's a linter error in the tests, but it looks good otherwise, once it's passing checks I'll approve. I'll ping the team to have another set of eyes take a look as well.

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.

🚀

@sotnich
Copy link
Contributor Author

sotnich commented Apr 19, 2018

@t8y8, what do you think about merge this PR?

@t8y8
Copy link
Collaborator

t8y8 commented Apr 19, 2018

I will merge this PR, but we realized that docs changes need to go to the gh-pages branch directly now. Can you open a PR there? @d45 and @RussTheAerialist can help if you have questions there.

We'll be removing the docs from this branch to make it less confusing in the future :)

@t8y8 t8y8 merged commit 18372ba into tableau:development Apr 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.