ReviewBot: add_review(): provide allow_duplicate parameter and default of False.

Also update calls based on default behavior change. As described in the
comment above add_review() there seems to be no reason to ever add a
duplicate review.

The check in leaper would skip unless state == declined. In such a case
the review would automatically be reopened along with the request so that
logic is unnecessary.

ReviewBot.py

@@ -171,7 +171,6 @@ class ReviewBot(object):
         if state == 'declined':
             if self.review_mode == 'fallback-onfail':
                 self.logger.info("%s needs fallback reviewer"%req.reqid)
-                # don't check duplicates, in case review was re-opened
                 self.add_review(req, by_group=by_group, by_user=by_user, msg="Automated review failed. Needs fallback reviewer.")
                 newstate = 'accepted'
         elif self.review_mode == 'fallback-always':
@@ -187,8 +186,13 @@ class ReviewBot(object):
         else:
             self.logger.debug("%s review not changed"%(req.reqid))
 
-    # note we intentionally don't check for duplicate review here!
+    # allow_duplicate=True should only be used if it makes sense to force a
-    def add_review(self, req, by_group=None, by_user=None, by_project = None, by_package = None, msg=None):
+    # re-review in a scenario where the bot adding the review will rerun.
+    # Normally a declined review will automatically be reopened along with the
+    # request and any other bot reviews already added will not be touched unless
+    # the issuing bot is rerun which does not fit normal workflow.
+    def add_review(self, req, by_group=None, by_user=None, by_project=None, by_package=None,
+                   msg=None, allow_duplicate=False):
         query = {
             'cmd': 'addreview'
         }
@@ -203,6 +207,19 @@ class ReviewBot(object):
         else:
             raise osc.oscerr.WrongArgs("missing by_*")
 
+        for r in req.reviews:
+            print(r.by_group, r.by_project, r.by_package, r.by_user, r.state)
+            if (r.by_group == by_group and
+                r.by_project == by_project and
+                r.by_package == by_package and
+                r.by_user == by_user and
+                # Only duplicate when allow_duplicate and state != new.
+                (not allow_duplicate or r.state == 'new')):
+                del query['cmd']
+                self.logger.info('skipped adding duplicate review for {}'.format(
+                    '/'.join(query.values())))
+                return
+
         u = osc.core.makeurl(self.apiurl, ['request', req.reqid], query)
         if self.dryrun:
             self.logger.info('POST %s' % u)

check_maintenance_incidents.py

@@ -55,7 +55,6 @@ class MaintenanceChecker(ReviewBot.ReviewBot):
         project = a.tgt_releaseproject if a.type == 'maintenance_incident' else req.actions[0].tgt_project
         root = owner_fallback(self.apiurl, project, package)
 
-        package_reviews = set((r.by_project, r.by_package) for r in req.reviews if r.by_project)
         for p in root.findall('./owner'):
             prj = p.get("project")
             pkg = p.get("package")
@@ -63,9 +62,6 @@ class MaintenanceChecker(ReviewBot.ReviewBot):
             if prj.startswith('openSUSE:Leap') or prj.startswith('openSUSE:1'):
                 self.logger.debug("%s looks wrong as maintainer, skipped", prj)
                 continue
-            if ((prj, pkg) in package_reviews):
-                self.logger.debug("%s/%s already is a reviewer, not adding again" % (prj, pkg))
-                continue
             self.add_review(req, by_project = prj, by_package = pkg,
                     msg = 'Submission for {} by someone who is not maintainer in the devel project ({}). Please review'.format(pkg, prj) )
 

leaper.py

@@ -440,29 +440,21 @@ class Leaper(ReviewBot.ReviewBot):
             self.comment_handler_lines_deduplicate()
             self.comment_write(state, result)
 
-        # list of tuple ('group', (states))
         add_review_groups = []
         if self.needs_release_manager:
-            add_review_groups.append((self.release_manager_group, ('new', 'accepted')))
+            add_review_groups.append(self.release_manager_group)
         if self.needs_reviewteam:
-            add_review_groups.append((self.review_team_group, None))
+            add_review_groups.append(self.review_team_group)
         if self.needs_legal_review:
-            add_review_groups.append((self.legal_review_group, None))
+            add_review_groups.append(self.legal_review_group)
         if self.needs_check_source and self.check_source_group is not None:
-            add_review_groups.append((self.check_source_group, None))
+            add_review_groups.append(self.check_source_group)
 
-        for (group, states) in add_review_groups:
+        for group in add_review_groups:
             if group is None:
                 continue
-            add_review = True
             self.logger.info("{0} needs review by [{1}](/group/show/{1})".format(req.reqid, group))
-            for r in req.reviews:
+            self.add_review(req, by_group=group):
-                if r.by_group == group and (states is None or r.state in states):
-                    add_review = False
-                    self.logger.debug("{} already is a reviewer".format(group))
-                    break
-            if add_review:
-                self.add_review(req, by_group=group):
 
         return request_ok