PR#163: Make Pagure vote parser more liberal and also safer - fedora-qa/blockerbugs

		`@@ -1,6 +1,5 @@`
		`#!/usr/bin/python3`

		`- import itertools`
		`import datetime`
		`import re`

		`@@ -10,63 +9,155 @@`
		`TRACKER_KEYWORDS = [`
		`'betablocker',`
		`'finalblocker',`
		`- 'betafe',`
		`- 'finalfe',`
		`+ 'betafreezeexception',`
		`+ 'finalfreezeexception',`
		`'0day',`
		`'previousrelease'`
		`]`

		`-`
		`- VOTE_KEYWORDS = [' '.join(v) for v in`
		`- itertools.product(TRACKER_KEYWORDS, ['-1', '0', '+1'])]`
		`-`
		`- OUTCOME_KEYWORDS = [''.join(r) for r in`
		`- itertools.product(['accepted', 'rejected'], TRACKER_KEYWORDS)]`
		`-`
		`- REVOTE_KEYWORDS = ['revote ' + t for t in TRACKER_KEYWORDS]`
		`-`
		`- #ADMIN_KEYWORDS = RESULT_KEYWORDS + REVOTE_KEYWORDS`
		`+ VOTES = ("+1", "0", "-1")`
		`+ TRACKER_RE = r'((beta\|final)(blocker\|fe\|freezeexception)\|0day\|previousrelease)'`
		`+ TRACKER_MATCHER = re.compile(TRACKER_RE)`
		`+ OUTCOME_RE = r'(accepted\|rejected)' + TRACKER_RE`
		`+ OUTCOME_MATCHER = re.compile(OUTCOME_RE)`

		`SUMMARY_HEADER = ""`
		`CLOSED_VOTES_HEADER = "The following votes have been closed:"`


		`- def agreed_parser(line):`
		`+ def agreed_revote_parser(line):`
		`+ """Parse an AGREED or a REVOTE line. If there's anything violating the`
		`+ rules for the line (e.g. it doesn't start with the expected command, or`
		`+ there's some extra text that shouldn't be there), ignore the line contents`
		`+ and return []. All instances of "fe" get expanded to "freezeexception".`
		`+`
		`+ :param str line: a single line`
		`+ :return: A list of commands with one tracker per line (or an empty list).`
		`+ E.g. for line 'agreed acceptedbetablocker rejectedfinalfe' this returns:`
		`+ ['agreed acceptedbetablocker',`
		`+ 'agreed rejectedfinalfreezeexception']`
		`+ For line 'revote finalblocker 0day' this returns:`
		`+ ['revote finalblocker',`
		`+ 'revote 0day']`
		`+ """`
		`+ # parsing word by word avoids complex and fragile regex syntax`
		`+ words = line.split()`
		`+ # first word must be 'agreed' or 'revote'`
		`+ if len(words) <= 0:`
		`+ return []`
		`+ command = words[0]`
		`+ if command == 'agreed':`
		`+ matcher = OUTCOME_MATCHER`
		`+ summary = True`
		`+ elif command == 'revote':`
		`+ matcher = TRACKER_MATCHER`
		`+ summary = False`
		`+`
		`out = []`
		`- outcome_pattern = '\|'.join(OUTCOME_KEYWORDS)`
		`- #pattern = '^agreed (({}) )+.*$'.format(outcome_pattern)`
		`- pattern = '^agreed ({})( ({}))( .)?$'.format(outcome_pattern, outcome_pattern)`
		`- m = re.match(pattern, line)`
		`- if m:`
		`- for outcome in OUTCOME_KEYWORDS:`
		`- if outcome in line:`
		`- out.append('agreed ' + outcome)`
		`+ for word in words[1:]:`
		`+ if matcher.fullmatch(word):`
		`+ # if we get a match, save it as a valid command`
		`+ word = expand_fe(word)`
		`+ out.append(f'{command} {word}')`
		`+ else:`
		`+ # if not, we break and return the matches we got so far if summary`
		`+ # is allowed, return empty if not`
		`+ if summary:`
		`+ break`
		`+ return []`
		`return out`

		`- def revote_parser(line):`
		`+`
		`+ def vote_parser(line):`
		`+ """Look for votes in a single line. We accept pairs of a vote and a match`
		`+ of TRACKER_RE, in either order, and nothing else; any other text`
		`+ invalidates the line. All instances of "fe" get expanded to "freezeexception".`
		`+`
		`+ :param str line: a line which might contain a vote`
		`+ :return: A list of votes with one tracker per line (or an empty list).`
		`+ E.g. for line 'betafe +1 finalblocker -1' this returns:`
		`+ ['betafreezeexception +1',`
		`+ 'finalblocker -1']`
		`+ """`
		`out = []`
		`- trackers = '\|'.join(TRACKER_KEYWORDS)`
		`- pattern = '^revote (({}) ?)+$'.format(trackers)`
		`- m = re.match(pattern, line)`
		`- if m:`
		`- for tracker in TRACKER_KEYWORDS:`
		`- if tracker in line:`
		`- out.append('revote ' + tracker)`
		`+ words = line.split()`
		`+ # if we have an odd number of words we don't have a clean vote line`
		`+ if (len(words) % 2) != 0:`
		`+ return out`
		`+ # split list into pairs:`
		`+ # https://stackoverflow.com/questions/312443`
		`+ for pair in (words[i:i + 2] for i in range(0, len(words), 2)):`
		`+ if TRACKER_MATCHER.fullmatch(pair[0]) and pair[1] in VOTES:`
		`+ pair[0] = expand_fe(pair[0])`
		`+ out.append(" ".join(pair))`
		`+ elif pair[0] in VOTES and TRACKER_MATCHER.fullmatch(pair[1]):`
		`+ pair[1] = expand_fe(pair[1])`
		`+ pair.reverse()`
		`+ out.append(" ".join(pair))`
		`+ else:`
		`+ # we found something other than a vote pair, so per`
		`+ # Kamil's Strict Vote Parsing Regime, we reject the line`
		`+ return []`
		`return out`


		`+ def expand_fe(keyword):`
		`+ '''Expand "FE" into "FreezeException" in tracker-related keywords.`
		`+`
		`+ While we accept "FE" as a handy user abbreviation, we want to process`
		`+ it as "FreezeException" internally, so that we don't need to handle both`
		`+ variants everywhere in our code.`
		`+`
		`+ :param str keyword: a tracker or an outcome keyword, lowercased`
		`+ :return: a keyword expanded to the full "FreezeException" variant if`
		`+ applicable, or the keyword unchanged)`
		`+ '''`
		`+ tracker_match = TRACKER_MATCHER.fullmatch(keyword)`
		`+ outcome_match = OUTCOME_MATCHER.fullmatch(keyword)`
		`+`
		`+ if tracker_match:`
		`+ # the 'fe' string is in group #3 in TRACKER_RE`
		`+ fe_group_num = 3`
		`+ match = tracker_match`
		`+ elif outcome_match:`
		`+ # the 'fe' string is in group #4 in OUTCOME_RE`
		`+ fe_group_num = 4`
		`+ match = outcome_match`
		`+ else:`
		`+ return keyword`
		`+`
		`+ if match.group(fe_group_num) != 'fe':`
		`+ return keyword`
		`+`
		`+ start = match.start(fe_group_num)`
		`+ end = start + len('fe')`
		`+ return keyword[:start] + 'freezeexception' + keyword[end:]`
		`+`
		`+`
		`def bot_loop_detected(comments):`
		`+ '''Detect a loop of bot comments in the list of comments. The loop is`
		`+ defined as a continuous stream of more than $threshold comments, all from`
		`+ the bot, including the last comment in the list. The threshold is defined`
		`+ in config.`
		`+`
		`+ :param comments: a list of Comment instances`
		`+ :return: bool whether there is a loop`
		`+ '''`
		`is_bot_comment = [comment.user == app.config['PAGURE_BOT_USERNAME'] for comment in comments]`
		`- if len(is_bot_comment) < app.config['PAGURE_BOT_LOOP_THRESHOLD']:`
		`+ threshold = app.config['PAGURE_BOT_LOOP_THRESHOLD']`
		`+ if len(is_bot_comment) < threshold:`
		`return False`

		`- return all(is_bot_comment[-app.config['PAGURE_BOT_LOOP_THRESHOLD']:])`
		`+ return all(is_bot_comment[-threshold:])`


		`class Comment():`
		`+ '''This represents a single comment from the voting ticket.`
		`+ '''`
		`def __init__(self, raw_comment):`
		`self.text = raw_comment['comment'].replace('\r', '')`
		`self.user = raw_comment['user']['name']`
		`+ #: Pagure's comment id (int)`
		`self.id = raw_comment['id']`

		`def __repr__(self):`
		`@@ -78,93 +169,141 @@`
		`return self.user in pagure_interface.get_group('fedora-qa')['members']`

		`def keywords(self):`
		`+ '''Search for command keywords in this comment, purge anything`
		`+ unrelated and return it.`
		`+`
		`+ :return: a list with one vote or command per line, e.g.:`
		`+ ['betablocker +1',`
		`+ 'agreed acceptedbetablocker',`
		`+ 'revote betablocker',`
		`+ 'betablocker -1']`
		`+ '''`
		`out = []`
		`for line in self.text.lower().split('\n'):`
		`- line = line.rstrip()`
		`- line = re.sub(' +', ' ', line)`
		`- if line in VOTE_KEYWORDS:`
		`- out.append(line)`
		`- elif line.startswith('agreed'):`
		`- out.extend(agreed_parser(line))`
		`- elif line.startswith('revote'):`
		`- out.extend(revote_parser(line))`
		`+ line = line.strip()`
		`+ if line.startswith('agreed') or line.startswith('revote'):`
		`+ out.extend(agreed_revote_parser(line))`
		`+ else:`
		`+ out.extend(vote_parser(line))`
		`return out`

		`def is_summary_post_of(self, tracker_keyword):`
		`- is_bot = self.user == app.config['PAGURE_BOT_USERNAME']`
		`- is_summary = CLOSED_VOTES_HEADER.lower() in self.text.lower()`
		`- is_tracker_relevant = tracker_keyword in self.text.lower()`
		`+ '''Return True/False whether this particular post is a summary of`
		`+ voting results for a particular tracker.`
		`+ '''`
		`+ is_bot = (self.user == app.config['PAGURE_BOT_USERNAME'])`
		`+ is_summary = (CLOSED_VOTES_HEADER.lower() in self.text.lower())`
		`+ is_tracker_relevant = (tracker_keyword in self.text.lower())`

		`return is_bot and is_summary and is_tracker_relevant`


		`class BugVoteTracker():`
		`- def __init__(self, accepted_keyword):`
		`+ '''Count votes specific for a single tracker. Just feed all comments to`
		`+ this class and it'll reflect the tracker's final state in its variables.`
		`+ '''`
		`+`
		`+ def __init__(self, tracker):`
		`self.open = True`
		`+ #: 'accepted' or 'rejected'`
		`self.outcome = None`
		`- self.accepted_keyword = accepted_keyword`
		`+ #: one of TRACKER_KEYWORDS`
		`+ self.tracker = tracker`
		`self.need_summary_post = False`

		`- # vote format: {`
		`- # 'user': {`
		`- # 'vote': '+1'`
		`- # 'comment_id': '21383'`
		`- # }`
		`- # }`
		`+ #: user votes in this format:`
		`+ #: {`
		`+ #: 'user': {`
		`+ #: 'vote': '+1'`
		`+ #: 'comment_id': 21383`
		`+ #: }`
		`+ #: }`
		`self.votes = {}`

		`- def _filter_relevant(self, keywords):`
		`- accepted_votes = [self.accepted_keyword + v for v in [' +1', ' 0', ' -1']]`
		`- accepted_agreed = ['agreed ' + o + self.accepted_keyword for o in ['accepted', 'rejected']]`
		`- accepted_revote = ['revote ' + self.accepted_keyword]`
		`- accepted_keywords = accepted_votes + accepted_agreed + accepted_revote`
		`- return [keyword for keyword in keywords if keyword in accepted_keywords]`
		`-`
		`def parse_comment(self, comment):`
		`- if self.need_summary_post:`
		`- self.need_summary_post = not comment.is_summary_post_of(self.accepted_keyword)`
		`+ '''Take a comment and parse all commands from it that are relevant to`
		`+ this particular tracker. Update the instance variables.'''`
		`+`
		`+ if comment.is_summary_post_of(self.tracker):`
		`+ self.need_summary_post = False`
		`+ return`
		`+`
		`+ # now that we detected summary posts, we can ignore all posts from the bot`
		`+ if comment.user == app.config['PAGURE_BOT_USERNAME']:`
		`+ return`
		`+`
		`+ for words in [line.split() for line in comment.keywords()]:`
		`+ agreed = (words[0] == 'agreed')`
		`+ revote = (words[0] == 'revote')`
		`+`
		`+ if agreed:`
		`+ match = OUTCOME_MATCHER.fullmatch(words[1])`
		`+ outcome = match.group(1)`
		`+ tracker = match.group(2)`
		`+ elif revote:`
		`+ tracker = words[1]`
		`+ else: # a vote`
		`+ tracker = words[0]`
		`+`
		`+ if tracker != self.tracker:`
		`+ continue`

		`- keywords = self._filter_relevant(comment.keywords())`
		`- for keyword in keywords:`
		`- # FIXME log rejected (non admin) users?`
		`- if keyword.startswith('agreed') and comment.user_is_admin():`
		`+ if (agreed or revote) and not comment.user_is_admin():`
		`+ app.logger.debug(`
		`+ f'A non-admin user {comment.user} tries to perform '`
		`+ f'administrative commands in comment {comment.id}, '`
		`+ 'ignoring.')`
		`+ continue`
		`+`
		`+ if agreed:`
		`self.open = False`
		`- self.outcome = keyword[7:15] # eh...`
		`+ self.outcome = outcome`
		`self.need_summary_post = True`
		`- elif keyword.startswith('revote') and comment.user_is_admin():`
		`+ elif revote:`
		`self.open = True`
		`+ self.outcome = None`
		`+ self.need_summary_post = False`
		`self.votes = {}`
		`- else:`
		`+ else: # a vote`
		`if not self.open:`
		`continue`
		`- for vote in ['-1', '0', '+1']:`
		`- if keyword.endswith(vote):`
		`- self.votes[comment.user] = {`
		`- 'vote': vote,`
		`- 'comment_id': comment.id`
		`- }`
		`- return keywords`
		`+ assert words[1] in VOTES`
		`+ self.votes[comment.user] = {`
		`+ 'vote': words[1],`
		`+ 'comment_id': comment.id`
		`+ }`

		`def enumerate_votes(self):`
		`- out = {`
		`- '-1': [],`
		`- '0': [],`
		`- '+1': []`
		`+ '''Return self.votes in a format:`
		`+ { '-1': [('user1', comment_id), ('user2', comment_id)],`
		`+ '0': [...],`
		`+ '+1': [...],`
		`}`
		`+ '''`
		`+ out = {vote: [] for vote in VOTES}`
		`for user, vote in self.votes.items():`
		`out[vote['vote']].append((user, vote['comment_id']))`

		`return out`

		`+`
		`def link(user, comment_id):`
		`+ '''Create a Markdown snippet containing a link to the user's comment.`
		`+ '''`
		`return "[%s](#comment-%s)" % (user, comment_id)`

		`+`
		`def tracker_summary(tracker_name, tracker):`
		`+ '''Create a summary of votes for a given tracker in a Markdown format.`
		`+`
		`+ :param str tracker_name: e.g. 'BetaBlocker'`
		`+ :param tracker: an instance of BugVoteTracker`
		`+ '''`
		`nicer = {`
		`'betablocker': 'BetaBlocker',`
		`'finalblocker': 'FinalBlocker',`
		`- 'betafe': 'BetaFE',`
		`- 'finalfe': 'FinalFE',`
		`+ 'betafreezeexception': 'BetaFreezeException',`
		`+ 'finalfreezeexception': 'FinalFreezeException',`
		`'0day': '0Day',`
		`'previousrelease': 'PreviousRelease',`
		`'accepted': 'Accepted',`
		`@@ -196,7 +335,16 @@`

		`return out`

		`+`
		`def summary(trackers, non_voting_users=None, last_comment_id=None, header=CLOSED_VOTES_HEADER):`
		`+ '''Create a summary text in a Markdown format which can be used either in`
		`+ a ticket description, or as a comment when a vote is closed.`
		`+`
		+ :param trackers: a dict as returned from :func:`parse_comments_to_trackers`
		`+ :param non_voting_users: a list of users who haven't voted yet`
		`+ :param int last_comment_id: the last comment id that was counted`
		`+ :param str head: an initial message text`
		`+ '''`
		`md_text = "%s\n\n" % header`

		`for tracker_name, tracker in trackers.items():`
		`@@ -209,27 +357,39 @@`

		`md_text += tracker_summary(tracker_name, tracker)`

		`- for neco in ['+1', '0', '-1']:`
		`- votes[neco] = [link(user, comment_id) for (user, comment_id) in votes[neco]]`
		`- if len(votes[neco]) == 0:`
		`+ for vote in VOTES:`
		`+ votes[vote] = [link(user, comment_id) for (user, comment_id) in votes[vote]]`
		`+ if len(votes[vote]) == 0:`
		`continue`
		`- md_text += " * %s by %s\n" % (neco, ', '.join(votes[neco]))`
		`+ md_text += " * %s by %s\n" % (vote, ', '.join(votes[vote]))`

		`md_text += '\n'`

		`if non_voting_users:`
		`- md_text += "Commented but haven't voted yet: %s\n\n" % ', '.join([link(u, c) for (u, c) in non_voting_users.items()])`
		`+ md_text += "Commented but haven't voted yet: %s\n\n" % ', '.join(`
		`+ [link(u, c) for (u, c) in non_voting_users.items()])`

		`if last_comment_id:`
		`time = datetime.datetime.utcnow().strftime("%Y-%m-%d %H:%M")`

		`md_text += ("*The votes have been last counted at %s UTC and the last "`
		`- "processed comment was [#comment-%s](#comment-%s)*" % (time, last_comment_id, last_comment_id))`
		`+ "processed comment was [#comment-%s](#comment-%s)*" % (`
		`+ time, last_comment_id, last_comment_id))`

		`return md_text`


		`def parse_comments_to_trackers(comments):`
		`+ '''Create BugVoteTracker for each tracker keyword supported, and then feed`
		`+ all comments to each tracker to fully update them.`
		`+`
		`+ :param comments: a list of Comment instances`
		`+ :return: A dict in format:`
		`+ {'betablocker': BugVoteTracker,`
		`+ 'betafreezeexception': BugVoteTracker,`
		`+ ...`
		`+ }`
		`+ '''`
		`trackers = {tracker: BugVoteTracker(tracker) for tracker in TRACKER_KEYWORDS}`

		`for comment in comments:`
		`@@ -240,6 +400,16 @@`


		`def voting_info(comments, trackers):`
		`+ '''Compute users who haven't voted yet, and the last processed comment`
		`+`
		`+ :param comments: a list of Comment instances`
		+ :param trackers: a dict as returned from :func:`parse_comments_to_trackers`
		`+ :return: a tuple of (dict of non_voting_users, last_comment_id), e.g.:`
		`+ ({'user1': 1234,`
		`+ 'user2': 1235`
		`+ },`
		`+ 1238)`
		`+ '''`
		`last_comment_id = 0`
		`non_voting_users = {}`
		`voting_users = set()`
		`@@ -259,6 +429,8 @@`


		`def webhook_handler(issue_id):`
		`+ '''React to a Pagure webhook notification that a discussion ticket changed.`
		`+ '''`
		`comments = [Comment(c) for c in pagure_interface.get_issue_comments(issue_id)]`

		`trackers = parse_comments_to_trackers(comments)`

