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

Open
wants to merge 15 commits into
base: TDL-14952-Remove-roles-stream
Choose a base branch
from
Open

TDL-26718: Implement token chaining #123

wants to merge 15 commits into from

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RushiT0122 RushiT0122 Awaiting requested review from RushiT0122

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