Tdl 19157 linkedin oauth add refresh token #41
Conversation
nitingaikwad1
requested review from
RushiT0122,
kethan1122,
KrisPersonal,
sgandhi1311 and
shantanu73
tap_linkedin_ads/__init__.py
Outdated
| state = {} | ||
| if parsed_args.state: | ||
| state = parsed_args.state | ||
|
|
||
| config = parsed_args.config |
There was a problem hiding this comment.
If we do this above L34, we could just used the config instead of parsed_args.config there as well.
There was a problem hiding this comment.
yes , i have same changes
tap_linkedin_ads/client.py
Outdated
| Server5xxError, | ||
| max_tries=5, | ||
| factor=2) | ||
| def get_access_token(self): |
There was a problem hiding this comment.
The name get_access_token as a method on a class insinuates that it would return an access token. What would you think about naming this something like fetch_and_set_access_token or something of the like? A little more wordy, but it describes the operation a little more clearly.
There was a problem hiding this comment.
correct i have renamed the method name
tap_linkedin_ads/client.py
Outdated
| if not self.__verified: | ||
| self.__verified = self.check_access_token() | ||
| self.__verified = self.get_access_token() |
There was a problem hiding this comment.
It's possible I might be missing something, but I don't think get_access_token returns a value. Also, would this do anything since we call self.get_access_token() on L268? If it wasn't verified then, would it be verified now?
There was a problem hiding this comment.
yes it does not return anything , i have corrected it
tap_linkedin_ads/client.py
Outdated
| @@ -141,12 +149,46 @@ def __init__(self, | |||
| max_tries=5, | |||
| factor=2) | |||
| def __enter__(self): | |||
| self.__verified = self.check_access_token() | |||
| self.__verified = self.get_access_token() | |||
There was a problem hiding this comment.
I don't think get_access_token returns anything here, so I don't think self.__verified will ever be set to anything.
I also don't think we use check_access_token anymore. Do we still need that function?
tap_linkedin_ads/client.py
Outdated
| if not self.__verified: | ||
| self.__verified = self.check_access_token() | ||
| self.__verified = self.fetch_and_set_access_token() |
There was a problem hiding this comment.
fetch_and_set_access_token is still not returning anything, so I don't think this would work. I think it needs to return what we want to se __verified to, or set __verified in the function.
We also call fetch_and_set_access_token just above in L241. Do we want to call this regardless of whether or not it's been verified? If so, I think we could remove the check here. If not, I think we should remove the call at L241. I believe we only want it to call it when we're not verified, but I could be mistaken.
Description of change
Generate access token programmatically using refresh token
Manual QA steps
Risks
Rollback steps