-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Chris/remove redirect from slice id endpoint #1044
Chris/remove redirect from slice id endpoint #1044
Conversation
if not (all_datasource_access or datasource_access): | ||
flash(__("You don't seem to have access to this datasource"), | ||
"danger") | ||
return redirect(error_redirect) | ||
|
||
action = request.args.get('action') | ||
# handle slc / viz obj | ||
slice_id = slice_id if slice_id != None else request.args.get("slice_id") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pythonesque coalesce:
silce_id = slice_id or request.args.get('slice_id')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this work for slice_id = 0
because slice_id is actually a string? thinking too much in JS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do int(request.args.get("slice_id"))
e7e4a2f
to
64b1f99
Compare
Code looks good, but Travis says no :( |
yeah js linting error. didn't change any js so will look into what's up. |
64b1f99
to
35acfd9
Compare
… redirecting to it.
… to avoid logging errors. add fix for MultiDict bug
…source_id/slice_id/
35acfd9
to
af2fc94
Compare
@mistercrunch
I used the approach we discussed yesterday for avoiding a re-direct for the
/caravel/slice/id
endpoint. It required a bit of refactoring of theexplore
method but not too bad. Overall if aslice_id
is passed as part of the URL pathname I'll use that, then fall back to the URL query, else None.In the case that a
slice_id
is found I update the database slice params with any specified in the URL. I coerce the updated param dict to aImmutableMultiDict
so that its type is consistent withrequest.args
. I ran into a reported MultiDict bug wherein if you initialize anImmutableMultiDict
with a dict containing a key whose value is an empty array, if you try to access that key, anIndexError
is thrown. I added a try catch for this where I was seeing errors. eg:md = MultiDict({ groupby: [] })
md.keys() # returns ['groupby']
md['groupby'] # IndexError
md.get('groupby') # IndexError
As noted I also found another bug where slices that have been over-written are saved with the original
slice_id
which caused weird bugs in the forms. I fixed this in thesave_slice_as
method and explicitly set theslice_id
in theslice_params
I use to the value passed in the URL.Updated tests / they all passed.