docs/source/review-voting.md

file modified

+16 -10

		`@@ -18,16 +18,17 @@`

		`You can Watch issues in this repo, which will send you an email for every new proposed bug and every new comment in any ticket. If you wish to participate in blocker review discussions in general and not just in a single topic, this is the best way to subscribe.`

		`+`
		`### How to vote`
		`Place a voting command on a separate line in the form of:<br/>`
		`TRACKER VOTE`

		- where `TRACKER` is one of:
		+ where `TRACKER` is one of these words (case-insensitive):

		* `BetaBlocker`
		* `FinalBlocker`
		- * `BetaFE` - FE standing for FreezeException
		- * `FinalFE` - FE standing for FreezeException
		+ * `BetaFreezeException` (or `BetaFE`)
		+ * `FinalFreezeException` (or `FinalFE`)
		* `0Day`
		* `PreviousRelease`

		`@@ -37,18 +38,23 @@`
		* `0` ([no strong feelings one way or the other](https://www.youtube.com/watch?v=ussCHoQttyQ) or to take your vote back)
		* `-1` (to vote against)

		`- If you want to cast several votes, use several separate lines (in one or more comments):`
		`+ Example:`
		```
		`- BetaBlocker -1`
		`- BetaFE +1`
		`- FinalBlocker +1`
		`+ BetaBlocker +1`
		+ ```
		`+ If you want to cast several votes, you can use several separate lines (in one or more comments), or a single line:`
		+ ```
		`+ BetaBlocker -1 BetaFE +1 FinalBlocker +1`
		+ ```
		`+ You can swap the word order as well:`
		+ ```
		`+ +1 FinalBlocker`
		```
		`Voting commands must not be mixed with arbitrary other words, this will not be counted:`
		```
		`- BetaBlocker +1 haha`
		`+ BetaBlocker +1 for sure`
		```
		`-`
		`- If you vote for the same tracker multiple times, only your last vote is counted. For example, if you submit these three comments:`
		`+ If you vote for the same tracker multiple times, only your last vote is counted. For example, if you submit these three votes (in one or more comments):`
		```
		`BetaBlocker +1`
		`BetaFE +1`

testing/test_pagure_bot.py

file modified

+447 -194

		`@@ -6,6 +6,7 @@`
		`from blockerbugs.util import pagure_interface`
		`from blockerbugs.util import pagure_bot`

		`+ EXTENDED_TRACKERS = pagure_bot.TRACKER_KEYWORDS + ["betafe", "finalfe"]`

		`def powerset(iterable):`
		`s = list(iterable)`
		`@@ -31,6 +32,14 @@`
		`"name": "fake-group"`
		`}`

		`+ def expand_fe(tracker):`
		`+ if tracker == 'betafe':`
		`+ return 'betafreezeexception'`
		`+ elif tracker == 'finalfe':`
		`+ return 'finalfreezeexception'`
		`+ else:`
		`+ return tracker`
		`+`
		`@pytest.fixture`
		`def setup_stubs(monkeypatch):`
		`stub_pagure_group_call = mock.MagicMock(return_value=stub_group_data)`
		`@@ -38,13 +47,14 @@`


		`class TestPagureBotBasics(object):`
		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("inversed", [False, True])`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("vote", ['+1', '-1', '0'])`
		`- def test_basic_vote(self, vote, keyword):`
		`+ def test_basic_vote(self, vote, keyword, inversed):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`'id': 1,`
		`- 'comment': "%s %s" % (keyword, vote),`
		`+ 'comment': "%s %s" % ((vote, keyword) if inversed else (keyword, vote)),`
		`'user': {`
		`'name': stub_admin_user1`
		`}`
		`@@ -52,14 +62,14 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {`
		`+ assert trackers[expand_fe(keyword)].votes == {`
		`stub_admin_user1: {`
		`'vote': '%s' % vote,`
		`'comment_id': 1`
		`}`
		`}`

		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`def test_basic_vote_repeated(self, keyword):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`@@ -79,7 +89,7 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {`
		`+ assert trackers[expand_fe(keyword)].votes == {`
		`stub_admin_user1: {`
		`'vote': '-1',`
		`'comment_id': 2`
		`@@ -88,7 +98,7 @@`


		`class TestPagureBotTypos(object):`
		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("sign", ['+', '-'])`
		`def test_typo(self, sign, keyword):`
		`keyword_typo = list(keyword)`
		`@@ -105,17 +115,48 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {}`
		`+ assert trackers[expand_fe(keyword)].votes == {}`
		`+`
		`+ def test_multivote_extra_comma(self):`
		`+ comments = [pagure_bot.Comment(c) for c in [`
		`+ {`
		`+ 'id': 1,`
		`+ 'comment': "BetaBlocker -1 BetaFE +1, FinalBocker -1",`
		`+ 'user': {`
		`+ 'name': stub_admin_user1`
		`+ }`
		`+ }`
		`+ ]]`
		`+`
		`+ trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`+ for tracker in trackers.values():`
		`+ assert tracker.votes == {}`
		`+`
		`+ def test_multivote_extra_vote(self):`
		`+ comments = [pagure_bot.Comment(c) for c in [`
		`+ {`
		`+ 'id': 1,`
		`+ 'comment': "BetaBlocker -1 +1 FinalBlocker -1 BetaFE -1",`
		`+ 'user': {`
		`+ 'name': stub_admin_user1`
		`+ }`
		`+ }`
		`+ ]]`
		`+`
		`+ trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`+ for tracker in trackers.values():`
		`+ assert tracker.votes == {}`


		`class TestPagureBotGotchas(object):`
		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("inversed", [False, True])`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("sign", ['＋', '–']) # these chars are not + or -`
		`- def test_unicode_plus(self, sign, keyword):`
		`+ def test_unicode_plus(self, sign, keyword, inversed):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`'id': 1,`
		`- 'comment': "%s %s1" % (keyword, sign),`
		`+ 'comment': "%s1 %s" % (sign, keyword) if inversed else "%s %s1" % (keyword, sign),`
		`'user': {`
		`'name': stub_admin_user1`
		`}`
		`@@ -123,15 +164,16 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {}`
		`+ assert trackers[expand_fe(keyword)].votes == {}`

		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("inversed", [False, True])`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("sign", ['+', '-'])`
		`- def test_plusminus_eleven(self, sign, keyword):`
		`+ def test_plusminus_eleven(self, sign, keyword, inversed):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`'id': 1,`
		`- 'comment': "%s %s11" % (keyword, sign),`
		`+ 'comment': "%s11 %s" % (sign, keyword) if inversed else "%s %s11" % (keyword, sign),`
		`'user': {`
		`'name': stub_admin_user1`
		`}`
		`@@ -139,15 +181,16 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {}`
		`+ assert trackers[expand_fe(keyword)].votes == {}`

		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("inversed", [False, True])`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("sign", ['+', '-'])`
		`- def test_plusminus_two(self, sign, keyword):`
		`+ def test_plusminus_two(self, sign, keyword, inversed):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`'id': 1,`
		`- 'comment': "%s %s2" % (keyword, sign),`
		`+ 'comment': "%s2 %s" % (sign, keyword) if inversed else "%s %s2" % (keyword, sign),`
		`'user': {`
		`'name': stub_admin_user1`
		`}`
		`@@ -155,15 +198,16 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {}`
		`+ assert trackers[expand_fe(keyword)].votes == {}`

		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("inversed", [False, True])`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("sign", ['+', '-'])`
		`- def test_plusminus_typo(self, sign, keyword):`
		`+ def test_plusminus_typo(self, sign, keyword, inversed):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`'id': 1,`
		`- 'comment': "%s %s!" % (keyword, sign),`
		`+ 'comment': "%s! %s" % (sign, keyword) if inversed else "%s %s!" % (keyword, sign),`
		`'user': {`
		`'name': stub_admin_user1`
		`}`
		`@@ -171,15 +215,16 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {}`
		`+ assert trackers[expand_fe(keyword)].votes == {}`

		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("inversed", [False, True])`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("sign", ['+', '-'])`
		`- def test_plusminus_float(self, sign, keyword):`
		`+ def test_plusminus_float(self, sign, keyword, inversed):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`'id': 1,`
		`- 'comment': "%s %s0.5" % (keyword, sign),`
		`+ 'comment': "%s0.5 %s" % (sign, keyword) if inversed else "%s %s0.5" % (keyword, sign),`
		`'user': {`
		`'name': stub_admin_user1`
		`}`
		`@@ -187,15 +232,16 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {}`
		`+ assert trackers[expand_fe(keyword)].votes == {}`

		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("inversed", [False, True])`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("sign", ['+', '-'])`
		`- def test_plusminus_emoji(self, sign, keyword):`
		`+ def test_plusminus_emoji(self, sign, keyword, inversed):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`'id': 1,`
		`- 'comment': "%s %s👎" % (keyword, sign),`
		`+ 'comment': "%s👎 %s" % (sign, keyword) if inversed else "%s %s👎" % (keyword, sign),`
		`'user': {`
		`'name': stub_admin_user1`
		`}`
		`@@ -203,15 +249,16 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {}`
		`+ assert trackers[expand_fe(keyword)].votes == {}`

		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("inversed", [False, True])`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("sign", ['+', '-'])`
		`- def test_plusminus_backtick(self, sign, keyword):`
		`+ def test_plusminus_backtick(self, sign, keyword, inversed):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`'id': 1,`
		- 'comment': "`%s %s1`" % (keyword, sign),
		+ 'comment': "`%s1 %s`" % (sign, keyword) if inversed else "`%s %s1`" % (keyword, sign),
		`'user': {`
		`'name': stub_admin_user1`
		`}`
		`@@ -219,17 +266,18 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {}`
		`+ assert trackers[expand_fe(keyword)].votes == {}`


		`class TestPagureBotMultiline(object):`
		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("inversed", [False, True])`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("sign", ['+', '-'])`
		`- def test_multiline_singlevote(self, sign, keyword):`
		`+ def test_multiline_singlevote(self, sign, keyword, inversed):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`'id': 1,`
		`- 'comment': "foobar\n%s %s1\nbarbaz" % (keyword, sign),`
		`+ 'comment': "foobar\n%s1 %s\nbarbaz" % (sign, keyword) if inversed else "foobar\n%s %s1\nbarbaz" % (keyword, sign),`
		`'user': {`
		`'name': stub_admin_user1`
		`}`
		`@@ -237,24 +285,96 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {`
		`+ assert trackers[expand_fe(keyword)].votes == {`
		`stub_admin_user1: {`
		`'vote': '%s1' % sign,`
		`'comment_id': 1`
		`}`
		`}`

		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("inversed", itertools.product([False, True], [False, True]))`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`+ @pytest.mark.parametrize("vote", [('+1', '-1'), ('-1', '+1'), ('+1', '0'),`
		`+ ('-1', '0'), ('0', '-1'), ('0', '+1')])`
		`+ def test_multiline_multivote_same(self, vote, keyword, inversed):`
		`+ vote_first = vote[0]`
		`+ vote_second = vote[1]`
		`+ first = (vote_first, keyword) if inversed[0] else (keyword, vote_first)`
		`+ second = (vote_second, keyword) if inversed[1] else (keyword, vote_second)`
		`+ comments = [pagure_bot.Comment(c) for c in [`
		`+ {`
		`+ 'id': 1,`
		`+ 'comment': "foobar\n%s %s\nbarbaz\n%s %s" % (first + second),`
		`+ 'user': {`
		`+ 'name': stub_admin_user1`
		`+ }`
		`+ }`
		`+ ]]`
		`+`
		`+ trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`+ assert trackers[expand_fe(keyword)].votes == {`
		`+ stub_admin_user1: {`
		`+ 'vote': '%s' % vote_second,`
		`+ 'comment_id': 1`
		`+ }`
		`+ }`
		`+`
		`+ @pytest.mark.parametrize("inversed", itertools.product([False, True], [False, True]))`
		`+ @pytest.mark.parametrize("keyword", itertools.permutations(EXTENDED_TRACKERS, 2))`
		`+ @pytest.mark.parametrize("vote", [('+1', '-1'), ('-1', '+1'), ('+1', '0'),`
		`+ ('-1', '0'), ('0', '-1'), ('0', '+1'),`
		`+ ('0', '0'), ('-1', '-1'), ('+1', '+1')])`
		`+ def test_multiline_multivote_different(self, vote, keyword, inversed):`
		`+ keyword_first = keyword[0]`
		`+ keyword_second = keyword[1]`
		`+ # if we hit two keywords which are synonyms for each other,`
		`+ # move on, test is invalid`
		`+ if keyword_first.replace("fe", "freezeexception") == keyword_second.replace("fe", "freezeexception"):`
		`+ return None`
		`+ vote_first = vote[0]`
		`+ vote_second = vote[1]`
		`+ first = (vote_first, keyword_first) if inversed[0] else (keyword_first, vote_first)`
		`+ second = (vote_second, keyword_second) if inversed[1] else (keyword_second, vote_second)`
		`+ comments = [pagure_bot.Comment(c) for c in [`
		`+ {`
		`+ 'id': 1,`
		`+ 'comment': "foobar\n%s %s\nbarbaz\n%s %s" % (first + second),`
		`+ 'user': {`
		`+ 'name': stub_admin_user1`
		`+ }`
		`+ }`
		`+ ]]`
		`+`
		`+ trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`+ assert trackers[expand_fe(keyword_first)].votes == {`
		`+ stub_admin_user1: {`
		`+ 'vote': '%s' % vote_first,`
		`+ 'comment_id': 1`
		`+ }`
		`+ }`
		`+ assert trackers[expand_fe(keyword_second)].votes == {`
		`+ stub_admin_user1: {`
		`+ 'vote': '%s' % vote_second,`
		`+ 'comment_id': 1`
		`+ }`
		`+ }`
		`+`
		`+`
		`+ class TestPagureBotSinglelineMultivote(object):`
		`+ @pytest.mark.parametrize("inversed", itertools.product([False, True], [False, True]))`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("vote", [('+1', '-1'), ('-1', '+1'), ('+1', '0'),`
		`('-1', '0'), ('0', '-1'), ('0', '+1')])`
		`- def test_multiline_multivote_same(self, vote, keyword):`
		`+ def test_singleline_multivote_same(self, vote, keyword, inversed):`
		`vote_first = vote[0]`
		`vote_second = vote[1]`
		`+ first = (vote_first, keyword) if inversed[0] else (keyword, vote_first)`
		`+ second = (vote_second, keyword) if inversed[1] else (keyword, vote_second)`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`'id': 1,`
		`- 'comment': "foobar\n%s %s\nbarbaz\n%s %s" % (keyword, vote_first,`
		`- keyword, vote_second),`
		`+ # Votes should be accepted with second taking precedence`
		`+ 'comment': "%s %s %s %s" % (first + second),`
		`'user': {`
		`'name': stub_admin_user1`
		`}`
		`@@ -262,27 +382,34 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {`
		`+ assert trackers[expand_fe(keyword)].votes == {`
		`stub_admin_user1: {`
		`'vote': '%s' % vote_second,`
		`'comment_id': 1`
		`}`
		`}`

		`- @pytest.mark.parametrize("keyword", itertools.permutations(pagure_bot.TRACKER_KEYWORDS, 2))`
		`+ @pytest.mark.parametrize("inversed", itertools.product([False, True], [False, True]))`
		`+ @pytest.mark.parametrize("keyword", itertools.permutations(EXTENDED_TRACKERS, 2))`
		`@pytest.mark.parametrize("vote", [('+1', '-1'), ('-1', '+1'), ('+1', '0'),`
		`('-1', '0'), ('0', '-1'), ('0', '+1'),`
		`('0', '0'), ('-1', '-1'), ('+1', '+1')])`
		`- def test_multiline_multivote_different(self, vote, keyword):`
		`+ def test_singleline_multivote_different(self, vote, keyword, inversed):`
		`keyword_first = keyword[0]`
		`keyword_second = keyword[1]`
		`+ # if we hit two keywords which are synonyms for each other,`
		`+ # move on, test is invalid`
		`+ if keyword_first.replace("fe", "freezeexception") == keyword_second.replace("fe", "freezeexception"):`
		`+ return None`
		`vote_first = vote[0]`
		`vote_second = vote[1]`
		`+ first = (vote_first, keyword_first) if inversed[0] else (keyword_first, vote_first)`
		`+ second = (vote_second, keyword_second) if inversed[1] else (keyword_second, vote_second)`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`'id': 1,`
		`- 'comment': "foobar\n%s %s\nbarbaz\n%s %s" % (keyword_first, vote_first,`
		`- keyword_second, vote_second),`
		`+ # Both votes should be accepted`
		`+ 'comment': "%s %s %s %s" % (first + second),`
		`'user': {`
		`'name': stub_admin_user1`
		`}`
		`@@ -290,22 +417,78 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword_first].votes == {`
		`+ assert trackers[expand_fe(keyword_first)].votes == {`
		`stub_admin_user1: {`
		`'vote': '%s' % vote_first,`
		`'comment_id': 1`
		`}`
		`}`
		`- assert trackers[keyword_second].votes == {`
		`+ assert trackers[expand_fe(keyword_second)].votes == {`
		`stub_admin_user1: {`
		`'vote': '%s' % vote_second,`
		`'comment_id': 1`
		`}`
		`}`

		`+ def test_singleline_multivote_three_votes_two_inversed(self):`
		`+ comments = [pagure_bot.Comment(c) for c in [`
		`+ {`
		`+ 'id': 1,`
		`+ 'comment': "BetaBlocker -1 +1 FinalBlocker +1 BetaFE",`
		`+ 'user': {`
		`+ 'name': stub_admin_user1`
		`+ }`
		`+ }`
		`+ ]]`
		`+`
		`+ trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`+ assert trackers['betablocker'].votes == {`
		`+ stub_admin_user1: {`
		`+ 'vote': '-1',`
		`+ 'comment_id': 1`
		`+ }`
		`+ }`
		`+ assert trackers['finalblocker'].votes == {`
		`+ stub_admin_user1: {`
		`+ 'vote': '+1',`
		`+ 'comment_id': 1`
		`+ }`
		`+ }`
		`+ assert trackers['betafreezeexception'].votes == {`
		`+ stub_admin_user1: {`
		`+ 'vote': '+1',`
		`+ 'comment_id': 1`
		`+ }`
		`+ }`
		`+`
		`+ def test_singleline_multivote_summary_rejected(self):`
		`+ """Votes should be rejected due to additional text. We need`
		`+ both comments to check we reject both even and odd word counts`
		`+ """`
		`+ comments = [pagure_bot.Comment(c) for c in [`
		`+ {`
		`+ 'id': 1,`
		`+ 'comment': "+1 FinalBlocker -1 BetaBlocker lalala",`
		`+ 'user': {`
		`+ 'name': stub_admin_user1`
		`+ }`
		`+ },`
		`+ {`
		`+ 'id': 2,`
		`+ 'comment': "+1 FinalBlocker -1 BetaBlocker lalala foofoofoo",`
		`+ 'user': {`
		`+ 'name': stub_admin_user1`
		`+ }`
		`+ }`
		`+ ]]`
		`+`
		`+ trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`+ assert trackers["finalblocker"].votes == {}`
		`+ assert trackers["betablocker"].votes == {}`
		`+`

		`class TestPagureBotAgreedRevote(object):`
		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("outcome", ['Accepted', 'Rejected'])`
		`def test_agreed_single_acceptedrejected_needs_summary(self, outcome, keyword, setup_stubs):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`@@ -326,15 +509,15 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {`
		`+ assert trackers[expand_fe(keyword)].votes == {`
		`stub_admin_user1: {`
		`'vote': '+1',`
		`'comment_id': 1`
		`}`
		`}`
		`- assert trackers[keyword].need_summary_post`
		`+ assert trackers[expand_fe(keyword)].need_summary_post`

		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("outcome", ['Accepted', 'Rejected'])`
		`def test_agreed_single_acceptedrejected_no_justification_needs_summary(self, outcome, keyword, setup_stubs):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`@@ -355,15 +538,15 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {`
		`+ assert trackers[expand_fe(keyword)].votes == {`
		`stub_admin_user1: {`
		`'vote': '+1',`
		`'comment_id': 1`
		`}`
		`}`
		`- assert trackers[keyword].need_summary_post`
		`+ assert trackers[expand_fe(keyword)].need_summary_post`

		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("outcome", ['Accepted', 'Rejected'])`
		`def test_agreed_single_acceptedrejected_by_impostor_need_summary(self, outcome, keyword, setup_stubs):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`@@ -383,7 +566,7 @@`
		`},`
		`{`
		`'id': 3,`
		`- 'comment': "%s\n%s" % (pagure_bot.CLOSED_VOTES_HEADER, keyword),`
		`+ 'comment': "%s\n%s" % (pagure_bot.CLOSED_VOTES_HEADER, expand_fe(keyword)),`
		`'user': {`
		`'name': 'impostor'`
		`}`
		`@@ -391,15 +574,15 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {`
		`+ assert trackers[expand_fe(keyword)].votes == {`
		`stub_admin_user1: {`
		`'vote': '+1',`
		`'comment_id': 1`
		`}`
		`}`
		`- assert trackers[keyword].need_summary_post`
		`+ assert trackers[expand_fe(keyword)].need_summary_post`

		`- @pytest.mark.parametrize("keywords", powerset(pagure_bot.TRACKER_KEYWORDS))`
		`+ @pytest.mark.parametrize("keywords", powerset(EXTENDED_TRACKERS))`
		`def test_agreed_multiple_accepts_need_summary(self, keywords, setup_stubs):`
		`outcomes = " ".join(["accepted%s" % keyword for keyword in keywords])`
		`comments = [pagure_bot.Comment(c) for c in [`
		`@@ -414,9 +597,9 @@`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`for keyword in keywords:`
		`- assert trackers[keyword].need_summary_post`
		`+ assert trackers[expand_fe(keyword)].need_summary_post`

		`- @pytest.mark.parametrize("keywords", powerset(pagure_bot.TRACKER_KEYWORDS))`
		`+ @pytest.mark.parametrize("keywords", powerset(EXTENDED_TRACKERS))`
		`def test_agreed_multiple_accepts_no_justification_need_summary(self, keywords, setup_stubs):`
		`outcomes = " ".join(["accepted%s" % keyword for keyword in keywords])`
		`comments = [pagure_bot.Comment(c) for c in [`
		`@@ -431,9 +614,9 @@`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`for keyword in keywords:`
		`- assert trackers[keyword].need_summary_post`
		`+ assert trackers[expand_fe(keyword)].need_summary_post`

		`- @pytest.mark.parametrize("keywords", powerset(pagure_bot.TRACKER_KEYWORDS))`
		`+ @pytest.mark.parametrize("keywords", powerset(EXTENDED_TRACKERS))`
		`def test_agreed_multiple_accepts_has_summary(self, keywords, setup_stubs):`
		`outcomes = " ".join(["accepted%s" % keyword for keyword in keywords])`
		`comments = [pagure_bot.Comment(c) for c in [`
		`@@ -446,7 +629,8 @@`
		`},`
		`{`
		`'id': 2,`
		`- 'comment': "%s\n%s" % (pagure_bot.CLOSED_VOTES_HEADER, "\n".join(keywords)),`
		`+ 'comment': "%s\n%s" % (pagure_bot.CLOSED_VOTES_HEADER,`
		`+ "\n".join([expand_fe(keyword) for keyword in keywords])),`
		`'user': {`
		`'name': app.config['PAGURE_BOT_USERNAME']`
		`}`
		`@@ -455,9 +639,9 @@`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`for keyword in keywords:`
		`- assert not trackers[keyword].need_summary_post`
		`+ assert not trackers[expand_fe(keyword)].need_summary_post`

		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`def test_revote_single(self, keyword, setup_stubs):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`@@ -475,13 +659,46 @@`
		`}`
		`},`
		`{`
		`- 'id': 2,`
		`- 'comment': "%s\n%s" % (pagure_bot.CLOSED_VOTES_HEADER, keyword),`
		`+ 'id': 3,`
		`+ 'comment': "%s\n%s" % (pagure_bot.CLOSED_VOTES_HEADER, expand_fe(keyword)),`
		`'user': {`
		`'name': app.config['PAGURE_BOT_USERNAME']`
		`}`
		`},`
		`{`
		`+ 'id': 4,`
		`+ 'comment': "REVOTE %s" % keyword,`
		`+ 'user': {`
		`+ 'name': stub_admin_user1`
		`+ }`
		`+ }`
		`+ ]]`
		`+`
		`+ trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`+ assert trackers[expand_fe(keyword)].votes == {}`
		`+ assert not trackers[expand_fe(keyword)].need_summary_post`
		`+`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`+ def test_revote_without_previous_summary(self, keyword, setup_stubs):`
		`+ '''If the summary didn't arrive in time and the tracker got revoted in`
		`+ the meantime, no need to supply summary anymore.`
		`+ '''`
		`+ comments = [pagure_bot.Comment(c) for c in [`
		`+ {`
		`+ 'id': 1,`
		`+ 'comment': "%s +1" % keyword,`
		`+ 'user': {`
		`+ 'name': stub_admin_user1`
		`+ }`
		`+ },`
		`+ {`
		`+ 'id': 2,`
		`+ 'comment': "AGREED Accepted%s foo bar" % keyword,`
		`+ 'user': {`
		`+ 'name': stub_admin_user1`
		`+ }`
		`+ },`
		`+ {`
		`'id': 3,`
		`'comment': "REVOTE %s" % keyword,`
		`'user': {`
		`@@ -491,12 +708,16 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {}`
		`- assert not trackers[keyword].need_summary_post`
		`+ assert trackers[expand_fe(keyword)].votes == {}`
		`+ assert not trackers[expand_fe(keyword)].need_summary_post`

		`@pytest.mark.parametrize("keyword_a, keyword_b",`
		`- itertools.permutations(pagure_bot.TRACKER_KEYWORDS, 2))`
		`+ itertools.permutations(EXTENDED_TRACKERS, 2))`
		`def test_agreed_stops_counting_counts_other(self, keyword_a, keyword_b, setup_stubs):`
		`+ # if we hit two keywords which are synonyms for each other,`
		`+ # move on`
		`+ if keyword_a.replace("fe", "freezeexception") == keyword_b.replace("fe", "freezeexception"):`
		`+ return None`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`'id': 1,`
		`@@ -522,13 +743,13 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword_a].votes == {`
		`+ assert trackers[expand_fe(keyword_a)].votes == {`
		`stub_admin_user1: {`
		`'vote': '+1',`
		`'comment_id': 1`
		`}`
		`}`
		`- assert trackers[keyword_b].votes == {`
		`+ assert trackers[expand_fe(keyword_b)].votes == {`
		`stub_admin_user1: {`
		`'vote': '+1',`
		`'comment_id': 1`
		`@@ -541,13 +762,58 @@`


		`class TestPagureBotWhitespaces(object):`
		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("inversed", [False, True])`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`+ @pytest.mark.parametrize("vote", ['+1', '-1', '0'])`
		`+ def test_trailing_whitespace(self, vote, keyword, inversed):`
		`+ comments = [pagure_bot.Comment(c) for c in [`
		`+ {`
		`+ 'id': 1,`
		`+ 'comment': "%s %s " % ((vote, keyword) if inversed else (keyword, vote)),`
		`+ 'user': {`
		`+ 'name': stub_admin_user1`
		`+ }`
		`+ }`
		`+ ]]`
		`+`
		`+ trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`+ assert trackers[expand_fe(keyword)].votes == {`
		`+ stub_admin_user1: {`
		`+ 'vote': '%s' % vote,`
		`+ 'comment_id': 1`
		`+ }`
		`+ }`
		`+`
		`+ @pytest.mark.parametrize("inversed", [False, True])`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`+ @pytest.mark.parametrize("vote", ['+1', '-1', '0'])`
		`+ def test_leading_whitespace(self, vote, keyword, inversed):`
		`+ comments = [pagure_bot.Comment(c) for c in [`
		`+ {`
		`+ 'id': 1,`
		`+ 'comment': " %s %s" % ((vote, keyword) if inversed else (keyword, vote)),`
		`+ 'user': {`
		`+ 'name': stub_admin_user1`
		`+ }`
		`+ }`
		`+ ]]`
		`+`
		`+ trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`+ assert trackers[expand_fe(keyword)].votes == {`
		`+ stub_admin_user1: {`
		`+ 'vote': '%s' % vote,`
		`+ 'comment_id': 1`
		`+ }`
		`+ }`
		`+`
		`+ @pytest.mark.parametrize("inversed", [False, True])`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("vote", ['+1', '-1', '0'])`
		`- def test_trailing_whitespace(self, vote, keyword):`
		`+ def test_whitespaces_inside_vote(self, vote, keyword, inversed):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`'id': 1,`
		`- 'comment': "%s %s " % (keyword, vote),`
		`+ 'comment': "%s %s" % ((vote, keyword) if inversed else (keyword, vote)),`
		`'user': {`
		`'name': stub_admin_user1`
		`}`
		`@@ -555,20 +821,21 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {`
		`+ assert trackers[expand_fe(keyword)].votes == {`
		`stub_admin_user1: {`
		`'vote': '%s' % vote,`
		`'comment_id': 1`
		`}`
		`}`

		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("inversed", [False, True])`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("vote", ['+1', '-1', '0'])`
		`- def test_whitespaces_inside_vote(self, vote, keyword):`
		`+ def test_trailing_leading_inside_whitespace(self, vote, keyword, inversed):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`'id': 1,`
		`- 'comment': "%s %s" % (keyword, vote),`
		`+ 'comment': " %s %s " % ((vote, keyword) if inversed else (keyword, vote)),`
		`'user': {`
		`'name': stub_admin_user1`
		`}`
		`@@ -576,20 +843,44 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {`
		`+ assert trackers[expand_fe(keyword)].votes == {`
		`stub_admin_user1: {`
		`'vote': '%s' % vote,`
		`'comment_id': 1`
		`}`
		`}`

		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("inversed", [False, True])`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("vote", ['+1', '-1', '0'])`
		`- def test_newline_inside_vote_ignored(self, vote, keyword):`
		`+ def test_newline_inside_vote_ignored(self, vote, keyword, inversed):`
		`+ comments = [pagure_bot.Comment(c) for c in [`
		`+ {`
		`+ 'id': 1,`
		`+ 'comment': "%s\n%s" % ((vote, keyword) if inversed else (keyword, vote)),`
		`+ 'user': {`
		`+ 'name': stub_admin_user1`
		`+ }`
		`+ }`
		`+ ]]`
		`+`
		`+ trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`+ assert trackers[expand_fe(keyword)].votes == {}`
		`+`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`+ @pytest.mark.parametrize("outcome", ['Accepted', 'Rejected'])`
		`+ def test_agreed_single_acceptedrejected_leading_wspace_needs_summary(self, outcome, keyword, setup_stubs):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`{`
		`'id': 1,`
		`- 'comment': "%s\n%s" % (keyword, vote),`
		`+ 'comment': "%s +1" % keyword,`
		`+ 'user': {`
		`+ 'name': stub_admin_user1`
		`+ }`
		`+ },`
		`+ {`
		`+ 'id': 2,`
		`+ 'comment': " AGREED %s%s foo bar" % (outcome, keyword),`
		`'user': {`
		`'name': stub_admin_user1`
		`}`
		`@@ -597,9 +888,9 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].votes == {}`
		`+ assert trackers[expand_fe(keyword)].need_summary_post`

		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("outcome", ['Accepted', 'Rejected'])`
		`def test_agreed_single_acceptedrejected_trailing_wspace_needs_summary(self, outcome, keyword, setup_stubs):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`@@ -620,9 +911,9 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].need_summary_post`
		`+ assert trackers[expand_fe(keyword)].need_summary_post`

		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("outcome", ['Accepted', 'Rejected'])`
		`def test_agreed_single_acceptedrejected_multi_wspace_inside_needs_summary(self, outcome, keyword, setup_stubs):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`@@ -635,7 +926,7 @@`
		`},`
		`{`
		`'id': 2,`
		`- 'comment': "AGREED %s%s foo bar " % (outcome, keyword),`
		`+ 'comment': " AGREED %s%s foo bar " % (outcome, keyword),`
		`'user': {`
		`'name': stub_admin_user1`
		`}`
		`@@ -643,9 +934,9 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert trackers[keyword].need_summary_post`
		`+ assert trackers[expand_fe(keyword)].need_summary_post`

		`- @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)`
		`+ @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)`
		`@pytest.mark.parametrize("outcome", ['Accepted', 'Rejected'])`
		`def test_agreed_single_acceptedrejected_newline_inside(self, outcome, keyword, setup_stubs):`
		`comments = [pagure_bot.Comment(c) for c in [`
		`@@ -666,9 +957,9 @@`
		`]]`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`- assert not trackers[keyword].need_summary_post`
		`+ assert not trackers[expand_fe(keyword)].need_summary_post`

		`- @pytest.mark.parametrize("keywords", powerset(pagure_bot.TRACKER_KEYWORDS))`
		`+ @pytest.mark.parametrize("keywords", powerset(EXTENDED_TRACKERS))`
		`def test_agreed_multiple_accepts_multi_wspace_inside_need_summary(self, keywords, setup_stubs):`
		`outcomes = " ".join(["accepted%s" % keyword for keyword in keywords])`
		`comments = [pagure_bot.Comment(c) for c in [`
		`@@ -683,7 +974,7 @@`

		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`for keyword in keywords:`
		`- assert trackers[keyword].need_summary_post`
		`+ assert trackers[expand_fe(keyword)].need_summary_post`


		`class TestPagureBotHaywire(object):`
		`@@ -776,119 +1067,81 @@`
		`assert not pagure_bot.bot_loop_detected(comments)`


		`- realworld_comments = [`
		`- {`
		`- "comment": "bork bork\r\nFinalBlocker +1\r\nBetaBlocker -1",`
		`- "id": 1,`
		`- "user": {`
		`- "name": "lbrabec"`
		`- }`
		`- },`
		`- {`
		`- "comment": "Obvious\r\nFinalBlocker +1\r\nBetaFE +1\r\n\r\nBut I don't like that much:\r\n"`
		`+ class TestPagureBotRealWorld(object):`
		`+`
		`+ realworld_comments = [`
		`+ {"comment": "bork bork\r\nFinalBlocker +1\r\nBetaBlocker -1",`
		`+ "id": 1,`
		`+ "user": {"name": "lbrabec"}`
		`+ },`
		`+ {"comment": "Obvious\r\nFinalBlocker +1\r\nBetaFE +1\r\n\r\nBut I don't like that much:\r\n"`
		`"BetaBlocker -1\r\n\r\nFor FinalFE, gee, I don't know:\r\nFinalFE 0",`
		`- "id": 2,`
		`- "user": {`
		`- "name": "kparal"`
		`- }`
		`- },`
		`- {`
		`- "comment": "Y'all are insane. Foo is working great for me and I'm ashamed that this"`
		`+ "id": 2,`
		`+ "user": {"name": "kparal"}`
		`+ },`
		`+ {"comment": "Y'all are insane. Foo is working great for me and I'm ashamed that this"`
		`"was ever considered to be a blocker.\r\n\r\nFinalBlocker -1\r\n"`
		`"BetaBlocker -1\r\nBetaFE -1\r\nOP -1",`
		`- "id": 3,`
		`- "user": {`
		`- "name": "tflink"`
		`- }`
		`- },`
		`- {`
		`- "comment": "On the off chance that my vote could be counted twice ...\r\n\r\n"`
		`+ "id": 3,`
		`+ "user": {"name": "tflink"}`
		`+ },`
		`+ {"comment": "On the off chance that my vote could be counted twice ...\r\n\r\n"`
		`"FinalBlocker -1\r\nAlphaBlocker -2",`
		`- "id": 4,`
		`- "user": {`
		`- "name": "tflink"`
		`- }`
		`- },`
		`- {`
		`- "comment": "or maybe I'm wrong .... but that's never happened before in all "`
		`+ "id": 4,`
		`+ "user": {"name": "tflink"}`
		`+ },`
		`+ {"comment": "or maybe I'm wrong .... but that's never happened before in all "`
		`"of recorded history\r\n\r\nBetaFE +1\r\nBetaBlocker +!",`
		`- "id": 5,`
		`- "user": {`
		`- "name": "tflink"`
		`- }`
		`- },`
		`- {`
		`- "comment": "Yeah, those who vote against it do not deserve to use Fedora, "`
		`+ "id": 5,`
		`+ "user": {"name": "tflink"}`
		`+ },`
		`+ {"comment": "Yeah, those who vote against it do not deserve to use Fedora, "`
		`"go to Windows.\r\n\r\nBetaBlocker +1\r\nFinalBlocker +1\r\n"`
		`"BetaFE +1\r\nFinalFE +1",`
		`- "id": 6,`
		`- "user": {`
		`- "name": "lruzicka"`
		`- }`
		`- },`
		`- {`
		- "comment": "Let's be sneaky\r\n`PreviousRelease +1`\r\n\r\n"
		`+ "id": 6,`
		`+ "user": {"name": "lruzicka"}`
		`+ },`
		+ {"comment": "Let's be sneaky\r\n`PreviousRelease +1`\r\n\r\n"
		"And\r\n```\r\n0Day 0\r\n```",
		`- "id": 7,`
		`- "user": {`
		`- "name": "kparal"`
		`- }`
		`- },`
		`- {`
		`- "comment": "日本語で書いたら壊しますか\r\n\r\nBetaFE 👎\r\nFinalBlocker: ＋1\r\n\r\nBetaBlocker: +1",`
		`- "id": 8,`
		`- "user": {`
		`- "name": "tflink"`
		`- }`
		`- },`
		`- {`
		`- "comment": "Nejkulatoulinkatejsi kulicka",`
		`- "id": 9,`
		`- "user": {`
		`- "name": "jskladan"`
		`- }`
		`- },`
		`- {`
		`- "comment": "AGREED AcceptedFinalBlocker Lorem ipsum dolor sit amet",`
		`- "id": 11,`
		`- "user": {`
		`- "name": "lbrabec"`
		`- }`
		`- },`
		`- {`
		`- "comment": "The following votes have been closed:\n\n"`
		`+ "id": 7,`
		`+ "user": {"name": "kparal"}`
		`+ },`
		`+ {"comment": "日本語で書いたら壊しますか\r\n\r\nBetaFE 👎\r\nFinalBlocker: ＋1\r\n\r\nBetaBlocker: +1",`
		`+ "id": 8,`
		`+ "user": {"name": "tflink"}`
		`+ },`
		`+ {"comment": "Nejkulatoulinkatejsi kulicka",`
		`+ "id": 9,`
		`+ "user": {"name": "jskladan"}`
		`+ },`
		`+ {"comment": "AGREED AcceptedFinalBlocker Lorem ipsum dolor sit amet",`
		`+ "id": 11,`
		`+ "user": {"name": "lbrabec"}`
		`+ },`
		`+ {"comment": "The following votes have been closed:\n\n"`
		`"* Accepted FinalBlocker (+3, 0, -1)\n"`
		`" * +1 by [lbrabec](#comment-1), [kparal](#comment-2), "`
		`"[lruzicka](#comment-6)\n"`
		`" * -1 by [tflink](#comment-4)\n",`
		`- "id": 12,`
		`- "user": {`
		`- "name": "blockerbot"`
		`- }`
		`- },`
		`- {`
		`- "comment": "AGREED RejectedBetaBlocker Lorem ipsum dolor sit amet",`
		`- "id": 13,`
		`- "user": {`
		`- "name": "lbrabec"`
		`- }`
		`- },`
		`- {`
		`- "comment": "The following votes have been closed:\n\n"`
		`+ "id": 12,`
		`+ "user": {"name": "blockerbot"}`
		`+ },`
		`+ {"comment": "AGREED RejectedBetaBlocker Lorem ipsum dolor sit amet",`
		`+ "id": 13,`
		`+ "user": {"name": "lbrabec"}`
		`+ },`
		`+ {"comment": "The following votes have been closed:\n\n"`
		`"* Rejected BetaBlocker (+1, 0, -3)\n"`
		`" * +1 by [lruzicka](#comment-6)\n"`
		`" * -1 by [lbrabec](#comment-1), [kparal](#comment-2), "`
		`"[tflink](#comment-3)\n",`
		`- "id": 14,`
		`- "user": {`
		`- "name": "blockerbot"`
		`- }`
		`- }`
		`- ]`
		`+ "id": 14,`
		`+ "user": {"name": "blockerbot"}`
		`+ }`
		`+ ]`

		`- class TestPagureBotRealWorld(object):`
		`def test_realworld(self, setup_stubs):`
		`- comments = [pagure_bot.Comment(c) for c in realworld_comments]`
		`+ comments = [pagure_bot.Comment(c) for c in self.realworld_comments]`
		`trackers = pagure_bot.parse_comments_to_trackers(comments)`
		`non_voting_users, last_comment_id = pagure_bot.voting_info(comments, trackers)`

		`@@ -930,7 +1183,7 @@`
		`'comment_id': 3`
		`}`
		`}`
		`- assert trackers['betafe'].votes == {`
		`+ assert trackers['betafreezeexception'].votes == {`
		`'kparal': {`
		`'vote': '+1',`
		`'comment_id': 2`
		`@@ -944,7 +1197,7 @@`
		`'comment_id': 5`
		`}`
		`}`
		`- assert trackers['finalfe'].votes == {`
		`+ assert trackers['finalfreezeexception'].votes == {`
		`'kparal': {`
		`'vote': '0',`
		`'comment_id': 2`

kparal commented 3 years ago

This is a replacement for #161, because I couldn't add commits to Adam's branch. Please review the additional commits one by one. I intend to squash them a bit before pushing, don't worry.

zuul commented 3 years ago

Build succeeded.

blockerbugs-pytest : SUCCESS in 1m 42s

adamwill commented 3 years ago

So mostly this seems fine to me. But:

"handle "FE" abbreviation in a safer way" seems to add an awful lot more complexity for, at the present time, exactly zero benefit. It'll never have any benefit unless we put the string "fe" into a tracker keyword in a way that doesn't mean "freeze exception", which hasn't happened in the near-decade since we invented this system and doesn't seem terribly likely to happen. It also seems more fragile than my "replace the text in exactly the right place" approach, because it means we have to remember to make sure lots of different places call the replacement function, and conceivably a call to it might need to be added to any new code that gets added and someone might not realize that. This was kinda why I picked the "find one key point to do the replacement" approach in the first place.
"make agreed_revote_parser more standalone" - this seems kinda neutral to me. It's not worse, but it doesn't really seem any better. It's just a deck chair re-arrangement.
I'm not a huge fan of "make comment parsing more reliable" as it stands because it involves kinda duplicating stuff from elsewhere. It just feels awkward to be essentially re-parsing all these lines that we actually have control over the contents of already - it's the parser functions that write the text we're parsing here, it's not being just dumped in unmodified from the comments. Re-parsing in this much detail seems unnecessary.

Edited 3 years ago by adamwill

kparal commented 3 years ago

"handle "FE" abbreviation in a safer way"

What I did was to place the "fe" expansion into the parser method, because that's the code that understands the internals of the command. You can pinpoint the expansion accurately in there. On the other hand, if you try to replace it afterwards (in Comment.keywords() or elsewhere), you no longer understand the structure and need to do a "naive" approach of replacing everything that contains "fe".

The complexity you talk about is the new expand_fe() function, I assume (because all the rest are just a few lines of changes). Yes, it's 20 lines instead of 1, but it's very self-contained and documented, and it's replacing exactly the substring it should, instead of any substring that happens to be "fe". I can easily change the function to the str.replace() one liner. But if I can do it reliably, quite easily, I don't see a point in keeping it unreliable. Sure, it's not likely that we'll have "fe" somewhere else in our tracker keywords in the near future, but this is not just about tracker keywords. Before this patch, the whole command line was str.replace'd. That didn't matter because we don't process AGREED summary at the moment, but we will, and this would break it (also mentioned below).

"make agreed_revote_parser more standalone"

OK. I feel better about it, because the previous state felt like half-parsing the line in advance, then forwarding implementation details to a function which did the other half, for no good reason. The Comment.keywords() shouldn't contain that logic, if it doesn't have to.

"make comment parsing more reliable"

I agree that it duplicates parsing and that it's stupid. However, it's miles better than self.outcome = keyword[7:15], don't you think? The problem here is that this whole system is weird. We parse the text the first time, extract just the commands (votes, agreed, revote), but we don't make them easily digestible for future processing. We keep them still in plaintext like BetaBlocker +1 or AGREED AcceptedBetaBlocker. So we need to parse them again. That is silly. I just reused the regular expressions to make the parsing behave the same way as when we parse it for the first time, instead of quick hacks like the one above. Surely it's a great coincidence that "accepted" and "rejected" have the same number of letters and that's why we can use keyword[7:15], but... come on? The other quick hack was self.tracker in keyword, which works fine now, but only because we ignore the AGREED summary at the moment. Once we start processing it (and we will need to, if we want to automatically update bugzilla), this will break and we'd need something like my current PR anyway.

The proper way to solve this is to convert the parsed keywords to something that's easily digestible by machines, so e.g. BetaBlocker +1 could be converted into e.g. namedtupleVote(tracker='betablocker', vote='+1'), and AGREED AcceptedBetaBlocker into e.g. namedtupleAgreed(tracker='betablocker', outcome='accepted'). But I didn't want to make this change as part of this PR. So I kept the spirit (double-parsing), just made it consistent and safer. I'd like to replace double-parsing with a better approach in the future.

Please note that I added a few more things on top of the old code, and that's why the diff seems larger. I fixed FIXME log rejected (non admin) users? and I added a safeguard against processing bot comments. Because of the first one, I needed to define some variables to be reused in multiple places. If I didn't do that, the change would be much shorter.

Do you consider my arguments convincing? Please let me know, and thanks for your review :-)

Edited 3 years ago by kparal

kparal commented 3 years ago

The proper way to solve this is to convert the parsed keywords to something that's easily digestible by machines

I implemented this in #164. It should hopefully resolve most of your concerns here. The expand_fe complexity is gone, the duplicate parsing is gone :-)