Improve handling of duplicate org name/slug (#1917)

Initial implementation of #1892 

- Modifies the backend to return `duplicate_org_name` or
`duplicate_org_slug` as appropriate on a pymongo `DuplicateKeyError`
- Updates frontend to handle `duplicate_org_name`, `duplicate_org_slug`,
and `invalid_slug` error details
- Update errors to be more consistent, also return `duplicate_org_subscription.subId` for duplicate subscription instead of the more generic `already_exists`
---------
Co-authored-by: Ilya Kreymer <ikreymer@gmail.com>

Author: tw4l

backend/btrixcloud/orgs.py

@@ -67,7 +67,12 @@
     ACTIVE,
 )
 from .pagination import DEFAULT_PAGE_SIZE, paginated_format
-from .utils import slug_from_name, validate_slug, JSONSerializer
+from .utils import (
+    slug_from_name,
+    validate_slug,
+    get_duplicate_key_error_field,
+    JSONSerializer,
+)
 
 if TYPE_CHECKING:
     from .invites import InviteOps
@@ -301,8 +306,15 @@ async def create_default_org(self) -> None:
         )
         try:
             await self.orgs.insert_one(org.to_dict())
-        except DuplicateKeyError:
-            print(f"Organization name {org.name} already in use - skipping", flush=True)
+        except DuplicateKeyError as err:
+            field = get_duplicate_key_error_field(err)
+            value = org.name
+            if field == "slug":
+                value = org.slug
+            print(
+                f"Organization {field} {value} already in use - skipping",
+                flush=True,
+            )
 
     async def create_org(
         self,
@@ -337,7 +349,10 @@ async def create_org(
         try:
             await self.orgs.insert_one(org.to_dict())
         except DuplicateKeyError as dupe:
-            raise HTTPException(status_code=400, detail="already_exists") from dupe
+            field = get_duplicate_key_error_field(dupe)
+            raise HTTPException(
+                status_code=400, detail=f"duplicate_org_{field}"
+            ) from dupe
 
         return org
 
@@ -1390,9 +1405,11 @@ async def rename_org(
 
         try:
             await ops.update_slug_and_name(org)
-        except DuplicateKeyError:
-            # pylint: disable=raise-missing-from
-            raise HTTPException(status_code=400, detail="duplicate_org_name")
+        except DuplicateKeyError as dupe:
+            field = get_duplicate_key_error_field(dupe)
+            raise HTTPException(
+                status_code=400, detail=f"duplicate_org_{field}"
+            ) from dupe
 
         return {"updated": True}
 

backend/btrixcloud/utils.py

@@ -16,6 +16,7 @@
 
 from fastapi import HTTPException
 from fastapi.responses import StreamingResponse
+from pymongo.errors import DuplicateKeyError
 from slugify import slugify
 
 
@@ -166,3 +167,16 @@ def stream_dict_list_as_csv(data: List[Dict[str, Union[str, int]]], filename: st
         media_type="text/csv",
         headers={"Content-Disposition": f"attachment;filename={filename}"},
     )
+
+
+def get_duplicate_key_error_field(err: DuplicateKeyError) -> str:
+    """Get name of duplicate field from pymongo DuplicateKeyError"""
+    dupe_field = "name"
+    if err.details:
+        key_value = err.details.get("keyValue")
+        if key_value:
+            try:
+                dupe_field = list(key_value.keys())[0]
+            except IndexError:
+                pass
+    return dupe_field

backend/test/conftest.py

@@ -28,6 +28,7 @@
 _all_crawls_delete_config_id = None
 
 NON_DEFAULT_ORG_NAME = "Non-default org"
+NON_DEFAULT_ORG_SLUG = "non-default-org"
 
 FAILED_STATES = ["canceled", "failed", "skipped_quota_reached"]
 
@@ -77,7 +78,7 @@ def non_default_org_id(admin_auth_headers):
     r = requests.post(
         f"{API_PREFIX}/orgs/create",
         headers=admin_auth_headers,
-        json={"name": NON_DEFAULT_ORG_NAME, "slug": "non-default-org"},
+        json={"name": NON_DEFAULT_ORG_NAME, "slug": NON_DEFAULT_ORG_SLUG},
     )
     assert r.status_code == 200
 

backend/test/test_org.py

@@ -4,7 +4,7 @@
 
 import pytest
 
-from .conftest import API_PREFIX
+from .conftest import API_PREFIX, NON_DEFAULT_ORG_NAME, NON_DEFAULT_ORG_SLUG
 from .utils import read_in_chunks
 
 curr_dir = os.path.dirname(os.path.realpath(__file__))
@@ -92,6 +92,34 @@ def test_rename_org_invalid_slug(admin_auth_headers, default_org_id):
     assert r.json()["detail"] == "invalid_slug"
 
 
+def test_rename_org_duplicate_name(
+    admin_auth_headers, default_org_id, non_default_org_id
+):
+    rename_data = {"name": NON_DEFAULT_ORG_NAME, "slug": "this-slug-should-be-okay"}
+    r = requests.post(
+        f"{API_PREFIX}/orgs/{default_org_id}/rename",
+        headers=admin_auth_headers,
+        json=rename_data,
+    )
+
+    assert r.status_code == 400
+    assert r.json()["detail"] == "duplicate_org_name"
+
+
+def test_rename_org_duplicate_name(
+    admin_auth_headers, default_org_id, non_default_org_id
+):
+    rename_data = {"name": "Should be okay", "slug": NON_DEFAULT_ORG_SLUG}
+    r = requests.post(
+        f"{API_PREFIX}/orgs/{default_org_id}/rename",
+        headers=admin_auth_headers,
+        json=rename_data,
+    )
+
+    assert r.status_code == 400
+    assert r.json()["detail"] == "duplicate_org_slug"
+
+
 def test_create_org(admin_auth_headers):
     NEW_ORG_NAME = "New Org"
     r = requests.post(
@@ -118,6 +146,30 @@ def test_create_org(admin_auth_headers):
     assert NEW_ORG_NAME in org_names
 
 
+def test_create_org_duplicate_name(admin_auth_headers, non_default_org_id):
+    r = requests.post(
+        f"{API_PREFIX}/orgs/create",
+        headers=admin_auth_headers,
+        json={"name": NON_DEFAULT_ORG_NAME, "slug": "another-new-org"},
+    )
+
+    assert r.status_code == 400
+    data = r.json()
+    assert data["detail"] == "duplicate_org_name"
+
+
+def test_create_org_duplicate_slug(admin_auth_headers, non_default_org_id):
+    r = requests.post(
+        f"{API_PREFIX}/orgs/create",
+        headers=admin_auth_headers,
+        json={"name": "Yet another new org", "slug": NON_DEFAULT_ORG_SLUG},
+    )
+
+    assert r.status_code == 400
+    data = r.json()
+    assert data["detail"] == "duplicate_org_slug"
+
+
 # disable until storage customization is enabled
 def _test_change_org_storage(admin_auth_headers):
     # change to invalid storage

backend/test/test_org_subs.py

@@ -144,7 +144,7 @@ def test_create_sub_org_and_invite_existing_user_dupe_sub(admin_auth_headers):
     )
 
     assert r.status_code == 400
-    assert r.json()["detail"] == "already_exists"
+    assert r.json()["detail"] == "duplicate_org_subscription.subId"
 
 
 def test_create_sub_org_and_invite_existing_user(admin_auth_headers):

frontend/src/pages/invite/ui/org-form.ts

@@ -147,10 +147,22 @@ export class OrgForm extends TailwindElement {
       await this.onRenameSuccess(payload);
     } catch (e) {
       console.debug(e);
-      if (isApiError(e) && e.details === "duplicate_org_name") {
-        throw new Error(
-          msg("This org name or URL is already taken, try another one."),
-        );
+      if (isApiError(e)) {
+        if (e.details === "duplicate_org_name") {
+          throw new Error(
+            msg("This org name is already taken, try another one."),
+          );
+        } else if (e.details === "duplicate_org_slug") {
+          throw new Error(
+            msg("This org URL is already taken, try another one."),
+          );
+        } else if (e.details === "invalid_slug") {
+          throw new Error(
+            msg(
+              "This org URL is invalid. Please use alphanumeric characters and dashes (-) only.",
+            ),
+          );
+        }
       }
 
       this._notify.toast({

frontend/src/pages/org/settings/settings.ts

@@ -626,13 +626,24 @@ export class OrgSettings extends TailwindElement {
     } catch (e) {
       console.debug(e);
 
+      let message = msg(
+        "Sorry, couldn't rename organization at this time. Try again later from org settings.",
+      );
+
+      if (isApiError(e)) {
+        if (e.details === "duplicate_org_name") {
+          message = msg("This org name is already taken, try another one.");
+        } else if (e.details === "duplicate_org_slug") {
+          message = msg("This org URL is already taken, try another one.");
+        } else if (e.details === "invalid_slug") {
+          message = msg(
+            "This org URL is invalid. Please use alphanumeric characters and dashes (-) only.",
+          );
+        }
+      }
+
       this.notify.toast({
-        message:
-          isApiError(e) && e.details === "duplicate_org_name"
-            ? msg("This org name or URL is already taken, try another one.")
-            : msg(
-                "Sorry, couldn't rename organization at this time. Try again later from org settings.",
-              ),
+        message: message,
         variant: "danger",
         icon: "exclamation-octagon",
       });