Facebook test client WIP, campaigns stream only #231

bhtowles · 2023-10-24T15:09:46Z

Description of change

Related to TDL-7577 to support pagination test
Facebook test client WIP, campaigns stream only

Manual QA steps

Risks

Rollback steps

revert this branch

tests/test_client.py

…ient

luandy64 · 2023-10-25T17:49:09Z

tests/test_client.py

+        self.api_version = 'v18.0'
+        self.account_id = os.getenv('TAP_FACEBOOK_ACCOUNT_ID')
+        self.access_token  = os.getenv('TAP_FACEBOOK_ACCESS_TOKEN')
+        self.account_url = self.base_url + self.api_version +'/act_{}'.format(self.account_id)


We should try to move away from .format() calls with new code. Every modern python version supports F-strings and any version that doesn't is deprecated

Suggested change

self.account_url = self.base_url + self.api_version +'/act_{}'.format(self.account_id)

self.account_url = f"{self.base_url}/{self.api_version}/act_{self.account_id}"

This also lets us make the decision that no piece of the url should have a trailing or leading slash and we can let whatever code builds a full URL add the slashes in for us.

Also, if we never separate 'https://graph.facebook.com/' from 'v18.0', then I think it's safe to say that the base_url is 'https://graph.facebook.com/v18.0'.

It makes the "build a full URL" lines shorter

It makes it harder to deviate from the one API version this client uses

Not sure if we should tie to one api version so updated accordingly

luandy64 · 2023-10-25T17:54:18Z

tests/test_client.py

+
+    def get_account_objects(self, stream):
+        assert stream in  self.stream_endpoint_map.keys(), \
+            f'Endpoint undefiend for specified stream: {stream}'


Suggested change

f'Endpoint undefiend for specified stream: {stream}'

f'Endpoint undefined for specified stream: {stream}'

luandy64 · 2023-10-25T17:56:31Z

tests/test_client.py

+        response = requests.get(url, params)
+        LOGGER.info(f"Returning get response: {response}")
+        return response.json()


The best practices for the requests library is

response = requests.get() response.raise_for_status() response.json()

https://requests.readthedocs.io/en/latest/user/quickstart/#response-status-codes

added the raise_for_status

Does this mean my status code check in the pagination test is now redudnant?

I don't think so. This client is just for the CRUD portion right?

And the tap is still the thing we are testing to see if pagination is working?

…insights

…ook into qa/api-eval-TDL-7577

HarrisonMarcRose · 2023-11-02T20:12:39Z

tests/test_facebook_pagination.py

+            response = fb_client.get_account_objects(stream)
+
+            number_of_records = len(response['data'])
+            if number_of_records >= limit and response.get('paging', {}).get('next'):


I would think for pagination to occur the number of records cannot be equal to the limit. If the limit is 100 and there are 100 then it won't go to a second page.

I'm not clear on what response.get('paging', {}).get('next'): does?

It appears as if the limit sent in the get request controls the number of records returned per page so I don't expect to get greater than 'limit' records back in a single response. I guess we should change this from ">=" to just "==". The "response.get('paging', {}).get('next'):" verifies that there is another page of data still left to get.

This logic should be in the get_account_objects. When the if statement is true we just continue to the next stream. Are we assuming because there is a next page, we have another record and are over the limit.

HarrisonMarcRose · 2023-11-02T20:15:39Z

tests/test_client.py

+    def get_account_objects(self, stream):
+        assert stream in  self.stream_endpoint_map.keys(), \
+            f'Endpoint undefined for specified stream: {stream}'
+        endpoint = self.stream_endpoint_map[stream]
+        url = self.account_url + endpoint
+        params = {'access_token': self.access_token,
+                  'limit': 100}
+        LOGGER.info(f"Getting url: {url}")
+        response = requests.get(url, params)
+        response.raise_for_status()
+        LOGGER.info(f"Returning get response: {response}")
+        return response.json()


