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

Approval list issues #36

Open
lilyball opened this issue Jun 18, 2014 · 0 comments
Open

Approval list issues #36

lilyball opened this issue Jun 18, 2014 · 0 comments

Comments

@lilyball
Copy link

Reading through the code, the approval list has several issues:

  • r=me on a commit will put both the posting user and a username "me" on the approval list
  • r=username on a commit is accepted even if neither the poster nor the referenced username are reviewers. This seems like a reasonably major issue. Edit: This isn't true, head_comments is actually filtered to only comments made by reviewers.
  • r=metajack on a commit will put both the posting user and "metajack" on the approval list; in this instance, it's seeing the r=me prefix and not realizing that the "me" was part of a larger username
  • r+ $SHA and r=username $SHA on PR comments requires $SHA to be all lowercase. Minor issue, but it really should accept uppercase as well (and the regex should require hex characters, rather than any letter, although that's moot since a non-hex char won't match the current sha anyway).

A couple of priority issues as well:

  • p=100 on a commit is accepted by anyone, even a non-reviewer, even if it's on a different comment than the comment that establishes approval. Edit: This isn't true, as above, head_comments is filtered to only reviewer comments. It still allows it on a different comment but that's probably fine.
  • p=100 can't be specified on a PR comment, even one that establishes approval.
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

No branches or pull requests

1 participant