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

Update new_max_bookmark, Add unittests #30

Merged
merged 5 commits into from
Sep 8, 2022
Merged

Update new_max_bookmark, Add unittests #30

merged 5 commits into from
Sep 8, 2022

Conversation

dsprayberry
Copy link
Contributor

@dsprayberry dsprayberry commented Sep 7, 2022 •
edited
Loading

Description of change

  • Updates new_max_bookmark to check that we do not write future-dated bookmark values (as we've observed Closeio returning future-dated activities records even when passing the date_created__lt parameter on requests)
  • Adds unittests

Manual QA steps

  • added 2 unittests that validate new_max_bookmark functionality::
    • test_new_max_bookmark_valid_potential
    • test_new_max_bookmark_future_dated_potential

Risks

  • Minimal. State is stored in the same format, but it will no longer have the potential to be a datetime string greater than now().

Rollback steps

  • revert this branch

@dsprayberry dsprayberry mentioned this pull request Sep 7, 2022
Merged
luandy64
luandy64 requested changes Sep 7, 2022
View reviewed changes
Copy link
Contributor

@luandy64 luandy64 left a comment

Choose a reason for hiding this comment

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

One small change, but then this is good

tap_closeio/streams.py Outdated Show resolved Hide resolved
@dsprayberry @luandy64
Change order of variables in new_max_bookmark
e6749bb 
Co-authored-by: Andy Lu <andy@stitchdata.com>
dmosorast
dmosorast reviewed Sep 7, 2022
View reviewed changes
tap_closeio/streams.py Outdated Show resolved Hide resolved
@dsprayberry dsprayberry mentioned this pull request Sep 8, 2022
Merged
@dsprayberry dsprayberry merged commit bf53386 into master Sep 8, 2022
@dsprayberry dsprayberry deleted the fix/do-not-store-future-dated-state-values branch September 8, 2022 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luandy64 luandy64 luandy64 approved these changes

@dmosorast dmosorast dmosorast 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.
3 participants
@dsprayberry @luandy64 @dmosorast