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

Correctly handle dismissed GitHub reviews #38

Merged
merged 2 commits into from
Dec 12, 2018

Conversation

bluekeyes
Copy link
Member

When a review is dismissed, remove it from the review list as though it
never happened. This should be equivalent to a user deleting an approval
comment, although unlike in that case, we don't audit for this event.
Because GitHub provides the ability to restrict review dismissal, this
seems safe.

Fixes #28.

@bluekeyes
Copy link
Member Author

Note that #37 needs to merge first, then I'll update this to reference develop.

@bluekeyes bluekeyes force-pushed the bkeyes/fix-review-dismissal branch 2 times, most recently from 5adda2d to 64ffc33 Compare December 10, 2018 18:47
When a review is dismissed, remove it from the review list as though it
never happened. This should be equivalent to a user deleting an approval
comment, although unlike in that case, we don't audit for this event.
Because GitHub provides the ability to restrict review dismissal, this
seems safe.
@bluekeyes bluekeyes changed the base branch from bkeyes/v4-timeline to develop December 11, 2018 23:59
@bluekeyes
Copy link
Member Author

After further testing, this may not be the best solution. GitHub does change the state of the review in the timeline to DISMISSED after a dismissal, but I suspect these is a delay here. When we process the webhook for the dismissal, the timeline still shows the original review, leading to the incorrect status. Any refresh event after the timeline is updated will show the correct status.

If the timeline already includes the dismissed event at the time of the webhook, this should solve it, although it would be better to change the state of the old review rather than remove it.

ReviewChangesRequested ReviewState = "changes_requested"
ReviewCommented ReviewState = "commented"
ReviewDismissed ReviewState = "dismissed"
ReviewPending ReviewState = "pending"
Copy link
Member Author

Choose a reason for hiding this comment

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

@asvoboda sorry I asked you to change code like this in another project; I was wrong and the type declaration only carries in the specific case of using iota, i.e. you have to omit everything after the name for the type information of the first line to carry to the other lines.

Copy link
Member

@asvoboda asvoboda Dec 12, 2018

Choose a reason for hiding this comment

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

No worries!

@bluekeyes bluekeyes merged commit 5e81dd9 into develop Dec 12, 2018
@bluekeyes bluekeyes deleted the bkeyes/fix-review-dismissal branch December 12, 2018 17:18
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