Provide user ids in team + time entries #146
Conversation
… entry we get a '0' value.
…I submitted a bug report to clickup
Thanks, yes it does for the most part, minus the location names that was mentioned in their issue. I don't mind tacking them on this PR. I'm a bit new to singer and taps in general, is the best course of action to add a new setting for this as I've done with the other two configs or is there another preferred way to add these types of configurations? |
If you could update the README portion of Time Entries to include where this PR would bring us to I'd be good with that. For the config's yes you're doing it right, the only thing you could do on top of that is update the configuration readme portion of the docs (if you run |
| # Formatted in ISO 8601, it must now be converted to milliseconds | ||
| start_date = datetime.strptime(self.config["time_entry_start_date"], "%Y-%m-%dT%H:%M:%SZ") | ||
| # Convert the datetime object to milliseconds | ||
| params["start_date"] = int(start_date.timestamp() * 1000) |
There was a problem hiding this comment.
Just noticed this, we should really be using the at replication key here not just the start_date.
See https://sdk.meltano.com/en/latest/incremental_replication.html#incremental-replication , specefically self.get_starting_timestamp(context) , and instead of having a seperate start date for time_entry we should probably just drop the time_entry_start_date configuration and just document that start_date does what time_entry_start_date is currently documented to do
There was a problem hiding this comment.
I've added an if statement to fallback to the time_entry_start_date in the case that the state is empty on a user's first execution. Otherwise it would use the "at" field as the replication key as suggested.
There was a problem hiding this comment.
why not use self.get_starting_timestamp(context) here? the start_date config variable is already use in this function so we don't need time_entry_start_date either, unless you think we need two seperate start date windows?
* Per reviewer suggestion, use replication key as query for start date * Bring back time_entry_start_date, in the case someone is just starting fresh, they would need to enter a date for the very first sync. The at field is set in milliseconds as the replication_key, we will plug that in when it is provided in the state * Bring back if statement for initial execution falling back to config date * Formatting
Done |
|
Is there something else we need to improve to get this merged? |
| # Formatted in ISO 8601, it must now be converted to milliseconds | ||
| start_date = datetime.strptime(self.config["time_entry_start_date"], "%Y-%m-%dT%H:%M:%SZ") | ||
| # Convert the datetime object to milliseconds | ||
| params["start_date"] = int(start_date.timestamp() * 1000) |
There was a problem hiding this comment.
why not use self.get_starting_timestamp(context) here? the start_date config variable is already use in this function so we don't need time_entry_start_date either, unless you think we need two seperate start date windows?
Took another peek and left a review! |
Notes:
The time_entry_start_date might be overkill but currently there isn't a query param we can use for this endpoint that allows us to query for entries that have changed. Something like date_updated_gt from the get tasks endpoint would be great here
The new default behavior for time entries will be to get the entries of ALL users in your team. If you want only a subset of assignees you can use the time_entry_assignees conf.