1183 saas connectors shopify erasure #1289
Conversation
…ectors-shopify-erasure
…ectors-shopify-erasure
…yca/fidesops into 1183-saas-connectors-shopify-erasure
…ectors-shopify-erasure
ghost
requested a review
from 
There was a problem hiding this comment.
generally looks good @HamzaWaseemOnBench , just a few questions that i wanted to check on before merging. happy to chat about it offline if that's easier!
also - please update the CHANGELOG.md (after merging or rebasing off of main), as we'll need that before we can merge the PR.
lastly, do we have the shopify credentials in vault already? if so, let's give this a Run Unsafe PR Checks label so that the full integration test suite runs against it, as it hits external systems.
| ) -> Generator: | ||
| """ | ||
| Creates a dynamic test data record for erasure tests. | ||
| Yields customer ID as this may be useful to have in test scenarios |
There was a problem hiding this comment.
nit: i'd probably update this comment since it's more than just customer ID that's yielded
There was a problem hiding this comment.
Nice catch, I'll update it too in new commit.
| assert x == { | ||
| f"{dataset_name}:customers": 1, | ||
| f"{dataset_name}:blogs": 0, | ||
| f"{dataset_name}:customer_orders": 1, | ||
| f"{dataset_name}:customer_addresses": 1, | ||
| f"{dataset_name}:blog_articles": 0, | ||
| f"{dataset_name}:blog_article_comments": 1, | ||
| f"{dataset_name}:customer_order_transactions": 0, | ||
| } |
There was a problem hiding this comment.
i assume that the updates take too long on the source system (i.e. on the shopify platform) to actually query to make sure our data gets updated - but just want to check.
There was a problem hiding this comment.
So here we were first going for only getting existing data in erasure fixture instead of creating new(like we are doing for blogs endpoint). Since after first test data was masked already, quering it again to check if it is masked didn't make sense at that time that is why we didn't add actual query check. Since now as we have changed our approach after discussion and we are actually creating some new data(and deleting it after fixture) it'll be good to add actual query checks, I'll add it now.
| # Deleting order and article after verifying update request | ||
| order_delete_response = requests.delete( | ||
| url=f"{base_url}/admin/api/2022-07/orders/{order_id}.json", | ||
| headers=headers, | ||
| ) | ||
| assert order_delete_response.ok | ||
| article_delete_response = requests.delete( | ||
| url=f"{base_url}/admin/api/2022-07/articles/{article_id}.json", | ||
| headers=headers, | ||
| ) | ||
| assert article_delete_response.ok |
There was a problem hiding this comment.
maybe i'm mising something, but is there a reason we don't delete/clean up the customer that we've created for the test here?
There was a problem hiding this comment.
Yes basically we don't have a delete customer endpoint in Shopify API, that is why we can't delete newly created customer.
There was a problem hiding this comment.
hmm, OK - so do we end up just creating a new customer every time we run a test? could that get unwieldy after a while? just wondering if shopify provides any insight into whether having too many customers could become a problem. i guess it's something they should be able to handle if they don't offer any API to delete a customer.
anyway, not a blocker, especially since i don't think there's a whole lot we can do about this. but i am a little bit concerned we may start running into problems after months of running tests and creating customers each time.
There was a problem hiding this comment.
I had discussion with Adrian related to it too and we were rooting for approach of not creating customer in every call first too and get already created customer only but we had a problem that data was already masked just after our first call and there was practically no use of our assertions for it so we decided to go for creating customer instead. Since we created a new Shopify store with no data and there is no limit of creating customers as far as we researched so we opted for this solution.
There was a problem hiding this comment.
got it - thanks for walking through that @HamzaWaseemOnBench !
ghost
added
the
* Initial commit for erasure endpoints * Updated endpoints and erasure data approach * Formatting before making changes * Fixing duplicate customer_addresses * Added assertions for update verification * Fixed isort issue Co-authored-by: Hamza W <[email protected]> Co-authored-by: Adrian Galvan <[email protected]>
Purpose
Changes
Checklist
CHANGELOG.mdfileCHANGELOG.mdfile is being appended toUnreleasedsection 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 Checkslabel has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #1183