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

rfcbot is missing non-checkbox reviews #91

Closed
anp opened this issue Oct 7, 2016 · 2 comments
Closed

rfcbot is missing non-checkbox reviews #91

anp opened this issue Oct 7, 2016 · 2 comments
Labels
rfcbot
Milestone
Adam goes incognito

Comments

@anp
Copy link
Member

anp commented Oct 7, 2016

@alexcrichton, #61 (comment) -- this was definitely missed. Need to look at logs.
@anp anp added the rfcbot label Oct 7, 2016
@anp anp added this to the Adam goes incognito milestone Oct 22, 2016
@anp
Copy link
Member Author

anp commented Oct 25, 2016

So it looks like this was happening because rfcbot set the reviewed status to true in the database, and then afterwards set it back when it parsed the status comment and the box hadn't been checked. Oops!

Quick fix: don't allow unchecking items to revert a reviewed status after rfcbot knows about it. This might be a problem down the road but will fix things for @michaelwoerister @vadimcn and @withoutboats who have been using the fcp reviewed command instead of the checkboxes.

@anp anp closed this as completed in b765d8d Oct 25, 2016
@bjorn3
Copy link
Member

bjorn3 commented Oct 25, 2016

Maybe also check the checkbox when it sees a review.

@anp anp mentioned this issue Oct 25, 2016
Merged
anoadragon453 pushed a commit to matrix-org/mscbot that referenced this issue Jul 6, 2018
@anp
Fixing fcp reviewed comment bug from DB race condition.
7efc710 
The bot was doing this:

1. receive reviewed command, set db entry reviewed=true
2. finish processing commands, starts parsing the status comment for checkboxes
3. found an unchecked box, set db entry reviewed=false
4. updating the status comment based on the comment's prior state instead of command

The easy change was to make it so that step 3 doesn't happen. Checkboxes are one-way
(as was originally intended) -- no takesies-backsies.

Closes rust-lang#91


Former-commit-id: b765d8d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
rfcbot
Projects
None yet
Milestone
Adam goes incognito
Development

No branches or pull requests
2 participants
@anp @bjorn3