Skip to content
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.

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

Make access request redirect to dashboard if user already has access #4469

Conversation

timifasubaa
Copy link
Contributor

@timifasubaa timifasubaa commented Feb 23, 2018

With the access request feature, users who have access often revisit the access request page and are still shown the access request page.
This PR checks to make sure the user still doesn't have access before showing the access request page and redirects to the dashboard if they have access.

@john-bodley @mistercrunch

@timifasubaa timifasubaa force-pushed the check_for_access_before_requesting_access branch 2 times, most recently from bc33642 to d1f004a Compare February 23, 2018 01:30
@timifasubaa timifasubaa changed the title Make access request redirect to dashboard Make access request redirect to dashboard if user already has access Feb 23, 2018

has_access = True
for datasource in datasources:
has_access &= (datasource and self.datasource_access(datasource))
Copy link
Member

Choose a reason for hiding this comment

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

I find the all and any functions much more readable than the &=, something like:
has_access = all((self.datasource_access(o) for o in datasources))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@timifasubaa timifasubaa force-pushed the check_for_access_before_requesting_access branch 2 times, most recently from 8a21996 to fc54600 Compare February 24, 2018 01:38
(
datasource and self.datasource_access(datasource)
for datasource in datasources
),
Copy link
Member

@mistercrunch mistercrunch Feb 25, 2018

Choose a reason for hiding this comment

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

Watch out here, I'm pretty sure this dangling comma changes the structure from passing a tuple to all to, instead, passing it a tuple of one element that contains a tuple.

Copy link
Member

Choose a reason for hiding this comment

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

It's a pretty common gotcha in python, v = 1, makes v a tuple

@timifasubaa
Copy link
Contributor Author

timifasubaa commented Feb 26, 2018

@mistercrunch Looks like it's a quirk with Python consistency. I initially didn't have trailing comma but the flake8 rules required me to add it and somehow python reads it correctly. Also, the check is done correctly.

From my interpreter:

>>> all((True, True, True),)
True
>>> all((True, True, False),)
False
>>> all((True, True, False))
False
>>> v = 1,
>>> v
(1,)

I removed the comma anyway.

@timifasubaa timifasubaa force-pushed the check_for_access_before_requesting_access branch from fc54600 to fba8b93 Compare February 26, 2018 23:49
@timifasubaa
Copy link
Contributor Author

PING

@mistercrunch
Copy link
Member

Yeah I ran similar tests in an interpreter and still I'm confused as to how this works properly. I agree we cannot rely on this odd behavior as it may flap in the future or across python versions. LGTM

@mistercrunch mistercrunch merged commit 8626793 into apache:master Feb 28, 2018
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.24.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.24.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants