-
Notifications
You must be signed in to change notification settings - Fork 16
1106 saas config shopify access endpoints #1220
Conversation
data/saas/config/shopify_config.yml
Outdated
requests: | ||
read: | ||
method: GET | ||
path: /admin/api/2022-07/customers/<customer_id>/orders.json?status=any |
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.
Can you move status=any
into query_params?
query_params:
- name: status
value: any
from fidesops.ops.schemas.redis_cache import PrivacyRequestIdentity | ||
from fidesops.ops.task import graph_task | ||
from tests.ops.graph.graph_test_util import assert_rows_match | ||
|
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.
Let's add a connection test here, look at test_datadog_task.py
for an example. I think it's good to add this so we know if failures are caused by a connectivity issue vs a deeper problem.
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.
I see, that's a nice test to know connectivity issue before diving into other endpoints.
- name: access_token | ||
connector_param: access_token | ||
data_path: articles | ||
- name: blog_article_comments |
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.
This is missing data_path: comments
, the data we are receiving is in the form
{
"comments": [{},{},...],
"comments": [{},{},...],
...
We need to "unwrap" it so we are just left with a single list of comment objects.
requests: | ||
read: | ||
method: GET | ||
path: /admin/api/2022-07/comments.json |
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.
This is returning comments for emails other than just the email identity, we need to add a filter and an assertion to make sure that we are only returning comments for our identity email.
configuration: | ||
source: headers | ||
rel: next | ||
- name: customer_addresses |
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.
Need to unwrap this further by adding data_path: addresses
.
configuration: | ||
source: headers | ||
rel: next | ||
- name: customer_order_transactions |
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.
Missing data_path: transactions
configuration: | ||
source: headers | ||
rel: next | ||
- name: customer_orders |
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.
Missing data_path: orders
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.
A few endpoints are missing the data_path
field which are causing the dataset and assertions to be incorrect. I'll stop the review here for now to start the fixes as soon as possible.
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.
Thanks for making the requested changes, approved!
Purpose
Changes
Checklist
CHANGELOG.md
fileCHANGELOG.md
file is being appended toUnreleased
section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.Run Unsafe PR Checks
label has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #1106