Create Maps API key automagically in Kuviz #1082

simon-contreras-deel · 2019-10-09T17:39:50Z

Solves: #731

alrocar

Beyond the missing tests and so on, I added a couple of comments. Not needed to be tackled in this PR. If this is time sensitive, let's merge it and tackle everything else in a different issue.

alrocar · 2019-10-10T12:41:21Z

cartoframes/data/clients/auth_api_client.py

+    """
+    def __init__(self, credentials=None):
+        credentials = credentials or get_default_credentials()
+        self._api_key_manager = _get_api_key_manager(credentials)


We should raise an error if we are not receiving the master API key here, because otherwise it won't work the API key creation. It could be responsibility of carto-python.

Filtering the API keys with the one provided by the user to check if it is the master one? This also applies to the Kuviz creation, the user should use the master one.

I am not sure about it. Maybe we can catch Access denied exceptions and remembering the user about using the master key?

Anyway, the validation of the credentials is something to work on because we are not validating if the credentials are defined or not (we are doing the classical credentials or get_default_credentials(), but not checking if we have credentials or not after that). Maybe we can create a task with both objectives

cartoframes/viz/kuviz.py

alrocar · 2019-10-10T12:47:15Z

cartoframes/viz/kuviz.py

+    def _sync_layer(self, layer, table_name):
+        layer.source.dataset.upload(table_name=table_name, credentials=self._credentials)
+        layer.source = Source(table_name, credentials=self._credentials)
+        warn('Table `{}` created. In order to publish the map, you will need to create a new Regular API '


Every time you publish a map, you are we gonna get a bunch of warnings right? Even when we are creating the regular API key automatically...

I think we should warn the client about that, don't you?

If you publish a map with several layers, you'd get a bunch of warnings which actually might seem errors.

I think we could at most show the message once with a summary. It would be much better using a logger as well (but we are not there yet).

In any case, isn't the whole purpose of this PR to create the API key automatically? Why we still need to show this message?

In any case, isn't the whole purpose of this PR to create the API key automatically? Why we still need to show this message?

You are right, the message is wrong.

Thinking about the rest, we are showing some messages (table creation), but not others (API key creation). Furthermore, our current messages seem like errors, so we could avoid them by now.

I have added a comment about that in the logging issue: #422 (comment) and I am going to remove the current ones.

test/viz/test_kuviz.py

test/data/client/test_auth__api_client.py

test/viz/test_map.py

test/data/dataset/test_dataset.py

alrocar

Quite comprehensive tests ✨

Fixed some typos

alrocar · 2019-10-11T17:29:41Z

cartoframes/data/clients/auth_api_client.py

+    """
+    def __init__(self, credentials=None):
+        credentials = credentials or get_default_credentials()
+        self._api_key_manager = _get_api_key_manager(credentials)


alrocar · 2019-10-11T17:36:09Z

cartoframes/viz/kuviz.py

