launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/launchpad/check-teamparticipation-fix-too-bug-897269 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #897269 in Launchpad itself: "check-teamparticipation.py does not fix spurious participation records"
  https://bugs.launchpad.net/launchpad/+bug/897269

For more details, see:
https://code.launchpad.net/~allenap/launchpad/check-teamparticipation-fix-too-bug-897269/+merge/85148

Fixes team participation issues when they're detected.
-- 
https://code.launchpad.net/~allenap/launchpad/check-teamparticipation-fix-too-bug-897269/+merge/85148
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/check-teamparticipation-fix-too-bug-897269 into lp:launchpad.

=== modified file 'cronscripts/check-teamparticipation.py'
--- cronscripts/check-teamparticipation.py	2011-11-04 21:48:55 +0000
+++ cronscripts/check-teamparticipation.py	2011-12-09 16:07:27 +0000
@@ -25,6 +25,7 @@
     check_teamparticipation_consistency,
     check_teamparticipation_self,
     fetch_team_participation_info,
+    fix_teamparticipation_consistency,
     )
 from lp.services.scripts.base import LaunchpadScript
 from lp.services.utils import (
@@ -59,8 +60,9 @@
         if self.options.save_info:
             save_bz2_pickle(participation_info, self.options.save_info)
         else:
-            check_teamparticipation_consistency(
+            errors = check_teamparticipation_consistency(
                 self.logger, participation_info)
+            fix_teamparticipation_consistency(self.logger, errors)
 
 
 if __name__ == '__main__':

=== modified file 'lib/lp/registry/scripts/teamparticipation.py'
--- lib/lp/registry/scripts/teamparticipation.py	2011-11-04 22:10:46 +0000
+++ lib/lp/registry/scripts/teamparticipation.py	2011-12-09 16:07:27 +0000
@@ -9,6 +9,7 @@
     "check_teamparticipation_consistency",
     "check_teamparticipation_self",
     "fetch_team_participation_info",
+    "fix_teamparticipation_consistency",
     ]
 
 from collections import (
@@ -26,17 +27,29 @@
 import transaction
 from zope.component import getUtility
 
-from canonical.database.sqlbase import quote
+from canonical.database.sqlbase import (
+    quote,
+    sqlvalues,
+    )
 from canonical.launchpad.webapp.interfaces import (
     IStoreSelector,
     MAIN_STORE,
+    MASTER_FLAVOR,
     SLAVE_FLAVOR,
     )
 from lp.registry.interfaces.teammembership import ACTIVE_STATES
 from lp.services.scripts.base import LaunchpadScriptFailure
 
 
-def get_store():
+def get_master_store():
+    """Return a master store.
+
+    Errors in `TeamPartipation` must be fixed in the master.
+    """
+    return getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR)
+
+
+def get_slave_store():
     """Return a slave store.
 
     Errors in `TeamPartipation` can be detected using a replicated copy.
@@ -57,7 +70,7 @@
              WHERE person = team)
            AND merged IS NULL
         """
-    non_self_participants = list(get_store().execute(query))
+    non_self_participants = list(get_slave_store().execute(query))
     if len(non_self_participants) > 0:
         log.warn(
             "Some people/teams are not members of themselves: %s",
@@ -77,7 +90,7 @@
            AND tp.person = tp2.team
            AND tp.id != tp2.id;
         """
-    circular_references = list(get_store().execute(query))
+    circular_references = list(get_slave_store().execute(query))
     if len(circular_references) > 0:
         raise LaunchpadScriptFailure(
             "Circular references found: %s" % circular_references)
@@ -117,7 +130,7 @@
 
 def fetch_team_participation_info(log):
     """Fetch people, teams, memberships and participations."""
-    slurp = partial(execute_long_query, get_store(), log, 10000)
+    slurp = partial(execute_long_query, get_slave_store(), log, 10000)
 
     people = dict(
         slurp(
@@ -209,3 +222,47 @@
             get_repr(error.team), error.type, people_repr)
 
     return errors
+
+
+def fix_teamparticipation_consistency(log, errors):
+    """Fix missing or spurious participations.
+
+    This function does not consult `TeamMembership` at all, so it /may/
+    introduce another participation inconsistency if the records that are the
+    subject of the given errors have been modified since being checked.
+
+    :param errors: An iterable of `ConsistencyError` tuples.
+    """
+    sql_missing = (
+        """
+        INSERT INTO TeamParticipation (team, person)
+        SELECT %(team)s, %(person)s
+        EXCEPT
+        SELECT team, person
+          FROM TeamParticipation
+         WHERE team = %(team)s
+           AND person = %(person)s
+        """)
+    sql_spurious = (
+        """
+        DELETE FROM TeamParticipation
+         WHERE team = %(team)s
+           AND person IN %(people)s
+        """)
+    store = get_master_store()
+    for error in errors:
+        if error.type == "missing":
+            for person in error.people:
+                statement = sql_missing % sqlvalues(
+                    team=error.team, person=person)
+                log.debug(statement)
+                store.execute(statement)
+                transaction.commit()
+        elif error.type == "spurious":
+            statement = sql_spurious % sqlvalues(
+                team=error.team, people=error.people)
+            log.debug(statement)
+            store.execute(statement)
+            transaction.commit()
+        else:
+            log.warn("Unrecognized error: %r", error)

=== modified file 'lib/lp/registry/tests/test_teammembership.py'
--- lib/lp/registry/tests/test_teammembership.py	2011-11-21 05:03:25 +0000
+++ lib/lp/registry/tests/test_teammembership.py	2011-12-09 16:07:27 +0000
@@ -62,6 +62,7 @@
     check_teamparticipation_self,
     ConsistencyError,
     fetch_team_participation_info,
+    fix_teamparticipation_consistency,
     )
 from lp.services.log.logger import BufferLogger
 from lp.testing import (
@@ -1157,32 +1158,58 @@
         self.assertEqual(0, len(out))
         self.failUnless(re.search('Circular references found', err))
 
-    def test_report_spurious_participants_of_people(self):
+    # A script to create two new people, where both participate in the first,
+    # and first is missing a self-participation.
+    script_create_inconsistent_participation = """
+        INSERT INTO
+            Person (id, name, displayname, creation_rationale)
+            VALUES (6969, 'bobby', 'Dazzler', 1);
+        INSERT INTO
+            Person (id, name, displayname, creation_rationale)
+            VALUES (6970, 'nobby', 'Jazzler', 1);
+        INSERT INTO
+            TeamParticipation (person, team)
+            VALUES (6970, 6969);
+        DELETE FROM
+            TeamParticipation
+            WHERE person = 6969
+              AND team = 6969;
+        """
+
+    def test_check_teamparticipation_consistency(self):
         """The script reports spurious participants of people.
 
         Teams can have multiple participants, but only the person should be a
         paricipant of him/herself.
         """
-        # Create two new people and make both participate in the first.
-        cursor().execute("""
-            INSERT INTO
-                Person (id, name, displayname, creation_rationale)
-                VALUES (6969, 'bobby', 'Dazzler', 1);
-            INSERT INTO
-                Person (id, name, displayname, creation_rationale)
-                VALUES (6970, 'nobby', 'Jazzler', 1);
-            INSERT INTO
-                TeamParticipation (person, team)
-                VALUES (6970, 6969);
-            """ % sqlvalues(approved=TeamMembershipStatus.APPROVED))
-        transaction.commit()
-        logger = BufferLogger()
-        self.addDetail("log", logger.content)
-        errors = check_teamparticipation_consistency(
-            logger, fetch_team_participation_info(logger))
-        self.assertEqual(
-            [ConsistencyError("spurious", 6969, [6970])],
-            errors)
+        cursor().execute(self.script_create_inconsistent_participation)
+        transaction.commit()
+        logger = BufferLogger()
+        self.addDetail("log", logger.content)
+        errors = check_teamparticipation_consistency(
+            logger, fetch_team_participation_info(logger))
+        errors_expected = [
+            ConsistencyError("spurious", 6969, [6970]),
+            ConsistencyError("missing", 6969, [6969]),
+            ]
+        self.assertContentEqual(errors_expected, errors)
+
+    def test_fix_teamparticipation_consistency(self):
+        """
+        `fix_teamparticipation_consistency` takes an iterable of
+        `ConsistencyError`s and attempts to repair the data.
+        """
+        cursor().execute(self.script_create_inconsistent_participation)
+        transaction.commit()
+        logger = BufferLogger()
+        self.addDetail("log", logger.content)
+        errors = check_teamparticipation_consistency(
+            logger, fetch_team_participation_info(logger))
+        self.assertNotEqual([], errors)
+        fix_teamparticipation_consistency(logger, errors)
+        errors = check_teamparticipation_consistency(
+            logger, fetch_team_participation_info(logger))
+        self.assertEqual([], errors)
 
     def test_load_and_save_team_participation(self):
         """The script can load and save participation info."""