Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

TDL-26718: Implement token chaining #123

Merged
merged 18 commits into from
Jan 10, 2025
Merged

TDL-26718: Implement token chaining #123

merged 18 commits into from
Jan 10, 2025

Conversation

prijendev
Copy link

@prijendev prijendev commented Nov 27, 2024 •
edited
Loading

Description of change

  • Token chaining to preserve the access_token.
    • If the token is older than 7 days, generate and preserve a new token.
  • Modify tap-tester suits that utilize tokens from the previous connection.
    • Rename all tests to tap_tester_square_sandbox_tests and tap_tester_square_prod_tests according to env.

Manual QA steps

  • Verify that tap generates and saves a new token when it is not provided
  • Verify that tap uses and validates existing access tokens.
  • Verify that the tap generates and saves a new token when the already present token is over 7 days.

Risks

Rollback steps

  • revert this branch

AI generated code

https://internal.qlik.dev/general/ways-of-working/code-reviews/#guidelines-for-ai-generated-code

  • this PR has been written with the help of GitHub Copilot or another generative AI tool

@prijendev prijendev changed the base branch from master to TDL-14952-Remove-roles-stream November 27, 2024 08:46
@prijendev prijendev changed the title Tdl 26718 preserve access token TDL-26718 preserve access token Dec 12, 2024
@prijendev prijendev changed the title TDL-26718 preserve access token TDL-26718: preserve access token Dec 12, 2024
@prijendev prijendev force-pushed the TDL-26718-preserve-access-token branch from c9e942f to eef630c Compare December 12, 2024 12:21
@prijendev prijendev marked this pull request as ready for review December 13, 2024 04:00
@prijendev prijendev changed the title TDL-26718: preserve access token TDL-26718: Implement token chaining Dec 13, 2024
RushiT0122
RushiT0122 reviewed Dec 23, 2024
View reviewed changes
tap_square/client.py Outdated Show resolved Hide resolved
RushiT0122
RushiT0122 reviewed Dec 23, 2024
View reviewed changes
tests/base.py Outdated Show resolved Hide resolved
RushiT0122
RushiT0122 reviewed Dec 23, 2024
View reviewed changes
tests/test_client.py Outdated Show resolved Hide resolved
RushiT0122
RushiT0122 reviewed Dec 23, 2024
View reviewed changes
tests/test_client.py Outdated Show resolved Hide resolved
RushiT0122
RushiT0122 reviewed Dec 23, 2024
View reviewed changes
tests/unittests/test_refresh_access_token.py Outdated
@patch("singer.utils.now")
def test_expired_access_token(self, mock_now, mock_timer):
"""Return true when the token is expired."""
mock_now.return_value = utils.strptime_with_tz("2024-12-24T12:23:41Z")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of hard-coded dates we should use relative dates to current dates.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done with unit tests update

RushiT0122
RushiT0122 requested changes Dec 23, 2024
View reviewed changes
Copy link

@RushiT0122 RushiT0122 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some suggestions in-line.

@prijendev prijendev requested a review from RushiT0122 December 26, 2024 11:34
RushiT0122
RushiT0122 reviewed Jan 8, 2025
View reviewed changes
tap_square/client.py Outdated Show resolved Hide resolved
RushiT0122
RushiT0122 reviewed Jan 8, 2025
View reviewed changes
tests/unittests/test_refresh_access_token.py Outdated

@patch("singer.http_request_timer")
def test_expired_access_token(self, mock_timer):
"""Return true when the token is expired."""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Token expires in 30 days but square recommends to refresh it after 7 days so I think this doc string is misleading.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

RushiT0122
RushiT0122 reviewed Jan 8, 2025
View reviewed changes
tests/unittests/test_refresh_access_token.py Outdated

@patch("singer.http_request_timer")
def test_almost_expired_access_token(self, mock_timer):
"""Return false when the token is exact just 7 days ago"""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some typo in this doc string?

Suggested change
"""Return false when the token is exact just 7 days ago"""
"""Return false when the token is created exactly 7 days ago"""

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated comment

RushiT0122
RushiT0122 requested changes Jan 8, 2025
View reviewed changes
Copy link

@RushiT0122 RushiT0122 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested few minor changes in-line.

@prijendev prijendev requested a review from RushiT0122 January 9, 2025 08:46
@prijendev prijendev merged commit e0ac6c9 into TDL-14952-Remove-roles-stream Jan 10, 2025
2 checks passed
@prijendev prijendev mentioned this pull request Jan 10, 2025
Draft
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RushiT0122 RushiT0122 RushiT0122 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@prijendev @RushiT0122