+    def _sync_layer(self, layer, table_name):
+        layer.source.dataset.upload(table_name=table_name, credentials=self._credentials)
+        layer.source = Source(table_name, credentials=self._credentials)
+        warn('Table `{}` created. In order to publish the map, you will need to create a new Regular API '


If you publish a map with several layers, you'd get a bunch of warnings which actually might seem errors.

I think we could at most show the message once with a summary. It would be much better using a logger as well (but we are not there yet).

In any case, isn't the whole purpose of this PR to create the API key automatically? Why we still need to show this message?

docs/developer-center/guides/10-Publishing.md

docs/guides/kuviz.rst

examples/05_publishing/01_publish_visualization_using_kuviz.ipynb

alrocar · 2019-10-11T17:44:39Z

Upss sorry, just saw this wasn't in the kanban yet... I just got the notification and thought it was fine to review it 🙏

Co-Authored-By: Alberto Romeu <alrocar@users.noreply.github.com>

…frames into feature/magic-apikey-kuviz

Co-Authored-By: Alberto Romeu <alrocar@users.noreply.github.com>

…frames into feature/magic-apikey-kuviz

alrocar

I continue with acceptance. @elenatorro If there are any changes needed in the docs/examples let's add comments here and tackle them in a different PR.

alrocar · 2019-10-14T12:57:19Z

Acceptance 🍏

Maybe I wasn't too comprehensive since I just validated the notebook actually does what it's expected to do.

I'm merging this one.

simon-contreras-deel added 12 commits October 9, 2019 15:34

publish use the same credentials

3978545

publish structure

68634e3

instantiate KuvizPublisher with all info

cc94622

auth api client

5165782

kuviz refactor

d218ce3

easy publish

8728491

kuviz easy get and set layers

acc1e94

updating kuviz delete and creation layers

3cc0d76

update kuviz

93f58de

using new update

8075113

fix tables usage

2ac70dc

setting api key naming

22db554

simon-contreras-deel changed the title ~~Feature/magic apikey kuviz~~ Create Maps API key automagically in Kuviz Oct 9, 2019

test auth api client

149875f

alrocar reviewed Oct 10, 2019

View reviewed changes

alrocar assigned simon-contreras-deel Oct 10, 2019

simon-contreras-deel added 7 commits October 10, 2019 20:10

udating kuviz test

25ff832

table_name is optional

3afa59a

remove tests testing mocks

dedf194

test naming

a4e3f3e

remove needless mocks

def0bc3

test publish method

c38d945

test update and delete

fcbb38e

houndci-bot reviewed Oct 10, 2019

View reviewed changes

test/viz/test_kuviz.py Outdated Show resolved Hide resolved

test/data/client/test_auth__api_client.py Outdated Show resolved Hide resolved

simon-contreras-deel added 6 commits October 11, 2019 11:56

ensuring working with layer copy

3676ff1

unique name for api key

2f0d06a

ensure table name

01d228c

saving and changing branch

7eeeb0e

Merge branch 'develop' into feature/magic-apikey-kuviz

3837ce8

tests several geoms columns and geodataframes

96be37a

simon-contreras-deel added 3 commits October 11, 2019 15:28

final notebook

0bc118b

updating map test behaviour

f264a8c

updating doc

dbc9b69

houndci-bot reviewed Oct 11, 2019

View reviewed changes

test/viz/test_map.py Outdated Show resolved Hide resolved

test/data/dataset/test_dataset.py Outdated Show resolved Hide resolved

test/data/dataset/test_dataset.py Outdated Show resolved Hide resolved

simon-contreras-deel added 4 commits October 11, 2019 16:31

hound

e16663a

removeing forgotten line

d477c00

more hound happiness

4343a16

updating guides

89619f4

simon-contreras-deel requested a review from elenatorro October 11, 2019 15:40

alrocar mentioned this pull request Oct 11, 2019

Validate credentials library wise #1097

Closed

alrocar suggested changes Oct 11, 2019

View reviewed changes

simon-contreras-deel and others added 8 commits October 13, 2019 16:19

needless import

d31e7f7

messaging in Kuviz publication and update

98c087d

improve message

a57fa18

Co-Authored-By: Alberto Romeu <alrocar@users.noreply.github.com>

Merge branch 'feature/magic-apikey-kuviz' of github.com:CartoDB/carto…

884ad08

…frames into feature/magic-apikey-kuviz

Improve message 2

82199f8

Co-Authored-By: Alberto Romeu <alrocar@users.noreply.github.com>

Merge branch 'feature/magic-apikey-kuviz' of github.com:CartoDB/carto…

9e271b9

…frames into feature/magic-apikey-kuviz

Improve message 3

43adc4f

Merge branch 'develop' into feature/magic-apikey-kuviz

b42afd9

simon-contreras-deel requested a review from alrocar October 14, 2019 08:51

simon-contreras-deel removed their assignment Oct 14, 2019

Update docs/guides/kuviz.rst

aa7784b

alrocar approved these changes Oct 14, 2019

View reviewed changes

alrocar merged commit a95185b into develop Oct 14, 2019

alrocar deleted the feature/magic-apikey-kuviz branch October 14, 2019 12:58

simon-contreras-deel mentioned this pull request Oct 14, 2019

Create Maps API key automagically in Sharing visualizations #731

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Maps API key automagically in Kuviz #1082

Create Maps API key automagically in Kuviz #1082

simon-contreras-deel commented Oct 9, 2019 •

edited

Loading

alrocar left a comment

alrocar Oct 10, 2019

simon-contreras-deel Oct 11, 2019

simon-contreras-deel Oct 11, 2019 •

edited

Loading

alrocar Oct 11, 2019

alrocar Oct 10, 2019

simon-contreras-deel Oct 11, 2019

alrocar Oct 11, 2019

simon-contreras-deel Oct 13, 2019

alrocar left a comment

alrocar Oct 11, 2019

alrocar Oct 11, 2019

alrocar commented Oct 11, 2019

alrocar left a comment

alrocar commented Oct 14, 2019

Create Maps API key automagically in Kuviz #1082

Create Maps API key automagically in Kuviz #1082

Conversation

simon-contreras-deel commented Oct 9, 2019 • edited Loading

alrocar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon-contreras-deel Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrocar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrocar commented Oct 11, 2019

alrocar left a comment

Choose a reason for hiding this comment

alrocar commented Oct 14, 2019

simon-contreras-deel commented Oct 9, 2019 •

edited

Loading

simon-contreras-deel Oct 11, 2019 •

edited

Loading