launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:oci-registry-credentials-unique-allow-region into launchpad:master.

Commit message:
Include region for uniqueness of OCI registry credentials

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/394880

Also make a few bits of the corresponding browser code more consistent in passing.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:oci-registry-credentials-unique-allow-region into launchpad:master.

diff --git a/lib/lp/oci/browser/ocirecipe.py b/lib/lp/oci/browser/ocirecipe.py
index c5385ed..f18779d 100644
--- a/lib/lp/oci/browser/ocirecipe.py
+++ b/lib/lp/oci/browser/ocirecipe.py
@@ -586,8 +586,8 @@ class OCIRecipeEditPushRulesView(LaunchpadFormView):
             except OCIRegistryCredentialsAlreadyExist:
                 self.setFieldError(
                     "add_url",
-                    "Credentials already exist with the same URL and "
-                    "username.")
+                    "Credentials already exist with the same URL, username, "
+                    "and region.")
                 return
             except ValidationError:
                 self.setFieldError("add_url", "Not a valid URL.")
diff --git a/lib/lp/oci/interfaces/ociregistrycredentials.py b/lib/lp/oci/interfaces/ociregistrycredentials.py
index 7d3085b..b73575f 100644
--- a/lib/lp/oci/interfaces/ociregistrycredentials.py
+++ b/lib/lp/oci/interfaces/ociregistrycredentials.py
@@ -42,7 +42,8 @@ class OCIRegistryCredentialsAlreadyExist(Exception):
 
     def __init__(self):
         super(OCIRegistryCredentialsAlreadyExist, self).__init__(
-            "Credentials already exist with the same URL and username.")
+            "Credentials already exist with the same URL, username, and "
+            "region.")
 
 
 @error_status(http_client.UNAUTHORIZED)
diff --git a/lib/lp/oci/model/ociregistrycredentials.py b/lib/lp/oci/model/ociregistrycredentials.py
index bb7c33d..6bf5301 100644
--- a/lib/lp/oci/model/ociregistrycredentials.py
+++ b/lib/lp/oci/model/ociregistrycredentials.py
@@ -169,7 +169,8 @@ class OCIRegistryCredentialsSet:
         for existing in self.findByOwner(owner):
             url_match = existing.url == url
             username_match = existing.username == credentials.get('username')
-            if (url_match and username_match):
+            region_match = existing.region == credentials.get('region')
+            if url_match and username_match and region_match:
                 return existing
         return None
 
diff --git a/lib/lp/oci/tests/test_ociregistrycredentials.py b/lib/lp/oci/tests/test_ociregistrycredentials.py
index 971f3cc..d29470b 100644
--- a/lib/lp/oci/tests/test_ociregistrycredentials.py
+++ b/lib/lp/oci/tests/test_ociregistrycredentials.py
@@ -206,6 +206,29 @@ class TestOCIRegistryCredentialsSet(OCIConfigHelperMixin, TestCaseWithFactory):
 
         self.assertEqual(new.id, existing.id)
 
+    def test_getOrCreate_existing_by_region(self):
+        owner = self.factory.makePerson()
+        url = self.factory.getUniqueURL()
+        west_credentials = {
+            'username': 'foo', 'password': 'bar', 'region': 'west',
+            }
+        west = getUtility(IOCIRegistryCredentialsSet).new(
+            registrant=owner, owner=owner, url=url,
+            credentials=west_credentials)
+        east_credentials = {
+            'username': 'foo', 'password': 'bar', 'region': 'east',
+            }
+        east = getUtility(IOCIRegistryCredentialsSet).new(
+            registrant=owner, owner=owner, url=url,
+            credentials=east_credentials)
+        self.assertNotEqual(west.id, east.id)
+
+        existing_west = getUtility(IOCIRegistryCredentialsSet).getOrCreate(
+            registrant=owner, owner=owner, url=url,
+            credentials=west_credentials)
+
+        self.assertEqual(west.id, existing_west.id)
+
     def test_getOrCreate_new(self):
         owner = self.factory.makePerson()
         url = self.factory.getUniqueURL()
diff --git a/lib/lp/registry/browser/person.py b/lib/lp/registry/browser/person.py
index af8f2e3..7e50377 100644
--- a/lib/lp/registry/browser/person.py
+++ b/lib/lp/registry/browser/person.py
@@ -3909,14 +3909,16 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
                         "confirm_password", credentials.id),
                     "Passwords do not match.")
             else:
-                credentials.setCredentials(
-                    {"username": username,
-                     "password": password})
-                credentials.url = parsed_credentials["url"]
+                raw_credentials = {
+                    "username": username,
+                    "password": password,
+                    }
+                if region:
+                    raw_credentials["region"] = region
+                credentials.setCredentials(raw_credentials)
         elif username != credentials.username:
             removeSecurityProxy(credentials).username = username
-            credentials.url = parsed_credentials["url"]
-        elif parsed_credentials["url"] != credentials.url:
+        if parsed_credentials["url"] != credentials.url:
             credentials.url = parsed_credentials["url"]
         if credentials.region != region:
             removeSecurityProxy(credentials).region = region
@@ -3969,6 +3971,8 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
                         "username.")
             else:
                 credentials = {'username': username}
+                if region:
+                    credentials["region"] = region
                 try:
                     getUtility(IOCIRegistryCredentialsSet).new(
                         registrant=self.user, owner=owner, url=url,