From a546fb6fe06e4877840b4222084fd54210860e42 Mon Sep 17 00:00:00 2001 From: Tessa Walsh Date: Wed, 10 Jul 2024 22:24:50 -0400 Subject: [PATCH] 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 --- backend/btrixcloud/orgs.py | 31 +++++++++--- backend/btrixcloud/utils.py | 14 ++++++ backend/test/conftest.py | 3 +- backend/test/test_org.py | 54 ++++++++++++++++++++- backend/test/test_org_subs.py | 2 +- frontend/src/pages/invite/ui/org-form.ts | 20 ++++++-- frontend/src/pages/org/settings/settings.ts | 23 ++++++--- 7 files changed, 127 insertions(+), 20 deletions(-) diff --git a/backend/btrixcloud/orgs.py b/backend/btrixcloud/orgs.py index 65a63d76..0908e005 100644 --- a/backend/btrixcloud/orgs.py +++ b/backend/btrixcloud/orgs.py @@ -67,7 +67,12 @@ from .models import ( 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 @@ class OrgOps: ) 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 @@ class OrgOps: 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 @@ def init_orgs_api( 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} diff --git a/backend/btrixcloud/utils.py b/backend/btrixcloud/utils.py index 5c3c55a0..bf207c22 100644 --- a/backend/btrixcloud/utils.py +++ b/backend/btrixcloud/utils.py @@ -16,6 +16,7 @@ from uuid import UUID 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 diff --git a/backend/test/conftest.py b/backend/test/conftest.py index 3e82af9b..2dd2745b 100644 --- a/backend/test/conftest.py +++ b/backend/test/conftest.py @@ -28,6 +28,7 @@ _all_crawls_config_id = None _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 diff --git a/backend/test/test_org.py b/backend/test/test_org.py index 58f73136..9ecf78c3 100644 --- a/backend/test/test_org.py +++ b/backend/test/test_org.py @@ -4,7 +4,7 @@ import uuid 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 diff --git a/backend/test/test_org_subs.py b/backend/test/test_org_subs.py index 2d5bf807..48804b7a 100644 --- a/backend/test/test_org_subs.py +++ b/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): diff --git a/frontend/src/pages/invite/ui/org-form.ts b/frontend/src/pages/invite/ui/org-form.ts index cb6eb7f8..c08ad2ae 100644 --- a/frontend/src/pages/invite/ui/org-form.ts +++ b/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({ diff --git a/frontend/src/pages/org/settings/settings.ts b/frontend/src/pages/org/settings/settings.ts index e032e4e4..02432c6b 100644 --- a/frontend/src/pages/org/settings/settings.ts +++ b/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", });