It looks like this will only make one request and not continue until we either get to the end of the records or (have enough to make sure we don't need more data). Should this be in a loop until we have a certain number of records or until there are no more records?

Also, I don't see where we set a start date. what if all the records are ones that we wouldn't get in the sync? I'm not sure how the API works but we should make sure we are getting the correct set of data and continue to paginate correctly ourselves. Do we need to specify a sort order for example?

Good point. Had not considered how limit, start_date, and order could change things. In general the idea was that only one request for 'limit' items would be needed and we could inspect the response to see if it had 'limit' records and a next page of data to know if there was enough there to paginate. But this assumes the limit is always 100 and the start date is always whatever the API default is. Once the start date differs from this then order will also matter. Will look into passing 'limit' and start date to the request and see if there is a way to dictate order.

Updated to pass limit and date range to the get query.

HarrisonMarcRose · 2023-11-02T20:20:06Z

tests/test_client.py

+        LOGGER.info(f"Returning post response: {response}")
+        return response
+
+    def generate_post_params(self, stream):


This is more than enough for now if we have random pk values. For an all fields test we would need to expand this and it could get messy. Might need to think about that more in the future, but not necessary now.

Also, to think about for the future (and maybe for the pagination test) is if we should store and return the data that we get and create to compare to what the sync gets and to make sure it is getting the correct data. Not sure if our standard pagination test worries about that but probably not since the data is unknown ahead of time.

…get request

HarrisonMarcRose

I do have some comments it would be good to implement but even without fixing those this is good. Love that we are making progress on this.

tests/base_new_frmwrk.py

tests/test_facebook_pagination.py

HarrisonMarcRose · 2023-11-14T14:34:22Z

tests/test_facebook_pagination.py

+            response = fb_client.get_account_objects(stream)
+
+            number_of_records = len(response['data'])
+            if number_of_records >= limit and response.get('paging', {}).get('next'):


This logic should be in the get_account_objects. When the if statement is true we just continue to the next stream. Are we assuming because there is a next page, we have another record and are over the limit.

tests/test_facebook_pagination.py

…te BaseCase page_size(), update self.start_date pattern

Facebook test client WIP, campaigns stream only

f9bb1ef

bhtowles added the testing QA work. No src code changes. label Oct 24, 2023

bhtowles commented Oct 24, 2023

View reviewed changes

tests/test_client.py Outdated Show resolved Hide resolved

Add pagination test for campaigns stream, update new base file and cl…

5d6ff16

…ient

bhtowles requested a review from HarrisonMarcRose October 24, 2023 21:10

luandy64 reviewed Oct 25, 2023

View reviewed changes

bhtowles and others added 8 commits October 25, 2023 19:19

First review comments, add adsets stream, start work on ads

05cfc4b

Merge branch 'master' into qa/api-eval-TDL-7577

20d37ba

Support for adcreative, ads, adsets, and campaigns, get only for ads_…

0356413

…insights

Merge branch 'qa/api-eval-TDL-7577' of github.com:singer-io/tap-faceb…

78004a6

…ook into qa/api-eval-TDL-7577

Merge branch 'master' into qa/api-eval-TDL-7577

8af6f4d

Clean up for next review round

e497066

Merge branch 'qa/api-eval-TDL-7577' of github.com:singer-io/tap-faceb…

810f602

…ook into qa/api-eval-TDL-7577

PR review, clean up for further review

826b505

HarrisonMarcRose reviewed Nov 2, 2023

View reviewed changes

bhtowles added 3 commits November 9, 2023 21:11

PR review comments round 2, pass limit and date range to test client …

01ad519

…get request

Convert times to utc to compare all datetime objects as tz naive

6753872

Use base_suite parse_date

668e165

HarrisonMarcRose approved these changes Nov 14, 2023

View reviewed changes

Final review comments, add TODO to update get_account_objects(), upda…

5e8ef41

…te BaseCase page_size(), update self.start_date pattern

bhtowles merged commit add18f1 into master Nov 15, 2023
2 checks passed

bhtowles deleted the qa/api-eval-TDL-7577 branch November 15, 2023 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Facebook test client WIP, campaigns stream only #231

Facebook test client WIP, campaigns stream only #231

bhtowles commented Oct 24, 2023

luandy64 Oct 25, 2023

luandy64 Oct 25, 2023

bhtowles Oct 25, 2023

luandy64 Oct 25, 2023

bhtowles Oct 25, 2023

luandy64 Oct 25, 2023

luandy64 Oct 25, 2023

bhtowles Oct 25, 2023

bhtowles Oct 25, 2023

luandy64 Oct 25, 2023

HarrisonMarcRose Nov 2, 2023

HarrisonMarcRose Nov 2, 2023

bhtowles Nov 2, 2023

HarrisonMarcRose Nov 14, 2023

HarrisonMarcRose Nov 2, 2023

bhtowles Nov 2, 2023

bhtowles Nov 9, 2023

HarrisonMarcRose Nov 2, 2023

bhtowles Nov 2, 2023

HarrisonMarcRose left a comment

HarrisonMarcRose Nov 14, 2023

	self.account_url = self.base_url + self.api_version +'/act_{}'.format(self.account_id)
	self.account_url = f"{self.base_url}/{self.api_version}/act_{self.account_id}"

	f'Endpoint undefiend for specified stream: {stream}'
	f'Endpoint undefined for specified stream: {stream}'

Facebook test client WIP, campaigns stream only #231

Facebook test client WIP, campaigns stream only #231

Conversation

bhtowles commented Oct 24, 2023

Description of change

Manual QA steps

Risks

Rollback steps

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

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

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

Choose a reason for hiding this comment

HarrisonMarcRose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment