[FIX] runbot_merge: make provisioning more resilient

A few cases of conflict were missing from the provisioning
handler.

They can't really be auto-fixed, so just output a warning and ignore
the entry, that way the rest of the provisioning succeeds.

runbot_merge/controllers/reviewer_provisioning.py

@@ -35,6 +35,11 @@ class MergebotReviewerProvisioning(Controller):
         Partners = env['res.partner']
         Users = env['res.users']
 
+        existing_logins = set()
+        existing_oauth = set()
+        for u in Users.with_context(active_test=False).search([]):
+            existing_logins.add(u.login)
+            existing_oauth .add((u.oauth_provider_id.id, u.oauth_uid))
         existing_partners = Partners.with_context(active_test=False).search([
             '|', ('email', 'in', [u['email'] for u in users]),
                  ('github_login', 'in', [u['github_login'] for u in users])
@@ -76,9 +81,30 @@ class MergebotReviewerProvisioning(Controller):
                 to_activate |= current
             # entry doesn't have user -> create user
             if not current.user_ids:
-                # skip users without an email (= login) as that
-                # fails
                 if not new['email']:
+                    _logger.info(
+                        "Unable to create user for %s: no email in provisioning data",
+                        current.display_name
+                    )
+                    continue
+                if 'oauth_uid' in new:
+                    if (new['oauth_provider_id'], new['oauth_uid']) in existing_oauth:
+                        _logger.warning(
+                            "Attempted to create user with duplicate oauth uid "
+                            "%s with provider %r for provisioning entry %r. "
+                            "There is likely a duplicate partner (one version "
+                            "with email, one with github login)",
+                            new['oauth_uid'], odoo_provider.display_name, new,
+                        )
+                        continue
+                if new['email'] in existing_logins:
+                    _logger.warning(
+                        "Attempted to create user with duplicate login %s for "
+                        "provisioning entry %r. There is likely a duplicate "
+                        "partner (one version with email, one with github "
+                        "login)",
+                        new['email'], new,
+                    )
                     continue
 
                 new['login'] = new['email']
@@ -93,7 +119,7 @@ class MergebotReviewerProvisioning(Controller):
             # otherwise update user (if there is anything to update)
             user = current.user_ids
             if len(user) != 1:
-                _logger.warning("Got %d users for partner %s.", len(user), current.display_name)
+                _logger.warning("Got %d users for partner %s, updating first.", len(user), current.display_name)
                 user = user[:1]
             new.setdefault("active", True)
             update_vals = {

runbot_merge/tests/test_provisioning.py

@@ -1,4 +1,3 @@
-import pytest
 import requests
 
 GEORGE = {
@@ -30,24 +29,13 @@ def test_basic_provisioning(env, port):
     r = provision_user(port, [dict(GEORGE, name="x", github_login="y", sub="42")])
     assert r == [0, 1]
 
-    # can't fail anymore because github_login now used to look up the existing
-    # user
-    # with pytest.raises(Exception):
-    #     provision_user(port, [{
-    #         'name': "other@example.org",
-    #         'email': "x",
-    #         'github_login': "y",
-    #         'sub': "42"
-    #     }])
-
     r = provision_user(port, [dict(GEORGE, active=False)])
     assert r == [0, 1]
     assert not env['res.users'].search([('login', '=', GEORGE['email'])])
     assert env['res.partner'].search([('email', '=', GEORGE['email'])])
 
 def test_upgrade_partner(env, port):
-    # If a partner exists for a github login (and / or email?) it can be
+    # matching partner with an email but no github login
-    # upgraded by creating a user for it
     p = env['res.partner'].create({
         'name': GEORGE['name'],
         'email': GEORGE['email'],
@@ -64,6 +52,7 @@ def test_upgrade_partner(env, port):
     p.user_ids.unlink()
     p.unlink()
 
+    # matching partner with a github login but no email
     p = env['res.partner'].create({
         'name': GEORGE['name'],
         'github_login': GEORGE['github_login'],
@@ -77,14 +66,14 @@ def test_upgrade_partner(env, port):
         'email': GEORGE['email'],
     }]
 
-    # deactivate user to see if provisioning re-enables them
+    # matching partner with a deactivated user
     p.user_ids.active = False
     r = provision_user(port, [GEORGE])
     assert r == [0, 1]
     assert len(p.user_ids) == 1, "provisioning should re-enable user"
     assert p.user_ids.active
 
-    # re-enable both user and partner to see if both get re-enabled
+    # matching deactivated partner (with a deactivated user)
     p.user_ids.active = False
     p.active = False
     r = provision_user(port, [GEORGE])
@@ -92,6 +81,33 @@ def test_upgrade_partner(env, port):
     assert p.active, "provisioning should re-enable partner"
     assert p.user_ids.active
 
+def test_duplicates(env, port):
+    """In case of duplicate data, the handler should probably not blow up, but
+    instead log a warning (so the data gets fixed eventually) and skip
+    """
+    # dupe 1: old oauth signup account & github interaction account, provisioning
+    # prioritises the github account & tries to create a user for it, which
+    # fails because the signup account has the same oauth uid (probably)
+    env['res.partner'].create({'name': 'foo', 'github_login': 'foo'})
+    env['res.users'].create({'login': 'foo@example.com', 'name': 'foo', 'email': 'foo@example.com', 'oauth_provider_id': 1, 'oauth_uid': '42'})
+    assert provision_user(port, [{
+        'name': "foo",
+        'email': 'foo@example.com',
+        'github_login': 'foo',
+        'sub': '42'
+    }]) == [0, 0]
+
+    # dupe 2: old non-oauth signup account & github interaction account, same
+    # as previous except it breaks on the login instead of the oauth_uid
+    env['res.partner'].create({'name': 'bar', 'github_login': 'bar'})
+    env['res.users'].create({'login': 'bar@example.com', 'name': 'bar', 'email': 'bar@example.com'})
+    assert provision_user(port, [{
+        'name': "bar",
+        'email': 'bar@example.com',
+        'github_login': 'bar',
+        'sub': '43'
+    }]) == [0, 0]
+
 def test_no_email(env, port):
     """ Provisioning system should ignore email-less entries
     """
@@ -182,6 +198,6 @@ def provision_user(port, users):
     })
     r.raise_for_status()
     json = r.json()
-    assert 'error' not in json
+    assert 'error' not in json, json['error']['data']['debug']
 
     return json['result']