Review subscription messages #1391
Conversation
| @@ -110,9 +107,6 @@ def _get_print_id(self): | |||
| return self.id | |||
|
|
|||
| def _download(self, file_path, credentials): | |||
| if not self._is_available_in('bq'): | |||
There was a problem hiding this comment.
You can have access to DO, but the Dataset or Geography not been ready in BigQuery. How are we checking that?
There was a problem hiding this comment.
This comes from #1331 (comment). We are checking this with the do/subscriptions call after the download. However, I'll check this in the acceptance test.
There was a problem hiding this comment.
@arredond @andy-esch do you think we should keep this check? For the situation when a dataset is "not ready"?
There was a problem hiding this comment.
About the comment about the duplicated check, we were checking if the dataset/geo has a price which is done by the backend too.
| bq_client.download_to_file(job, file_path, column_names=column_names) | ||
| except NotFound: | ||
| raise CartoException('You have not purchased the dataset `{}` yet'.format(self.id)) | ||
| bq_client.download_to_file(job, file_path, column_names=column_names) |
There was a problem hiding this comment.
Same here,
You can have access to DO, but not to a Dataset or Geography. How are we checking that?
There was a problem hiding this comment.
This is checked also in the do/subscriptions call, after the download.
There was a problem hiding this comment.
Yes, you are right. Since we are checking if the user is subscribed, ideally we can avoid the try-except. In any case, I will keep it as I tell you offline (but this is opinionable)
|
This requires CartoDB/carto-python#159 to be merged. I have tested manually the features and all the changes work OK 🍏 About this: #1391 (comment). do we keep the "is available in bq" check in the client? cc @cmongut @arredond @andy-esch |
Yes, we need it |
This reverts commit a94f23a.
|
OK, changes done. |
Jesus89
force-pushed
the
1155-review-subscription-messages
branch
from
a5a798b to
706a771
Compare
|
Actually, I'm not fully convinced we need that check @oleurud. I understand these checks were in place to satisfy a certain condition:
After what we talked in "Data Observatory next steps" meeting last Tuesday, it seems that BigQuery is going to be the single source of truth for all datasets, so any dataset must be imported first into BQ and then copied to other services. That means that the above case would never happen. In any case, if we were going to keep performing this check (which I don't think is wrong, just maybe unnecessary), it should be performed via the future API and not validating the client's local metadata, which could be modified by the own client. Does this make sense @alasarr ? |
|
Then it could be interesting to know if the lib should check it thinking in future versions of CARTOframes (if it is not needed, we could want to remove available_in from the catalog and we would break previous versions).
|
Closes #1155, #1331.
estimated_delivery_days