TDL-20767 resetting bookmark date to start date #178
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rdeshmukh15
force-pushed
the
TDL-20767-changes
branch
from
c6aa936 to
9b3ec25
Compare
… have events update date older than 30 days
tap_stripe/__init__.py
Outdated
| if max_created != bookmark_value and (bookmark_value != start_date or flag_value is True): | ||
| reset_bookmark_for_event_updates(is_sub_stream, stream_name, sub_stream_name, start_date) | ||
| raise Exception("Provided current bookmark date for event updates is older than 30 days."\ | ||
| " Hence, resetting the bookmark date of respective parent/child stream to start date.") |
There was a problem hiding this comment.
could you add the name of the stream(s) to the exception message
| event_update_window_size = Context.event_update_window_size | ||
| events_update_date_window_size = int(60 * 60 * 24 * event_update_window_size) # event_update_window_size in seconds | ||
| sync_start_time = dt_to_epoch(utils.now()) | ||
| start_date = int(utils.strptime_to_utc(Context.config["start_date"]).timestamp()) |
There was a problem hiding this comment.
Since theres a variable for start_date now, it can be used on line 971 and 978
tap_stripe/__init__.py
Outdated
| @@ -952,9 +952,11 @@ def sync_event_updates(stream_name, is_sub_stream): | |||
| or when called through only child stream i.e. when parent is not selected. | |||
| ''' | |||
| LOGGER.info("Started syncing event based updates") | |||
| flag_value = False | |||
There was a problem hiding this comment.
could this variable name be more descriptive
| # Verify warning message for bookmark of less than last 30 days. | ||
| self.assertEqual(mock_logger.mock_calls, expected_logger_warning) | ||
| with self.assertRaises(Exception) as e: | ||
| sync_event_updates('charges', False) |
There was a problem hiding this comment.
Could you add assertions around the altered state after the exception?
There was a problem hiding this comment.
Added the test where we can check he call count, not added the assertion to compare the state because reset_bookmark_for_event_updates function doesn't return anything.
| @mock.patch('tap_stripe.LOGGER.warning') | ||
| def test_sync_event_updates_bookmark_before_last_7_days(self, mock_logger, mock_write_bookmark, mock_stripe_event, mock_utils_now): | ||
| @mock.patch('singer.utils.now', return_value = datetime.datetime.strptime("2023-05-10T08:30:50Z", "%Y-%m-%dT%H:%M:%SZ")) | ||
| def test_sync_event_updates_bookmark_before_last_30_days(self, mock_utils_now, mock_stripe_event): |
There was a problem hiding this comment.
also add a test where two streams are selected
leslievandemark
approved these changes
May 25, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of change
Manual QA steps
Tested following scenarios :
- Scenario 1 : subscriptions and subscription_items stream are selected, passed 15th march 2023 as created in state file for subscriptions and subscriptions_events and 1st may 2023 as created for subscription_items
==>
In the first run, it should raise the exception and reset the created for
subscriptionsto start_date and removesubscriptions_eventskey from the state and same should happen with thesubscription_itemsas wellIn the second run, it should fetch the historical data for
subscriptionsand thesubscription_itemsstream- Scenario 2 : If only one stream is selected and it has quite older(> 30 days) event updates then
==>
Risks
Rollback steps