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.

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

fix stale review_started_at column #1138

Merged
merged 2 commits into from
Aug 12, 2024

Conversation

CollinBeczak
Copy link
Contributor

@CollinBeczak CollinBeczak commented Jul 27, 2024

Issue: Upon a second attempt to review a task, the review session has the first review session’s start time recorded under the field review_started_at. This results in incorrect, resulting high review time being calculated.

Solution: this pr removes the condition that prevent the column from being updated when a review session is started.

Copy link

sonarcloud bot commented Jul 29, 2024

@CollinBeczak CollinBeczak marked this pull request as ready for review August 6, 2024 18:12
Copy link
Contributor

@ljdelight ljdelight left a comment

Choose a reason for hiding this comment

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

Wouldn't this revert #964 and re-introduce the issue described in that PR?

@CollinBeczak
Copy link
Contributor Author

CollinBeczak commented Aug 9, 2024

Wouldn't this revert #964 and re-introduce the issue described in that PR?

Yes it will, though im not entirely sure that is a problem... The column is used to set the matching column in the task_review_history table. I can't think of a scenario where updating the column when a new reviewer starts a task is an issue or how the current implementation doesn't have the same problem since the review time will only get longer because it isn't being update after the first review.

@ljdelight ljdelight requested a review from jschwarz2030 August 10, 2024 03:58
@ljdelight
Copy link
Contributor

@CollinBeczak @jschwarz2030, I'll leave it to ya'll to figure out if this is sufficient or adds a regression. The timing itself is not documented very well, neither PR (the previous and this one) details enough to really "know" what the problem was, and there are also zero tests.

@ljdelight ljdelight dismissed their stale review August 10, 2024 04:03

Dismissing my review, see my latest comment.

@jschwarz2030
Copy link
Collaborator

The former PR was a preference change on how review time was captured for user review CSV reports. I am ok with the revert. It makes more sense that each review gets its own start time as it populated each review in the history table.

@CollinBeczak CollinBeczak merged commit 184a0f1 into main Aug 12, 2024
11 checks passed
@CollinBeczak CollinBeczak deleted the fix-stale-review_started_at-column branch August 12, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants