Secret key #264
Secret key #264
Conversation
This is only for plot (not plot for now) + correct some pep8
| @@ -1246,9 +1274,13 @@ def _send_to_plotly(figure, **plot_options): | |||
| url = get_config()['plotly_domain'] + "/clientresp" | |||
|
|
|||
| r = requests.post(url, data=payload, | |||
| verify=get_config()['plotly_ssl_verification']) | |||
| verify=get_config()['plotly_ssl_verification'],) | |||
There was a problem hiding this comment.
did you mean to add this?
|
The general setup for this looks good to me. Sort of a bummer to have an extra request but it's the safest for all users (enterprise included) Every time we make a change to in streambed, we need to consider when certain users will actually receive that update. When we start wrapping the other new endpoints I'd really like to move to a more RESTful design which will make this a bit smoother. Ping me when you want a final review of it : D |
both plot/ iplot can activate share_key
|
@theengineear @chriddyp I've added the share_key for iplot but I wasn't able to fix the error. The error happens on https://github.com/plotly/python-api/blob/master/plotly/plotly/plotly.py#L829-L831 I am not too happy about |
| warnings.warn( | ||
| "Looks like you've activated the share_key for a public plot. " | ||
| "share_key can only be activaited for private plots." | ||
| "\nQuestions? support@plot.ly") |
There was a problem hiding this comment.
🐄 This isn't your fault, but should really be changed. support@plot.ly is intended for enterprise-related emails, not for the general public consumers. We should really be pointing users to feedback@plot.ly. We do this in a number of places.
@chriddyp actually, we could be a bit sneaky and return different supporty emails based on whether the plotly_domain matches the default plotly_domain in the session config? too sneaky? worthwhile?
added six.moves.urllib.parse.urlparse
|
@theengineear Hey thanks for your help and reviews! I wanted to keep you in the loop, last night @chriddyp had the idea to create a new key , |
|
would the new key be a python library thing or something that's going to be added to the backend api? |
|
python library |
|
do you mean |
|
yeah, that seems like a pretty user-friendly way to do it. one concern is keeping backwards compat with the |
|
yes, my bad I meant |
|
@theengineear @chriddyp @cldougl |
|
sweet, sounds good then! |
|
re: #264 (comment) try merging in master (you'll need to fix some conflicts) and then pushing back up. I think that will be resolved. |
Merge branch 'master' into secret_key
|
looks great to me!
|
|
(and after this gets merged, lets update the appropriate documentation: the getting started and the privacy section) |
|
Re: #264 (comment) , @aneda can you either open a PR for this or open an issue in the 1.7 milestone so we don't forget? |
|
@chriddyp 's comment here still needs addressing: https://github.com/plotly/python-api/pull/264/files#r36436323 |
| if (key == 'sharing' and not (kwargs[key] in SHARING_OPTIONS)): | ||
| raise exceptions.PlotlyError("'{0}' must be of either '{1}', '{2}'" | ||
| " or '{3}'" | ||
| .format(key, *SHARING_OPTIONS)) |
|
non-blocking from me, i think there's a comment from chris that still needs to be checked and some general code style things that could be improved. Feel free to 💃 after you've taken care of #264 (comment) and https://github.com/plotly/python-api/pull/264/files#r36436323 |
replaced PlotlyRequestError with requests.exceptions.HTTPError
RequestException as we have changed the response_handler
@theengineear @chriddyp @cldougl
Hey, this is a rough draft for secret_key. This only works for plot (py.plot(...)). I haven't done any tests and I am working on iplot (being stuck there). So I wanted to check if I am on the right track and this is what you had in mind. Thnaks!