From 9f99d4a4e16abc9d8327c72f13a0f6f934a07bbb Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 20 Oct 2025 10:07:58 +0200 Subject: [PATCH 01/23] rename as profile --- .../users/_controller/rest/{users_rest.py => profile_rest.py} | 0 .../web/server/src/simcore_service_webserver/users/plugin.py | 4 ++-- .../server/tests/unit/with_dbs/03/test_users_rest_phone.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) rename services/web/server/src/simcore_service_webserver/users/_controller/rest/{users_rest.py => profile_rest.py} (100%) diff --git a/services/web/server/src/simcore_service_webserver/users/_controller/rest/users_rest.py b/services/web/server/src/simcore_service_webserver/users/_controller/rest/profile_rest.py similarity index 100% rename from services/web/server/src/simcore_service_webserver/users/_controller/rest/users_rest.py rename to services/web/server/src/simcore_service_webserver/users/_controller/rest/profile_rest.py diff --git a/services/web/server/src/simcore_service_webserver/users/plugin.py b/services/web/server/src/simcore_service_webserver/users/plugin.py index f85d0a7e88cb..6289cf2a9e55 100644 --- a/services/web/server/src/simcore_service_webserver/users/plugin.py +++ b/services/web/server/src/simcore_service_webserver/users/plugin.py @@ -13,7 +13,7 @@ ) from ..user_preferences.bootstrap import setup_user_preferences_feature from ..user_tokens.bootstrap import setup_user_tokens_feature -from ._controller.rest import accounts_rest, users_rest +from ._controller.rest import accounts_rest, profile_rest _logger = logging.getLogger(__name__) @@ -31,7 +31,7 @@ def setup_users(app: web.Application): assert app[APP_SETTINGS_APPKEY].WEBSERVER_USERS # nosec setup_observer_registry(app) - app.router.add_routes(users_rest.routes) + app.router.add_routes(profile_rest.routes) app.router.add_routes(accounts_rest.routes) setup_user_notification_feature(app) diff --git a/services/web/server/tests/unit/with_dbs/03/test_users_rest_phone.py b/services/web/server/tests/unit/with_dbs/03/test_users_rest_phone.py index c93af68cd925..3f92d86fec4a 100644 --- a/services/web/server/tests/unit/with_dbs/03/test_users_rest_phone.py +++ b/services/web/server/tests/unit/with_dbs/03/test_users_rest_phone.py @@ -19,7 +19,7 @@ from pytest_simcore.helpers.webserver_users import UserInfoDict from servicelib.aiohttp import status from simcore_service_webserver.models import PhoneNumberStr -from simcore_service_webserver.users._controller.rest.users_rest import ( +from simcore_service_webserver.users._controller.rest.profile_rest import ( _REGISTRATION_CODE_VALUE_FAKE, ) From 3736675c4199efb84aebd70dcfc506c4b553db5c Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 20 Oct 2025 11:46:17 +0200 Subject: [PATCH 02/23] @matusdrobuliak66 case-insensitive email search --- .../src/models_library/api_schemas_webserver/users.py | 4 ++-- packages/models-library/src/models_library/string_types.py | 2 ++ .../server/tests/unit/with_dbs/03/test_users_rest_search.py | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/models-library/src/models_library/api_schemas_webserver/users.py b/packages/models-library/src/models_library/api_schemas_webserver/users.py index 7412e2b8df71..c554c1bdd9cb 100644 --- a/packages/models-library/src/models_library/api_schemas_webserver/users.py +++ b/packages/models-library/src/models_library/api_schemas_webserver/users.py @@ -338,7 +338,7 @@ class UserAccountSearchQueryParams(RequestParameters): email: Annotated[ GlobPatternSafeStr | None, Field( - description="complete or glob pattern for an email", + description="complete or glob pattern for an email (case insensitive)", ), ] = None primary_group_id: Annotated[ @@ -350,7 +350,7 @@ class UserAccountSearchQueryParams(RequestParameters): user_name: Annotated[ GlobPatternSafeStr | None, Field( - description="complete or glob pattern for a username", + description="complete or glob pattern for a username (case insensitive)", ), ] = None diff --git a/packages/models-library/src/models_library/string_types.py b/packages/models-library/src/models_library/string_types.py index cc7a2b4dcc5d..05d4c4f75c6d 100644 --- a/packages/models-library/src/models_library/string_types.py +++ b/packages/models-library/src/models_library/string_types.py @@ -203,6 +203,7 @@ def validate_input_xss_safety(value: str) -> str: max_length=200, strip_whitespace=True, pattern=r"^[A-Za-z0-9 ._\*@-]*$", # Allow alphanumeric, spaces, dots, underscores, hyphens, asterisks and at signs + to_lower=True # make case-insensitive ), AfterValidator(validate_input_xss_safety), ] @@ -215,6 +216,7 @@ def validate_input_xss_safety(value: str) -> str: min_length=1, max_length=200, pattern=r"^[A-Za-z0-9 ._@-]*$", # Allow alphanumeric, spaces, dots, underscores, hyphens, and at signs + to_lower=True # make case-insensitive ), AfterValidator(validate_input_xss_safety), annotated_types.doc( diff --git a/services/web/server/tests/unit/with_dbs/03/test_users_rest_search.py b/services/web/server/tests/unit/with_dbs/03/test_users_rest_search.py index 7473033cbd1a..79c5ba21eadf 100644 --- a/services/web/server/tests/unit/with_dbs/03/test_users_rest_search.py +++ b/services/web/server/tests/unit/with_dbs/03/test_users_rest_search.py @@ -238,7 +238,7 @@ async def test_search_users_by_partial_email( url = ( client.app.router["search_user_accounts"] .url_for() - .with_query(email=partial_email) + .with_query(email=partial_email.upper()) # NOTE: case insensitive! ) resp = await client.get(f"{url}") await assert_status(resp, status.HTTP_403_FORBIDDEN) From 7839c403b9e5c306948fd5b593841a799ff1baac Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 20 Oct 2025 13:26:00 +0200 Subject: [PATCH 03/23] Enh InvitationContent and tests consecutive codes --- .../src/models_library/invitations.py | 76 ++++++++++++------- services/invitations/tests/unit/conftest.py | 6 +- .../tests/unit/test_invitations.py | 41 ++++++++++ 3 files changed, 92 insertions(+), 31 deletions(-) diff --git a/packages/models-library/src/models_library/invitations.py b/packages/models-library/src/models_library/invitations.py index adb1545f0b6d..cadc4667b5ec 100644 --- a/packages/models-library/src/models_library/invitations.py +++ b/packages/models-library/src/models_library/invitations.py @@ -1,7 +1,14 @@ -from datetime import datetime, timezone -from typing import Final +from datetime import UTC, datetime +from typing import Annotated, Final -from pydantic import BaseModel, EmailStr, Field, PositiveInt, field_validator +from pydantic import ( + AfterValidator, + BaseModel, + EmailStr, + Field, + PositiveInt, + field_validator, +) from .products import ProductName @@ -11,29 +18,40 @@ class InvitationInputs(BaseModel): """Input data necessary to create an invitation""" - issuer: str = Field( - ..., - description="Identifies who issued the invitation. E.g. an email, a service name etc. NOTE: it will be trimmed if exceeds maximum", - min_length=1, - max_length=_MAX_LEN, - ) - guest: EmailStr = Field( - ..., - description="Invitee's email. Note that the registration can ONLY be used with this email", - ) - trial_account_days: PositiveInt | None = Field( - default=None, - description="If set, this invitation will activate a trial account." - "Sets the number of days from creation until the account expires", - ) - extra_credits_in_usd: PositiveInt | None = Field( - default=None, - description="If set, the account's primary wallet will add extra credits corresponding to this ammount in USD", - ) - product: ProductName | None = Field( - default=None, - description="If None, it will use INVITATIONS_DEFAULT_PRODUCT", - ) + issuer: Annotated[ + str, + Field( + description="Identifies who issued the invitation. E.g. an email, a service name etc. NOTE: it will be trimmed if exceeds maximum", + min_length=1, + max_length=_MAX_LEN, + ), + ] + guest: Annotated[ + EmailStr, + AfterValidator(lambda v: v.lower()), + Field( + description="Invitee's email. Note that the registration can ONLY be used with this email", + ), + ] + trial_account_days: Annotated[ + PositiveInt | None, + Field( + description="If set, this invitation will activate a trial account." + "Sets the number of days from creation until the account expires", + ), + ] = None + extra_credits_in_usd: Annotated[ + PositiveInt | None, + Field( + description="If set, the account's primary wallet will add extra credits corresponding to this ammount in USD", + ), + ] = None + product: Annotated[ + ProductName | None, + Field( + description="If None, it will use INVITATIONS_DEFAULT_PRODUCT", + ), + ] = None @field_validator("issuer", mode="before") @classmethod @@ -44,10 +62,10 @@ def trim_long_issuers_to_max_length(cls, v): class InvitationContent(InvitationInputs): - """Data in an invitation""" + """Data within an invitation""" # avoid using default to mark exactly the time - created: datetime = Field(..., description="Timestamp for creation") + created: Annotated[datetime, Field(description="Timestamp for creation")] def as_invitation_inputs(self) -> InvitationInputs: return self.model_validate( @@ -62,6 +80,6 @@ def create_from_inputs( kwargs = invitation_inputs.model_dump(exclude_none=True) kwargs.setdefault("product", default_product) return cls( - created=datetime.now(tz=timezone.utc), + created=datetime.now(tz=UTC), **kwargs, ) diff --git a/services/invitations/tests/unit/conftest.py b/services/invitations/tests/unit/conftest.py index 414062b9f944..bf240b17ed9f 100644 --- a/services/invitations/tests/unit/conftest.py +++ b/services/invitations/tests/unit/conftest.py @@ -114,7 +114,7 @@ def invitation_data( # first version kwargs = { "issuer": "LicenseRequestID=123456789", - "guest": faker.email(), + "guest": faker.email().upper(), # to test case insensitivity "trial_account_days": faker.pyint(min_value=1) if is_trial_account else None, } # next version, can include product @@ -124,4 +124,6 @@ def invitation_data( if extra_credits_in_usd is not None: kwargs["extra_credits_in_usd"] = extra_credits_in_usd - return InvitationInputs.model_validate(kwargs) + data = InvitationInputs.model_validate(kwargs) + assert data.guest == kwargs["guest"].lower() # emails are case insensitive + return data diff --git a/services/invitations/tests/unit/test_invitations.py b/services/invitations/tests/unit/test_invitations.py index 770dba67bb99..f18e6e455e1e 100644 --- a/services/invitations/tests/unit/test_invitations.py +++ b/services/invitations/tests/unit/test_invitations.py @@ -203,3 +203,44 @@ def test_aliases_uniqueness(): ).items() if count > 1 ] # nosec + + +def test_consecutive_invitation_codes_are_different_due_to_timestamps( + invitation_data: InvitationInputs, + secret_key: str, + default_product: ProductName, +): + """Test that two consecutive invitation codes with same data are different because of different timestamps.""" + secret = secret_key.encode() + + # Create first invitation code + content1 = InvitationContent.create_from_inputs(invitation_data, default_product) + code1 = _create_invitation_code(content1, secret) + + # Create second invitation code with same data + content2 = InvitationContent.create_from_inputs(invitation_data, default_product) + code2 = _create_invitation_code(content2, secret) + + # Codes should be different + assert code1 != code2 + + # Decrypt both codes + invitation1 = decrypt_invitation( + invitation_code=code1.decode(), + secret_key=secret, + default_product=default_product, + ) + invitation2 = decrypt_invitation( + invitation_code=code2.decode(), + secret_key=secret, + default_product=default_product, + ) + + # Content should be the same except for timestamps + assert invitation1.model_dump(exclude={"created"}) == invitation2.model_dump( + exclude={"created"} + ) + + # Timestamps should be different + assert invitation1.created != invitation2.created + assert invitation2.created >= invitation1.created From 9cae515710ba46fc4c1018d440d8cb49900ade15 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 20 Oct 2025 16:32:49 +0200 Subject: [PATCH 04/23] new OrderingQueryParams --- .../src/models_library/rest_ordering.py | 115 ++++++++++++++---- .../tests/test_rest_ordering.py | 48 ++++++++ 2 files changed, 142 insertions(+), 21 deletions(-) diff --git a/packages/models-library/src/models_library/rest_ordering.py b/packages/models-library/src/models_library/rest_ordering.py index 1fdd9d6cfed4..59488ae298f3 100644 --- a/packages/models-library/src/models_library/rest_ordering.py +++ b/packages/models-library/src/models_library/rest_ordering.py @@ -1,8 +1,16 @@ from enum import Enum -from typing import Annotated +from typing import Annotated, Generic, TypeVar +from common_library.basic_types import DEFAULT_FACTORY from common_library.json_serialization import json_dumps -from pydantic import BaseModel, BeforeValidator, ConfigDict, Field, field_validator +from pydantic import ( + BaseModel, + BeforeValidator, + ConfigDict, + Field, + field_validator, +) +from pydantic.generics import GenericModel from .basic_types import IDStr from .rest_base import RequestParameters @@ -15,15 +23,16 @@ class OrderDirection(str, Enum): class OrderBy(BaseModel): - # Based on https://google.aip.dev/132#ordering - field: IDStr = Field(..., description="field name identifier") - direction: OrderDirection = Field( - default=OrderDirection.ASC, - description=( - f"As [A,B,C,...] if `{OrderDirection.ASC.value}`" - f" or [Z,Y,X, ...] if `{OrderDirection.DESC.value}`" + field: Annotated[IDStr, Field(description="field name identifier")] + direction: Annotated[ + OrderDirection, + Field( + description=( + f"As [A,B,C,...] if `{OrderDirection.ASC.value}`" + f" or [Z,Y,X, ...] if `{OrderDirection.DESC.value}`" + ) ), - ) + ] = OrderDirection.ASC class _BaseOrderQueryParams(RequestParameters): @@ -91,12 +100,10 @@ def _check_ordering_field_and_map(cls, v): return _ordering_fields_api_to_column_map.get(v) or v assert "json_schema_extra" in _OrderBy.model_config # nosec - assert isinstance(_OrderBy.model_config["json_schema_extra"], dict) # nosec - assert isinstance( # nosec - _OrderBy.model_config["json_schema_extra"]["examples"], list - ) - order_by_example = _OrderBy.model_config["json_schema_extra"]["examples"][0] + + order_by_example = _OrderBy.model_json_schema()["examples"][0] order_by_example_json = json_dumps(order_by_example) + assert _OrderBy.model_validate(order_by_example), "Example is invalid" # nosec converted_default = _OrderBy.model_validate( @@ -104,17 +111,83 @@ def _check_ordering_field_and_map(cls, v): default.model_dump() ) - class _OrderQueryParams(_BaseOrderQueryParams): - order_by: Annotated[_OrderBy, BeforeValidator(parse_json_pre_validator)] = ( + class _OrderJsonQueryParams(_BaseOrderQueryParams): + order_by: Annotated[ + _OrderBy, + BeforeValidator(parse_json_pre_validator), Field( - default=converted_default, description=( f"Order by field (`{msg_field_options}`) and direction (`{msg_direction_options}`). " f"The default sorting order is `{json_dumps(default)}`." ), examples=[order_by_example], json_schema_extra={"example_json": order_by_example_json}, - ) - ) + ), + ] = converted_default + + return _OrderJsonQueryParams + - return _OrderQueryParams +# +# +# NOTE: OrderingQueryParams is a more flexible variant for generic usage and that +# does include multiple ordering clauses +# + +TField = TypeVar("TField", bound=str) + + +class OrderClause(GenericModel, Generic[TField]): + field: TField + direction: OrderDirection = OrderDirection.ASC + + +class OrderingQueryParams(GenericModel, Generic[TField]): + order_by: Annotated[ + list[OrderClause[TField]], + Field( + default_factory=list, + description="Order by clauses e.g. ?order_by=-created_at,name", + ), + ] = DEFAULT_FACTORY + + @field_validator("order_by", mode="before") + @classmethod + def _parse_order_by(cls, v): + """ + Example: + + GET /items?order_by=-created_at,name + + Parses to: + [ + OrderClause(field="created_at", direction=OrderDirection.DESC), + OrderClause(field="name", direction=OrderDirection.ASC), + ] + """ + if not v: + return [] + if isinstance(v, str): + v = v.split(",") + clauses = [] + for t in v: + token = t.strip() + if not token: + continue + if token.startswith("-"): + clauses.append( + OrderClause[TField]( + field=token[1:].strip(), direction=OrderDirection.DESC + ) + ) + elif token.startswith("+"): + clauses.append( + OrderClause[TField]( + field=token[1:].strip(), direction=OrderDirection.ASC + ) + ) + else: + clauses.append( + OrderClause[TField](field=token, direction=OrderDirection.ASC) + ) + return clauses diff --git a/packages/models-library/tests/test_rest_ordering.py b/packages/models-library/tests/test_rest_ordering.py index 33e1ad654d10..80cfba632f41 100644 --- a/packages/models-library/tests/test_rest_ordering.py +++ b/packages/models-library/tests/test_rest_ordering.py @@ -1,11 +1,14 @@ import pickle +from typing import Literal import pytest from common_library.json_serialization import json_dumps from models_library.basic_types import IDStr from models_library.rest_ordering import ( OrderBy, + OrderClause, OrderDirection, + OrderingQueryParams, create_ordering_query_model_class, ) from pydantic import ( @@ -236,3 +239,48 @@ def test_ordering_query_parse_json_pre_validator(): assert error["loc"] == ("order_by",) assert error["type"] == "value_error" assert error["input"] == bad_json_value + + +def test_ordering_query_params_parsing(): + """Test OrderingQueryParams parsing from URL query format like ?order_by=-created_at,name,+gender""" + + # Define allowed fields using Literal type + ValidField = Literal["created_at", "name", "gender"] + + class TestOrderingParams(OrderingQueryParams[ValidField]): + pass + + # Test parsing from comma-separated string + params = TestOrderingParams.model_validate({"order_by": "-created_at,name,+gender"}) + + assert params.order_by == [ + OrderClause[ValidField](field="created_at", direction=OrderDirection.DESC), + OrderClause[ValidField](field="name", direction=OrderDirection.ASC), + OrderClause[ValidField](field="gender", direction=OrderDirection.ASC), + ] + + +def test_ordering_query_params_validation_error_with_invalid_fields(): + """Test that OrderingQueryParams raises ValidationError when invalid fields are used""" + + # Define allowed fields using Literal type + ValidField = Literal["created_at", "name"] + + class TestOrderingParams(OrderingQueryParams[ValidField]): + pass + + # Test with invalid field should raise ValidationError + with pytest.raises(ValidationError) as err_info: + TestOrderingParams.model_validate( + {"order_by": "-created_at,invalid_field,name"} + ) + + # Verify the validation error details + exc = err_info.value + assert exc.error_count() > 0 + + # Check that the error mentions the invalid field + error_messages = [error["msg"] for error in exc.errors()] + assert any( + "Input should be 'created_at' or 'name'" in msg for msg in error_messages + ) From 58284eaa0efe163757fbbc6b3ed6074130eb0556 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 20 Oct 2025 17:39:14 +0200 Subject: [PATCH 05/23] list operations sorting --- .../src/models_library/list_operations.py | 72 +++++++++++++++++++ .../tests/test_list_operations.py | 67 +++++++++++++++++ 2 files changed, 139 insertions(+) create mode 100644 packages/models-library/src/models_library/list_operations.py create mode 100644 packages/models-library/tests/test_list_operations.py diff --git a/packages/models-library/src/models_library/list_operations.py b/packages/models-library/src/models_library/list_operations.py new file mode 100644 index 000000000000..85a1be42773c --- /dev/null +++ b/packages/models-library/src/models_library/list_operations.py @@ -0,0 +1,72 @@ +"""List operation models and helpers + +- Ordering: https://google.aip.dev/132#ordering + + +""" + +from enum import Enum +from typing import TYPE_CHECKING, Generic, TypeVar + +from pydantic.generics import GenericModel + + +class OrderDirection(str, Enum): + ASC = "asc" + DESC = "desc" + + +if TYPE_CHECKING: + from typing import Protocol + + class LiteralField(Protocol): + """Protocol for Literal string types""" + + def __str__(self) -> str: ... + + TField = TypeVar("TField", bound=LiteralField) +else: + TField = TypeVar("TField", bound=str) + + +class OrderClause(GenericModel, Generic[TField]): + field: TField + direction: OrderDirection = OrderDirection.ASC + + +def check_ordering_list( + order_by: list[tuple[TField, OrderDirection]], +) -> list[tuple[TField, OrderDirection]]: + """Validates ordering list and removes duplicate entries. + + Ensures that each field appears at most once. If a field is repeated: + - With the same direction: silently drops the duplicate + - With different directions: raises ValueError + + + Args: + order_by: List of (field, direction) tuples + + Returns: + List with duplicates removed, preserving order of first occurrence + + Raises: + ValueError: If a field appears with conflicting directions + """ + seen_fields = {} + unique_order_by = [] + + for field, direction in order_by: + if field in seen_fields: + # Field already seen - check if direction matches + if seen_fields[field] != direction: + msg = f"Field '{field}' appears with conflicting directions: {seen_fields[field].value} and {direction.value}" + raise ValueError(msg) + # Same field and direction - skip duplicate + continue + + # First time seeing this field + seen_fields[field] = direction + unique_order_by.append((field, direction)) + + return unique_order_by diff --git a/packages/models-library/tests/test_list_operations.py b/packages/models-library/tests/test_list_operations.py new file mode 100644 index 000000000000..274c43b2ddd1 --- /dev/null +++ b/packages/models-library/tests/test_list_operations.py @@ -0,0 +1,67 @@ +import pytest +from models_library.list_operations import OrderDirection, check_ordering_list + + +def test_check_ordering_list_drops_duplicates_silently(): + """Test that check_ordering_list silently drops duplicate entries with same field and direction""" + + # Input with duplicates (same field and direction) + order_by = [ + ("email", OrderDirection.ASC), + ("created", OrderDirection.DESC), + ("email", OrderDirection.ASC), # Duplicate - should be dropped + ("name", OrderDirection.ASC), + ("created", OrderDirection.DESC), # Duplicate - should be dropped + ] + + result = check_ordering_list(order_by) + + # Should return unique entries preserving order of first occurrence + expected = [ + ("email", OrderDirection.ASC), + ("created", OrderDirection.DESC), + ("name", OrderDirection.ASC), + ] + + assert result == expected + + +def test_check_ordering_list_raises_for_conflicting_directions(): + """Test that check_ordering_list raises ValueError when same field has different directions""" + + # Input with same field but different directions + order_by = [ + ("email", OrderDirection.ASC), + ("created", OrderDirection.DESC), + ("email", OrderDirection.DESC), # Conflict! Same field, different direction + ] + + with pytest.raises(ValueError, match="conflicting directions") as exc_info: + check_ordering_list(order_by) + + error_msg = str(exc_info.value) + assert "Field 'email' appears with conflicting directions" in error_msg + assert "asc" in error_msg + assert "desc" in error_msg + + +def test_check_ordering_list_empty_input(): + """Test that check_ordering_list handles empty input correctly""" + + result = check_ordering_list([]) + assert result == [] + + +def test_check_ordering_list_no_duplicates(): + """Test that check_ordering_list works correctly when there are no duplicates""" + + order_by = [ + ("email", OrderDirection.ASC), + ("created", OrderDirection.DESC), + ("name", OrderDirection.ASC), + ] + + result = check_ordering_list(order_by) + + # Should return the same list + assert result == order_by From 8183919eec9471ffa9cbe9a6c2974442a43219d8 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 20 Oct 2025 17:49:07 +0200 Subject: [PATCH 06/23] Refactor ordering query parsing and enhance validation error handling --- .../src/models_library/rest_ordering.py | 95 ++++++++----------- .../tests/test_rest_ordering.py | 11 +-- 2 files changed, 46 insertions(+), 60 deletions(-) diff --git a/packages/models-library/src/models_library/rest_ordering.py b/packages/models-library/src/models_library/rest_ordering.py index 59488ae298f3..c84e028d2ed8 100644 --- a/packages/models-library/src/models_library/rest_ordering.py +++ b/packages/models-library/src/models_library/rest_ordering.py @@ -1,5 +1,4 @@ -from enum import Enum -from typing import Annotated, Generic, TypeVar +from typing import Annotated, Generic from common_library.basic_types import DEFAULT_FACTORY from common_library.json_serialization import json_dumps @@ -13,16 +12,17 @@ from pydantic.generics import GenericModel from .basic_types import IDStr +from .list_operations import OrderClause, OrderDirection, TField, check_ordering_list from .rest_base import RequestParameters -from .utils.common_validators import parse_json_pre_validator - +from .utils.common_validators import ( + parse_json_pre_validator, +) -class OrderDirection(str, Enum): - ASC = "asc" - DESC = "desc" +__all__: tuple[str, ...] = ("OrderDirection",) class OrderBy(BaseModel): + # NOTE: use instead OrderClause[TField] where TField is Literal of valid fields field: Annotated[IDStr, Field(description="field name identifier")] direction: Annotated[ OrderDirection, @@ -128,21 +128,10 @@ class _OrderJsonQueryParams(_BaseOrderQueryParams): return _OrderJsonQueryParams -# -# -# NOTE: OrderingQueryParams is a more flexible variant for generic usage and that -# does include multiple ordering clauses -# - -TField = TypeVar("TField", bound=str) - - -class OrderClause(GenericModel, Generic[TField]): - field: TField - direction: OrderDirection = OrderDirection.ASC - - class OrderingQueryParams(GenericModel, Generic[TField]): + # NOTE: OrderingQueryParams is a more flexible variant for generic usage and that + # does include multiple ordering clauses + # order_by: Annotated[ list[OrderClause[TField]], Field( @@ -153,41 +142,39 @@ class OrderingQueryParams(GenericModel, Generic[TField]): @field_validator("order_by", mode="before") @classmethod - def _parse_order_by(cls, v): - """ - Example: - - GET /items?order_by=-created_at,name - - Parses to: - [ - OrderClause(field="created_at", direction=OrderDirection.DESC), - OrderClause(field="name", direction=OrderDirection.ASC), - ] + def _parse_order_by_string(cls, v): + """Parses a comma-separated string into a list of OrderClause + + Example, given the query parameter `order_by` in a request like `GET /items?order_by=-created_at,name` + It parses to: + [ + OrderClause(field="created_at", direction=OrderDirection.DESC), + OrderClause(field="name", direction=OrderDirection.ASC), + ] """ if not v: return [] + if isinstance(v, str): + # 1. from comma-separated string to list of OrderClause v = v.split(",") - clauses = [] - for t in v: - token = t.strip() - if not token: - continue - if token.startswith("-"): - clauses.append( - OrderClause[TField]( - field=token[1:].strip(), direction=OrderDirection.DESC - ) - ) - elif token.startswith("+"): - clauses.append( - OrderClause[TField]( - field=token[1:].strip(), direction=OrderDirection.ASC - ) - ) - else: - clauses.append( - OrderClause[TField](field=token, direction=OrderDirection.ASC) - ) - return clauses + clauses: list[tuple[str, OrderDirection]] = [] + for t in v: + token = t.strip() + if not token: + continue + if token.startswith("-"): + clauses.append((token[1:].strip(), OrderDirection.DESC)) + elif token.startswith("+"): + clauses.append((token[1:].strip(), OrderDirection.ASC)) + else: + clauses.append((token, OrderDirection.ASC)) + # 2. check for duplicates and conflicting directions + return [ + {"field": field, "direction": direction} + for field, direction in check_ordering_list(clauses) + ] + + # NOTE: Parses ONLY strings into list[OrderClause], otherwise raises TypeError + msg = f"Invalid type for order_by: expected str, got {type(v)}" + raise TypeError(msg) diff --git a/packages/models-library/tests/test_rest_ordering.py b/packages/models-library/tests/test_rest_ordering.py index 80cfba632f41..6feda4731df6 100644 --- a/packages/models-library/tests/test_rest_ordering.py +++ b/packages/models-library/tests/test_rest_ordering.py @@ -277,10 +277,9 @@ class TestOrderingParams(OrderingQueryParams[ValidField]): # Verify the validation error details exc = err_info.value - assert exc.error_count() > 0 + assert exc.error_count() == 1 - # Check that the error mentions the invalid field - error_messages = [error["msg"] for error in exc.errors()] - assert any( - "Input should be 'created_at' or 'name'" in msg for msg in error_messages - ) + error = exc.errors()[0] + assert error["loc"] == ("order_by", 1, "field") + assert error["type"] == "literal_error" + assert error["input"] == "invalid_field" From 3e86f7a5e16761f485f89fa45472ce50116a48cf Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 20 Oct 2025 18:06:16 +0200 Subject: [PATCH 07/23] Enhance user account query parameters with ordering support and add utility for retrieving literal values --- .../src/models_library/api_schemas_webserver/users.py | 10 +++++++++- .../src/models_library/list_operations.py | 9 ++++++++- packages/models-library/tests/test_rest_ordering.py | 3 +++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/packages/models-library/src/models_library/api_schemas_webserver/users.py b/packages/models-library/src/models_library/api_schemas_webserver/users.py index c554c1bdd9cb..b07f49fdf20a 100644 --- a/packages/models-library/src/models_library/api_schemas_webserver/users.py +++ b/packages/models-library/src/models_library/api_schemas_webserver/users.py @@ -32,6 +32,7 @@ SearchPatternSafeStr, validate_input_xss_safety, ) +from ..rest_ordering import OrderingQueryParams from ..users import ( FirstNameStr, LastNameStr, @@ -317,7 +318,14 @@ class UsersForAdminListFilter(Filters): model_config = ConfigDict(extra="forbid") -class UsersAccountListQueryParams(UsersForAdminListFilter, PageQueryParameters): ... +UserAccountOrderFields: TypeAlias = Literal["email", "created_at"] + + +class UsersAccountListQueryParams( + UsersForAdminListFilter, + PageQueryParameters, + OrderingQueryParams[UserAccountOrderFields], +): ... class _InvitationDetails(InputSchema): diff --git a/packages/models-library/src/models_library/list_operations.py b/packages/models-library/src/models_library/list_operations.py index 85a1be42773c..df801edc2649 100644 --- a/packages/models-library/src/models_library/list_operations.py +++ b/packages/models-library/src/models_library/list_operations.py @@ -6,7 +6,7 @@ """ from enum import Enum -from typing import TYPE_CHECKING, Generic, TypeVar +from typing import TYPE_CHECKING, Any, Generic, Literal, TypeVar, get_args, get_origin from pydantic.generics import GenericModel @@ -29,6 +29,13 @@ def __str__(self) -> str: ... TField = TypeVar("TField", bound=str) +def get_literal_values(tfield: Any) -> tuple[str, ...] | None: + """Return Literal values if TField is a Literal, else None.""" + if get_origin(tfield) is Literal: + return get_args(tfield) + return None + + class OrderClause(GenericModel, Generic[TField]): field: TField direction: OrderDirection = OrderDirection.ASC diff --git a/packages/models-library/tests/test_rest_ordering.py b/packages/models-library/tests/test_rest_ordering.py index 6feda4731df6..2b214d218be1 100644 --- a/packages/models-library/tests/test_rest_ordering.py +++ b/packages/models-library/tests/test_rest_ordering.py @@ -4,6 +4,7 @@ import pytest from common_library.json_serialization import json_dumps from models_library.basic_types import IDStr +from models_library.list_operations import get_literal_values from models_library.rest_ordering import ( OrderBy, OrderClause, @@ -259,6 +260,8 @@ class TestOrderingParams(OrderingQueryParams[ValidField]): OrderClause[ValidField](field="gender", direction=OrderDirection.ASC), ] + assert get_literal_values(ValidField) == ("created_at", "name", "gender") + def test_ordering_query_params_validation_error_with_invalid_fields(): """Test that OrderingQueryParams raises ValidationError when invalid fields are used""" From 17857ba1ff3860c51d8d5a1b34b6c36f7832fb48 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 20 Oct 2025 19:10:27 +0200 Subject: [PATCH 08/23] Add mapping and validation functions for order fields with literals --- .../src/models_library/list_operations.py | 52 +++++++++ .../tests/test_list_operations.py | 108 +++++++++++++++++- 2 files changed, 159 insertions(+), 1 deletion(-) diff --git a/packages/models-library/src/models_library/list_operations.py b/packages/models-library/src/models_library/list_operations.py index df801edc2649..7f86570ec812 100644 --- a/packages/models-library/src/models_library/list_operations.py +++ b/packages/models-library/src/models_library/list_operations.py @@ -77,3 +77,55 @@ def check_ordering_list( unique_order_by.append((field, direction)) return unique_order_by + + +def map_order_fields( + order_clauses: list[OrderClause[TField]], field_mapping: dict[str, str] +) -> list[tuple[str, OrderDirection]]: + """Map order clause fields using a field mapping dictionary. + + Args: + order_clauses: List of OrderClause objects with API field names + field_mapping: Dictionary mapping API field names to domain/DB field names + + Returns: + List of tuples with mapped field names and directions + + Example: + >>> clauses = [OrderClause(field="email", direction=OrderDirection.ASC)] + >>> mapping = {"email": "user_email", "created_at": "created"} + >>> map_order_fields(clauses, mapping) + [("user_email", OrderDirection.ASC)] + """ + return [ + (field_mapping[str(clause.field)], clause.direction) for clause in order_clauses + ] + + +def validate_order_fields_with_literals( + order_by: list[tuple[str, str]], + valid_fields: set[str], +) -> None: + """Validate order_by list with string field names and directions. + + Args: + order_by: List of (field_name, direction) tuples with string values + valid_fields: Set of allowed field names + valid_directions: Set of allowed direction values + + Raises: + ValueError: If any field or direction is invalid + """ + valid_directions = {OrderDirection.ASC.value, OrderDirection.DESC.value} + + invalid_fields = {field for field, _ in order_by if field not in valid_fields} + if invalid_fields: + msg = f"Invalid order_by field(s): {invalid_fields}. Valid fields are: {valid_fields}" + raise ValueError(msg) + + invalid_directions = { + direction for _, direction in order_by if direction not in valid_directions + } + if invalid_directions: + msg = f"Invalid order direction(s): {invalid_directions}. Must be one of: {valid_directions}" + raise ValueError(msg) diff --git a/packages/models-library/tests/test_list_operations.py b/packages/models-library/tests/test_list_operations.py index 274c43b2ddd1..624489312f89 100644 --- a/packages/models-library/tests/test_list_operations.py +++ b/packages/models-library/tests/test_list_operations.py @@ -1,5 +1,13 @@ +from typing import Literal + import pytest -from models_library.list_operations import OrderDirection, check_ordering_list +from models_library.list_operations import ( + OrderClause, + OrderDirection, + check_ordering_list, + map_order_fields, + validate_order_fields_with_literals, +) def test_check_ordering_list_drops_duplicates_silently(): @@ -65,3 +73,101 @@ def test_check_ordering_list_no_duplicates(): # Should return the same list assert result == order_by + + +def test_map_order_fields(): + """Test that map_order_fields correctly maps field names using provided mapping""" + + ValidField = Literal["email", "created_at", "name"] + + order_clauses = [ + OrderClause[ValidField](field="email", direction=OrderDirection.ASC), + OrderClause[ValidField](field="created_at", direction=OrderDirection.DESC), + OrderClause[ValidField](field="name", direction=OrderDirection.ASC), + ] + + field_mapping = { + "email": "user_email", + "created_at": "created_timestamp", + "name": "display_name", + } + + result = map_order_fields(order_clauses, field_mapping) + + expected = [ + ("user_email", OrderDirection.ASC), + ("created_timestamp", OrderDirection.DESC), + ("display_name", OrderDirection.ASC), + ] + + assert result == expected + + +def test_map_order_fields_with_unmapped_field(): + """Test that map_order_fields raises KeyError when field is not in mapping""" + + ValidField = Literal["email", "unknown"] + + order_clauses = [ + OrderClause[ValidField](field="email", direction=OrderDirection.ASC), + OrderClause[ValidField](field="unknown", direction=OrderDirection.DESC), + ] + + field_mapping = { + "email": "user_email", + # "unknown" is missing from mapping + } + + with pytest.raises(KeyError): + map_order_fields(order_clauses, field_mapping) + + +def test_validate_order_fields_with_literals_valid(): + """Test that validate_order_fields_with_literals passes with valid fields and directions""" + + order_by = [ + ("email", "asc"), + ("created", "desc"), + ("name", "asc"), + ] + + valid_fields = {"email", "created", "name"} + + # Should not raise any exception + validate_order_fields_with_literals(order_by, valid_fields) + + +def test_validate_order_fields_with_literals_invalid_field(): + """Test that validate_order_fields_with_literals raises ValueError for invalid fields""" + + order_by = [ + ("email", "asc"), + ("invalid_field", "desc"), + ] + + valid_fields = {"email", "created"} + + with pytest.raises(ValueError, match="Invalid order_by field") as exc_info: + validate_order_fields_with_literals(order_by, valid_fields) + + error_msg = str(exc_info.value) + assert "invalid_field" in error_msg + assert "Valid fields are" in error_msg + + +def test_validate_order_fields_with_literals_invalid_direction(): + """Test that validate_order_fields_with_literals raises ValueError for invalid directions""" + + order_by = [ + ("email", "ascending"), # Invalid direction + ("created", "desc"), + ] + + valid_fields = {"email", "created"} + + with pytest.raises(ValueError, match="Invalid order direction") as exc_info: + validate_order_fields_with_literals(order_by, valid_fields) + + error_msg = str(exc_info.value) + assert "ascending" in error_msg + assert "Must be one of" in error_msg From 9329cfb6cbea831817e446a89fc2e54013122869 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 20 Oct 2025 21:54:19 +0200 Subject: [PATCH 09/23] minor --- api/specs/web-server/_common.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/specs/web-server/_common.py b/api/specs/web-server/_common.py index 4ca4ef69e16b..bc74e765f6a5 100644 --- a/api/specs/web-server/_common.py +++ b/api/specs/web-server/_common.py @@ -60,7 +60,7 @@ def as_query(model_class: type[BaseModel]) -> type[BaseModel]: for field_name, field_info in model_class.model_fields.items(): field_default = field_info.default - assert not field_info.default_factory # nosec + assert not field_info.default_factory, f"got {field_info=}" # nosec query_kwargs = { "alias": field_info.alias, "title": field_info.title, From 02b6e2002de091695de374cd38b020c16b7cc1c0 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Tue, 21 Oct 2025 17:09:02 +0200 Subject: [PATCH 10/23] fixing oas --- api/specs/web-server/_users_admin.py | 21 +++- .../src/models_library/rest_ordering.py | 103 ++++++++++-------- .../api/v0/openapi.yaml | 15 +++ 3 files changed, 90 insertions(+), 49 deletions(-) diff --git a/api/specs/web-server/_users_admin.py b/api/specs/web-server/_users_admin.py index 2e4ff647c3cc..fba3a8df3d68 100644 --- a/api/specs/web-server/_users_admin.py +++ b/api/specs/web-server/_users_admin.py @@ -7,14 +7,14 @@ from enum import Enum from typing import Annotated -from _common import as_query -from fastapi import APIRouter, Depends, status +from fastapi import APIRouter, Depends, Query, status from models_library.api_schemas_webserver.users import ( + PageQueryParameters, UserAccountApprove, UserAccountGet, UserAccountReject, UserAccountSearchQueryParams, - UsersAccountListQueryParams, + UsersForAdminListFilter, ) from models_library.generics import Envelope from models_library.rest_pagination import Page @@ -26,13 +26,26 @@ _extra_tags: list[str | Enum] = ["admin"] +# NOTE: I still do not have a clean solution for this +# +class _Q(UsersForAdminListFilter, PageQueryParameters): ... + + @router.get( "/admin/user-accounts", response_model=Page[UserAccountGet], tags=_extra_tags, ) async def list_users_accounts( - _query: Annotated[as_query(UsersAccountListQueryParams), Depends()], + _query: Annotated[_Q, Depends()], + order_by: Annotated[ + str, + Query( + title="Order By", + description="Comma-separated list of fields for ordering (prefix with '-' for descending).", + example="-created_at,name", + ), + ] = "", ): ... diff --git a/packages/models-library/src/models_library/rest_ordering.py b/packages/models-library/src/models_library/rest_ordering.py index c84e028d2ed8..753bea42cb1f 100644 --- a/packages/models-library/src/models_library/rest_ordering.py +++ b/packages/models-library/src/models_library/rest_ordering.py @@ -36,6 +36,7 @@ class OrderBy(BaseModel): class _BaseOrderQueryParams(RequestParameters): + # Use OrderingQueryParams instead for more flexible ordering order_by: OrderBy @@ -128,53 +129,65 @@ class _OrderJsonQueryParams(_BaseOrderQueryParams): return _OrderJsonQueryParams +def _parse_order_by(v): + if not v: + return [] + + if isinstance(v, list): + v = ",".join(v) + + if not isinstance(v, str): + msg = "order_by must be a string" + raise TypeError(msg) + + # 1. from comma-separated string to list of OrderClause + clauses = [] + for t in v.split(","): + token = t.strip() + if not token: + continue + if token.startswith("-"): + clauses.append((token[1:], OrderDirection.DESC)) + elif token.startswith("+"): + clauses.append((token[1:], OrderDirection.ASC)) + else: + clauses.append((token, OrderDirection.ASC)) + + # 2. check for duplicates and conflicting directions + return [ + {"field": field, "direction": direction} + for field, direction in check_ordering_list(clauses) + ] + + class OrderingQueryParams(GenericModel, Generic[TField]): - # NOTE: OrderingQueryParams is a more flexible variant for generic usage and that - # does include multiple ordering clauses - # + """ + This class is designed to parse query parameters for ordering results in an API request. + + It supports multiple ordering clauses and allows for flexible sorting options. + + NOTE: It only parses strings and validates into list[OrderClause[TField]] + where TField is a type variable representing valid field names. + + + For example: + + /my/path?order_by=field1,-field2,+field3 + + would sort by field1 ascending, field2 descending, and field3 ascending. + """ + order_by: Annotated[ list[OrderClause[TField]], - Field( - default_factory=list, - description="Order by clauses e.g. ?order_by=-created_at,name", - ), + BeforeValidator(_parse_order_by), + Field(default_factory=list), ] = DEFAULT_FACTORY - @field_validator("order_by", mode="before") - @classmethod - def _parse_order_by_string(cls, v): - """Parses a comma-separated string into a list of OrderClause - - Example, given the query parameter `order_by` in a request like `GET /items?order_by=-created_at,name` - It parses to: - [ - OrderClause(field="created_at", direction=OrderDirection.DESC), - OrderClause(field="name", direction=OrderDirection.ASC), - ] - """ - if not v: - return [] - - if isinstance(v, str): - # 1. from comma-separated string to list of OrderClause - v = v.split(",") - clauses: list[tuple[str, OrderDirection]] = [] - for t in v: - token = t.strip() - if not token: - continue - if token.startswith("-"): - clauses.append((token[1:].strip(), OrderDirection.DESC)) - elif token.startswith("+"): - clauses.append((token[1:].strip(), OrderDirection.ASC)) - else: - clauses.append((token, OrderDirection.ASC)) - # 2. check for duplicates and conflicting directions - return [ - {"field": field, "direction": direction} - for field, direction in check_ordering_list(clauses) - ] - - # NOTE: Parses ONLY strings into list[OrderClause], otherwise raises TypeError - msg = f"Invalid type for order_by: expected str, got {type(v)}" - raise TypeError(msg) + model_config = ConfigDict( + json_schema_extra={ + "examples": [ + {"order_by": "-created_at,name,+gender"}, + {"order_by": ""}, + ], + } + ) diff --git a/services/web/server/src/simcore_service_webserver/api/v0/openapi.yaml b/services/web/server/src/simcore_service_webserver/api/v0/openapi.yaml index bb8fa617e76d..337b824296fb 100644 --- a/services/web/server/src/simcore_service_webserver/api/v0/openapi.yaml +++ b/services/web/server/src/simcore_service_webserver/api/v0/openapi.yaml @@ -1724,11 +1724,25 @@ paths: summary: List Users Accounts operationId: list_users_accounts parameters: + - name: order_by + in: query + required: false + schema: + type: string + title: Order By + description: Comma-separated list of fields for ordering (prefix with '-' + for descending). + default: '' + description: Comma-separated list of fields for ordering (prefix with '-' + for descending). + example: -created_at,name - name: limit in: query required: false schema: type: integer + maximum: 50 + minimum: 1 default: 20 title: Limit - name: offset @@ -1736,6 +1750,7 @@ paths: required: false schema: type: integer + minimum: 0 default: 0 title: Offset - name: review_status From 9802715b77350505c5b30040fc34f6cb61462c8a Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Tue, 21 Oct 2025 17:55:02 +0200 Subject: [PATCH 11/23] Add ordering support to list_merged_pre_and_registered_users and related tests --- .../users/_accounts_repository.py | 91 ++++++++-- .../users/test_users_accounts_repository.py | 163 ++++++++++++++++++ 2 files changed, 237 insertions(+), 17 deletions(-) diff --git a/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py b/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py index 5dfcb27f7833..2c843c9c44fa 100644 --- a/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py +++ b/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py @@ -1,5 +1,5 @@ import logging -from typing import Any, cast +from typing import Any, Literal, TypeAlias, cast import sqlalchemy as sa from common_library.exclude import Unset, is_unset @@ -8,6 +8,7 @@ from models_library.users import ( UserID, ) +from pydantic import validate_call from simcore_postgres_database.models.groups import groups, user_to_groups from simcore_postgres_database.models.products import products from simcore_postgres_database.models.users import UserStatus, users @@ -426,6 +427,11 @@ async def search_merged_pre_and_registered_users( return result.fetchall() +OrderKeys: TypeAlias = Literal["email", "current_status_created"] +OrderDirs: TypeAlias = Literal["asc", "desc"] + + +@validate_call(config={"arbitrary_types_allowed": True}) async def list_merged_pre_and_registered_users( engine: AsyncEngine, connection: AsyncConnection | None = None, @@ -435,6 +441,7 @@ async def list_merged_pre_and_registered_users( filter_include_deleted: bool = False, pagination_limit: int = 50, pagination_offset: int = 0, + order_by: list[tuple[OrderKeys, OrderDirs]] | None = None, ) -> tuple[list[dict[str, Any]], int]: """Retrieves and merges users from both users and pre-registration tables. @@ -452,17 +459,21 @@ async def list_merged_pre_and_registered_users( filter_include_deleted: Whether to include deleted users pagination_limit: Maximum number of results to return pagination_offset: Number of results to skip (for pagination) + order_by: List of (field, direction) tuples. Valid fields: "email", "current_status_created" + Default: [("email", "asc"), ("is_pre_registered", "desc"), ("current_status_created", "desc")] Returns: Tuple of (list of merged user data, total count) """ # Base where conditions for both queries - pre_reg_where = [users_pre_registration_details.c.product_name == product_name] - users_where = [] + pre_reg_query_conditions = [ + users_pre_registration_details.c.product_name == product_name + ] + user_conditions = [] # Add account request status filter if specified if filter_any_account_request_status: - pre_reg_where.append( + pre_reg_query_conditions.append( users_pre_registration_details.c.account_request_status.in_( filter_any_account_request_status ) @@ -470,7 +481,7 @@ async def list_merged_pre_and_registered_users( # Add filter for deleted users if not filter_include_deleted: - users_where.append(users.c.status != UserStatus.DELETED) + user_conditions.append(users.c.status != UserStatus.DELETED) # Create subquery for reviewer username account_request_reviewed_by_username = ( @@ -479,7 +490,7 @@ async def list_merged_pre_and_registered_users( # Query for pre-registered users # We need to left join with users to identify if the pre-registered user is already in the system - pre_reg_query = ( + pre_registered_users_query = ( sa.select( users_pre_registration_details.c.id, users_pre_registration_details.c.pre_email.label("email"), @@ -495,6 +506,12 @@ async def list_merged_pre_and_registered_users( users_pre_registration_details.c.user_id.label("pre_reg_user_id"), users_pre_registration_details.c.extras, users_pre_registration_details.c.created, + # Computed current_status_created column + sa.func.coalesce( + users.c.created_at, # If user exists, use users.created_at + users_pre_registration_details.c.account_request_reviewed_at, # Else if reviewed, use review date + users_pre_registration_details.c.created, # Else use pre-registration created date + ).label("current_status_created"), users_pre_registration_details.c.account_request_status, users_pre_registration_details.c.account_request_reviewed_by, users_pre_registration_details.c.account_request_reviewed_at, @@ -512,11 +529,11 @@ async def list_merged_pre_and_registered_users( users, users_pre_registration_details.c.user_id == users.c.id ) ) - .where(sa.and_(*pre_reg_where)) + .where(sa.and_(*pre_reg_query_conditions)) ) # Query for users that are associated with the product through groups - users_query = ( + registered_users_query = ( sa.select( sa.literal(None).label("id"), users.c.email, @@ -532,6 +549,8 @@ async def list_merged_pre_and_registered_users( sa.literal(None).label("pre_reg_user_id"), sa.literal(None).label("extras"), users.c.created_at.label("created"), + # For regular users, current_status_created is just their created_at + users.c.created_at.label("current_status_created"), sa.literal(None).label("account_request_status"), sa.literal(None).label("account_request_reviewed_by"), sa.literal(None).label("account_request_reviewed_at"), @@ -549,29 +568,28 @@ async def list_merged_pre_and_registered_users( .join(groups, groups.c.gid == user_to_groups.c.gid) .join(products, products.c.group_id == groups.c.gid) ) - .where(sa.and_(products.c.name == product_name, *users_where)) + .where(sa.and_(products.c.name == product_name, *user_conditions)) ) # If filtering by account request status, we only want pre-registered users with any of those statuses # No need to union with regular users as they don't have account_request_status merged_query: sa.sql.Select | sa.sql.CompoundSelect if filter_any_account_request_status: - merged_query = pre_reg_query + merged_query = pre_registered_users_query else: - merged_query = pre_reg_query.union_all(users_query) + merged_query = pre_registered_users_query.union_all(registered_users_query) # Add distinct on email to eliminate duplicates merged_query_subq = merged_query.subquery() + + # Build ordering clauses using the extracted function + order_by_clauses = _build_ordering_clauses(merged_query_subq, order_by) + distinct_query = ( sa.select(merged_query_subq) .select_from(merged_query_subq) .distinct(merged_query_subq.c.email) - .order_by( - merged_query_subq.c.email, - # Prioritize pre-registration records if duplicate emails exist - merged_query_subq.c.is_pre_registered.desc(), - merged_query_subq.c.created.desc(), - ) + .order_by(*order_by_clauses) .limit(pagination_limit) .offset(pagination_offset) ) @@ -594,3 +612,42 @@ async def list_merged_pre_and_registered_users( records = result.mappings().all() return cast(list[dict[str, Any]], records), total_count + + +def _build_ordering_clauses( + merged_query_subq: sa.sql.Subquery, + order_by: list[tuple[OrderKeys, OrderDirs]] | None = None, +) -> list[sa.sql.ColumnElement]: + """Build ORDER BY clauses for merged user queries. + + Args: + merged_query_subq: The merged query subquery containing all columns + order_by: List of (field, direction) tuples for ordering + + Returns: + List of SQLAlchemy ordering clauses + """ + _ordering_criteria: list[tuple[str, OrderDirs]] = [] + + if order_by is None: + # Default ordering + _ordering_criteria = [ + ("email", "asc"), + ("is_pre_registered", "desc"), + ("current_status_created", "desc"), + ] + else: + _ordering_criteria = list(order_by) + # Always append is_pre_registered prioritization for custom ordering + if not any(field == "is_pre_registered" for field, _ in order_by): + _ordering_criteria.append(("is_pre_registered", "desc")) + + order_by_clauses = [] + for field, direction in _ordering_criteria: + column = merged_query_subq.columns[field] + if direction == "asc": + order_by_clauses.append(column.asc()) + else: + order_by_clauses.append(column.desc()) + + return order_by_clauses diff --git a/services/web/server/tests/unit/with_dbs/03/users/test_users_accounts_repository.py b/services/web/server/tests/unit/with_dbs/03/users/test_users_accounts_repository.py index 71c178405675..65213ca99412 100644 --- a/services/web/server/tests/unit/with_dbs/03/users/test_users_accounts_repository.py +++ b/services/web/server/tests/unit/with_dbs/03/users/test_users_accounts_repository.py @@ -959,3 +959,166 @@ async def test_search_merged_users_by_primary_group_id( assert found_user is not None assert found_user.first_name == product_owner_user["first_name"] assert found_user.last_name == product_owner_user["last_name"] + + +async def test_list_merged_users_default_ordering( + app: web.Application, + product_name: ProductName, + mixed_user_data: MixedUserTestData, +): + """Test default ordering behavior (email asc, is_pre_registered desc, current_status_created desc).""" + asyncpg_engine = get_asyncpg_engine(app) + + # Act - Get all users with default ordering (order_by=None) + users_list, total_count = ( + await _accounts_repository.list_merged_pre_and_registered_users( + asyncpg_engine, + product_name=product_name, + filter_include_deleted=False, + order_by=None, # Explicit None to test default + ) + ) + + # Assert + assert total_count >= 3, "Should have at least 3 users" + assert len(users_list) >= 3, "Should return at least 3 users" + + # Extract emails in order they were returned + returned_emails = [user["email"] for user in users_list] + + # Verify emails are sorted in ascending order (default behavior) + expected_test_emails = sorted( + [ + mixed_user_data.pre_reg_email, + mixed_user_data.product_owner_email, + mixed_user_data.approved_email, + ] + ) + + # Find positions of our test emails in the result + test_email_positions = [] + for email in expected_test_emails: + try: + pos = returned_emails.index(email) + test_email_positions.append(pos) + except ValueError: + pytest.fail(f"Expected email {email} not found in results") + + # Verify our test emails appear in ascending alphabetical order + assert test_email_positions == sorted(test_email_positions), ( + f"Test emails should be in ascending order. " + f"Expected: {expected_test_emails}, Got: {[returned_emails[pos] for pos in test_email_positions]}" + ) + + +async def test_list_merged_users_custom_ordering( + app: web.Application, + product_name: ProductName, + mixed_user_data: MixedUserTestData, +): + """Test custom ordering behavior (email desc) vs default (email asc).""" + asyncpg_engine = get_asyncpg_engine(app) + + # Act 1 - Get users with default ordering + default_users, count1 = ( + await _accounts_repository.list_merged_pre_and_registered_users( + asyncpg_engine, + product_name=product_name, + filter_include_deleted=False, + order_by=None, + ) + ) + + # Act 2 - Get users with custom ordering (email desc) + custom_users, count2 = ( + await _accounts_repository.list_merged_pre_and_registered_users( + asyncpg_engine, + product_name=product_name, + filter_include_deleted=False, + order_by=[("email", "desc")], + ) + ) + + # Assert + assert count1 == count2, "Total counts should match" + assert len(default_users) == len(custom_users), "Should return same number of users" + + # Extract test emails from both results + default_test_emails = [ + user["email"] + for user in default_users + if user["email"] + in { + mixed_user_data.pre_reg_email, + mixed_user_data.product_owner_email, + mixed_user_data.approved_email, + } + ] + + custom_test_emails = [ + user["email"] + for user in custom_users + if user["email"] + in { + mixed_user_data.pre_reg_email, + mixed_user_data.product_owner_email, + mixed_user_data.approved_email, + } + ] + + # Verify we have the same emails in both results + assert set(default_test_emails) == set( + custom_test_emails + ), "Should have same emails in both results" + + # Verify the ordering is different (ascending vs descending) + assert default_test_emails == sorted( + default_test_emails + ), "Default should be ascending order" + assert custom_test_emails == sorted( + custom_test_emails, reverse=True + ), "Custom should be descending order" + + # Verify they are actually different (unless all emails are the same, which they're not) + if ( + len(set(default_test_emails)) > 1 + ): # Only test if we have multiple distinct emails + assert ( + default_test_emails != custom_test_emails + ), "Default and custom ordering should produce different results" + + +async def test_list_merged_users_custom_ordering_with_current_status_created( + app: web.Application, + product_name: ProductName, + mixed_user_data: MixedUserTestData, +): + """Test ordering by current_status_created field.""" + asyncpg_engine = get_asyncpg_engine(app) + + # Act - Get users ordered by current_status_created descending + users_list, _ = await _accounts_repository.list_merged_pre_and_registered_users( + asyncpg_engine, + product_name=product_name, + filter_include_deleted=False, + order_by=[("current_status_created", "desc")], + ) + + # Assert + assert len(users_list) >= 3, "Should have at least 3 users" + + # Verify all users have current_status_created field + for user in users_list: + assert ( + "current_status_created" in user + ), "All users should have current_status_created field" + assert ( + user["current_status_created"] is not None + ), "current_status_created should not be None" + + # Verify that current_status_created values are in descending order (newest first) + current_status_dates = [user["current_status_created"] for user in users_list] + sorted_dates = sorted(current_status_dates, reverse=True) + assert ( + current_status_dates == sorted_dates + ), "Users should be ordered by current_status_created desc" From 1dad1b3ac4a7ddc70cbbf27ef7f761f58fc03d35 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Tue, 21 Oct 2025 18:13:57 +0200 Subject: [PATCH 12/23] Enhance list_merged_pre_and_registered_users with detailed parameter documentation and validation for order_by field --- .../users/_accounts_repository.py | 83 +++++++++++-------- .../users/test_users_accounts_repository.py | 42 ++++++++++ 2 files changed, 89 insertions(+), 36 deletions(-) diff --git a/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py b/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py index 2c843c9c44fa..796512fe3dd3 100644 --- a/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py +++ b/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py @@ -1,7 +1,8 @@ import logging -from typing import Any, Literal, TypeAlias, cast +from typing import Annotated, Any, Literal, TypeAlias, cast import sqlalchemy as sa +from annotated_types import doc from common_library.exclude import Unset, is_unset from common_library.users_enums import AccountRequestStatus from models_library.products import ProductName @@ -437,11 +438,21 @@ async def list_merged_pre_and_registered_users( connection: AsyncConnection | None = None, *, product_name: ProductName, - filter_any_account_request_status: list[AccountRequestStatus] | None = None, + filter_any_account_request_status: Annotated[ + list[AccountRequestStatus] | None, + doc( + "If provided, only returns users with account request status in this list (only pre-registered users with any of these statuses will be included)" + ), + ] = None, filter_include_deleted: bool = False, pagination_limit: int = 50, pagination_offset: int = 0, - order_by: list[tuple[OrderKeys, OrderDirs]] | None = None, + order_by: Annotated[ + list[tuple[OrderKeys, OrderDirs]] | None, + doc( + 'Valid fields: "email", "current_status_created". Default: [("email", "asc"), ("is_pre_registered", "desc"), ("current_status_created", "desc")]' + ), + ] = None, ) -> tuple[list[dict[str, Any]], int]: """Retrieves and merges users from both users and pre-registration tables. @@ -450,18 +461,6 @@ async def list_merged_pre_and_registered_users( 2. Users who are pre-registered (in users_pre_registration_details table) 3. Users who are both registered and pre-registered - Args: - engine: Database engine - connection: Optional existing connection - product_name: Product name to filter by - filter_any_account_request_status: If provided, only returns users with account request status in this list - (only pre-registered users with any of these statuses will be included) - filter_include_deleted: Whether to include deleted users - pagination_limit: Maximum number of results to return - pagination_offset: Number of results to skip (for pagination) - order_by: List of (field, direction) tuples. Valid fields: "email", "current_status_created" - Default: [("email", "asc"), ("is_pre_registered", "desc"), ("current_status_created", "desc")] - Returns: Tuple of (list of merged user data, total count) """ @@ -579,22 +578,41 @@ async def list_merged_pre_and_registered_users( else: merged_query = pre_registered_users_query.union_all(registered_users_query) - # Add distinct on email to eliminate duplicates + # Add distinct on email to eliminate duplicates using ROW_NUMBER() merged_query_subq = merged_query.subquery() + # Use ROW_NUMBER() to prioritize records per email + # This allows us to order by any field without DISTINCT ON constraints + ranked_query = sa.select( + merged_query_subq, + sa.func.row_number() + .over( + partition_by=merged_query_subq.c.email, + order_by=[ + merged_query_subq.c.is_pre_registered.desc(), # Prioritize pre-registered + merged_query_subq.c.current_status_created.desc(), # Then by most recent + ], + ) + .label("rn"), + ).subquery() + + # Filter to get only the first record per email (rn = 1) + filtered_query = sa.select( + *[col for col in ranked_query.c if col.name != "rn"] + ).where(ranked_query.c.rn == 1) + # Build ordering clauses using the extracted function - order_by_clauses = _build_ordering_clauses(merged_query_subq, order_by) + order_by_clauses = _build_ordering_clauses_for_filtered_query( + filtered_query, order_by + ) - distinct_query = ( - sa.select(merged_query_subq) - .select_from(merged_query_subq) - .distinct(merged_query_subq.c.email) - .order_by(*order_by_clauses) + final_query = ( + filtered_query.order_by(*order_by_clauses) .limit(pagination_limit) .offset(pagination_offset) ) - # Count query (for pagination) + # Count query (for pagination) - count distinct emails count_query = sa.select(sa.func.count().label("total")).select_from( sa.select(merged_query_subq.c.email) .select_from(merged_query_subq) @@ -608,25 +626,17 @@ async def list_merged_pre_and_registered_users( total_count = count_result.scalar_one() # Get user records - result = await conn.execute(distinct_query) + result = await conn.execute(final_query) records = result.mappings().all() return cast(list[dict[str, Any]], records), total_count -def _build_ordering_clauses( - merged_query_subq: sa.sql.Subquery, +def _build_ordering_clauses_for_filtered_query( + query: sa.sql.Select, order_by: list[tuple[OrderKeys, OrderDirs]] | None = None, ) -> list[sa.sql.ColumnElement]: - """Build ORDER BY clauses for merged user queries. - - Args: - merged_query_subq: The merged query subquery containing all columns - order_by: List of (field, direction) tuples for ordering - - Returns: - List of SQLAlchemy ordering clauses - """ + """Build ORDER BY clauses for filtered query (no DISTINCT ON constraints).""" _ordering_criteria: list[tuple[str, OrderDirs]] = [] if order_by is None: @@ -644,7 +654,8 @@ def _build_ordering_clauses( order_by_clauses = [] for field, direction in _ordering_criteria: - column = merged_query_subq.columns[field] + # Get column from the query's selected columns + column = next(col for col in query.selected_columns if col.name == field) if direction == "asc": order_by_clauses.append(column.asc()) else: diff --git a/services/web/server/tests/unit/with_dbs/03/users/test_users_accounts_repository.py b/services/web/server/tests/unit/with_dbs/03/users/test_users_accounts_repository.py index 65213ca99412..d2e4eef463bb 100644 --- a/services/web/server/tests/unit/with_dbs/03/users/test_users_accounts_repository.py +++ b/services/web/server/tests/unit/with_dbs/03/users/test_users_accounts_repository.py @@ -13,6 +13,7 @@ from common_library.users_enums import AccountRequestStatus from models_library.products import ProductName from models_library.users import UserID +from pydantic import ValidationError from simcore_postgres_database.models.users_details import ( users_pre_registration_details, ) @@ -1122,3 +1123,44 @@ async def test_list_merged_users_custom_ordering_with_current_status_created( assert ( current_status_dates == sorted_dates ), "Users should be ordered by current_status_created desc" + + +@pytest.mark.parametrize( + "invalid_order_by,expected_error_pattern", + [ + # Invalid field name + ( + [("invalid_field", "asc")], + r"Input should be 'email' or 'current_status_created'", + ), + # Invalid direction + ([("email", "invalid_direction")], r"Input should be 'asc' or 'desc'"), + # Multiple invalid values + ( + [("invalid_field", "invalid_direction")], + r"Input should be 'email' or 'current_status_created'", + ), + # Mixed valid and invalid + ( + [("email", "asc"), ("invalid_field", "desc")], + r"Input should be 'email' or 'current_status_created'", + ), + ], +) +async def test_list_merged_users_invalid_order_by_validation( + app: web.Application, + product_name: ProductName, + invalid_order_by: list[tuple[str, str]], + expected_error_pattern: str, +): + """Test that invalid order_by parameters are rejected by Pydantic validation.""" + + asyncpg_engine = get_asyncpg_engine(app) + + # Act & Assert - Should raise ValidationError + with pytest.raises(ValidationError, match=expected_error_pattern): + await _accounts_repository.list_merged_pre_and_registered_users( + asyncpg_engine, + product_name=product_name, + order_by=invalid_order_by, + ) From bd49d9e7c363696fdca9a492850926aa013242c8 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Tue, 21 Oct 2025 18:17:50 +0200 Subject: [PATCH 13/23] Add limit parameter to search_merged_pre_and_registered_users to prevent excessive results --- .../simcore_service_webserver/users/_accounts_repository.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py b/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py index 796512fe3dd3..b29612fa344b 100644 --- a/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py +++ b/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py @@ -351,6 +351,7 @@ async def search_merged_pre_and_registered_users( filter_by_user_name_like: str | None = None, filter_by_primary_group_id: int | None = None, product_name: ProductName | None = None, + limit: int = 50, ) -> list[Row]: """Searches and merges users from both users and pre-registration tables""" users_alias = sa.alias(users, name="users_alias") @@ -423,6 +424,9 @@ async def search_merged_pre_and_registered_users( final_query = queries[0] if len(queries) == 1 else sa.union(*queries) + # Add limit to prevent excessive results + final_query = final_query.limit(limit) + async with pass_or_acquire_connection(engine, connection) as conn: result = await conn.execute(final_query) return result.fetchall() From bec4870cda07550526db1db901c064194a9a59d8 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Tue, 21 Oct 2025 18:28:56 +0200 Subject: [PATCH 14/23] Refactor list_merged_pre_and_registered_users to use MergedUserData type and update ordering logic with OrderDirection --- .../users/_accounts_repository.py | 65 +++++++++++++++---- 1 file changed, 52 insertions(+), 13 deletions(-) diff --git a/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py b/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py index b29612fa344b..f3b72322e2ef 100644 --- a/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py +++ b/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py @@ -1,10 +1,12 @@ import logging -from typing import Annotated, Any, Literal, TypeAlias, cast +from datetime import datetime +from typing import Annotated, Any, Literal, TypeAlias, TypedDict, cast import sqlalchemy as sa from annotated_types import doc from common_library.exclude import Unset, is_unset from common_library.users_enums import AccountRequestStatus +from models_library.list_operations import OrderDirection from models_library.products import ProductName from models_library.users import ( UserID, @@ -433,7 +435,43 @@ async def search_merged_pre_and_registered_users( OrderKeys: TypeAlias = Literal["email", "current_status_created"] -OrderDirs: TypeAlias = Literal["asc", "desc"] + + +class MergedUserData(TypedDict, total=False): + """Type definition for merged user data returned by list_merged_pre_and_registered_users.""" + + # Pre-registration specific fields + id: int | None # pre-registration ID + pre_reg_user_id: int | None # user_id from pre-registration table + institution: str | None + address: str | None + city: str | None + state: str | None + postal_code: str | None + country: str | None + extras: dict[str, Any] | None + account_request_status: AccountRequestStatus | None + account_request_reviewed_by: int | None + account_request_reviewed_at: datetime | None + created_by: int | None + account_request_reviewed_by_username: str | None + + # Common fields (from either pre-registration or users table) + email: str + first_name: str | None + last_name: str | None + phone: str | None + created: datetime | None + current_status_created: datetime + + # User table specific fields + user_id: int | None # actual user ID from users table + user_name: str | None + user_primary_group_id: int | None + status: str | None # UserStatus + + # Computed fields + is_pre_registered: bool @validate_call(config={"arbitrary_types_allowed": True}) @@ -452,12 +490,13 @@ async def list_merged_pre_and_registered_users( pagination_limit: int = 50, pagination_offset: int = 0, order_by: Annotated[ - list[tuple[OrderKeys, OrderDirs]] | None, + list[tuple[OrderKeys, OrderDirection]] | None, doc( - 'Valid fields: "email", "current_status_created". Default: [("email", "asc"), ("is_pre_registered", "desc"), ("current_status_created", "desc")]' + 'Valid fields: "email", "current_status_created". ' + 'Default: [("email", OrderDirection.ASC), ("is_pre_registered", OrderDirection.DESC), ("current_status_created", OrderDirection.DESC)]' ), ] = None, -) -> tuple[list[dict[str, Any]], int]: +) -> tuple[list[MergedUserData], int]: """Retrieves and merges users from both users and pre-registration tables. This returns: @@ -633,34 +672,34 @@ async def list_merged_pre_and_registered_users( result = await conn.execute(final_query) records = result.mappings().all() - return cast(list[dict[str, Any]], records), total_count + return cast(list[MergedUserData], records), total_count def _build_ordering_clauses_for_filtered_query( query: sa.sql.Select, - order_by: list[tuple[OrderKeys, OrderDirs]] | None = None, + order_by: list[tuple[OrderKeys, OrderDirection]] | None = None, ) -> list[sa.sql.ColumnElement]: """Build ORDER BY clauses for filtered query (no DISTINCT ON constraints).""" - _ordering_criteria: list[tuple[str, OrderDirs]] = [] + _ordering_criteria: list[tuple[str, OrderDirection]] = [] if order_by is None: # Default ordering _ordering_criteria = [ - ("email", "asc"), - ("is_pre_registered", "desc"), - ("current_status_created", "desc"), + ("email", OrderDirection.ASC), + ("is_pre_registered", OrderDirection.DESC), + ("current_status_created", OrderDirection.DESC), ] else: _ordering_criteria = list(order_by) # Always append is_pre_registered prioritization for custom ordering if not any(field == "is_pre_registered" for field, _ in order_by): - _ordering_criteria.append(("is_pre_registered", "desc")) + _ordering_criteria.append(("is_pre_registered", OrderDirection.DESC)) order_by_clauses = [] for field, direction in _ordering_criteria: # Get column from the query's selected columns column = next(col for col in query.selected_columns if col.name == field) - if direction == "asc": + if direction == OrderDirection.ASC: order_by_clauses.append(column.asc()) else: order_by_clauses.append(column.desc()) From a47d141c7aa7600270206bedefd31c8d3d31715f Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Tue, 21 Oct 2025 18:38:35 +0200 Subject: [PATCH 15/23] Refactor user account functions to enhance parameter documentation and type annotations --- .../users/_accounts_service.py | 84 ++++++++++--------- 1 file changed, 45 insertions(+), 39 deletions(-) diff --git a/services/web/server/src/simcore_service_webserver/users/_accounts_service.py b/services/web/server/src/simcore_service_webserver/users/_accounts_service.py index 0baf07c9a1f2..bfe225633333 100644 --- a/services/web/server/src/simcore_service_webserver/users/_accounts_service.py +++ b/services/web/server/src/simcore_service_webserver/users/_accounts_service.py @@ -1,19 +1,22 @@ import logging -from typing import Any +from typing import Annotated, Any from aiohttp import web +from annotated_types import doc from common_library.users_enums import AccountRequestStatus from models_library.api_schemas_webserver.users import UserAccountGet from models_library.emails import LowerCaseEmailStr +from models_library.list_operations import OrderDirection from models_library.products import ProductName from models_library.users import UserID from notifications_library._email import create_email_session from pydantic import HttpUrl from settings_library.email import SMTPSettings -from simcore_service_webserver.products._service import get_product from ..db.plugin import get_asyncpg_engine +from ..products._service import get_product from . import _accounts_repository, _users_repository +from ._accounts_repository import OrderKeys from .exceptions import ( AlreadyPreRegisteredError, PendingPreRegistrationNotFoundError, @@ -31,7 +34,10 @@ async def pre_register_user( app: web.Application, *, profile: UserAccountRestPreRegister, - creator_user_id: UserID | None, + creator_user_id: Annotated[ + UserID | None, + doc("ID of the user creating the pre-registration (None for anonymous)"), + ], product_name: ProductName, ) -> UserAccountGet: @@ -92,19 +98,23 @@ async def list_user_accounts( app: web.Application, *, product_name: ProductName, - filter_any_account_request_status: list[AccountRequestStatus] | None = None, + filter_any_account_request_status: Annotated[ + list[AccountRequestStatus] | None, + doc("List of any account request statuses to filter by"), + ] = None, pagination_limit: int = 50, pagination_offset: int = 0, -) -> tuple[list[dict[str, Any]], int]: + order_by: Annotated[ + list[tuple[OrderKeys, OrderDirection]] | None, + doc("List of (field_name, direction) tuples for ordering"), + ] = None, +) -> Annotated[ + tuple[list[dict[str, Any]], int], + doc("Tuple containing (list of user dictionaries, total count of users)"), +]: """ Get a paginated list of users for admin view with filtering options. - Args: - app: The web application instance - filter_any_account_request_status: List of *any* account request statuses to filter by - pagination_limit: Maximum number of users to return - pagination_offset: Number of users to skip for pagination - Returns: A tuple containing (list of user dictionaries, total count of users) """ @@ -118,6 +128,7 @@ async def list_user_accounts( filter_any_account_request_status=filter_any_account_request_status, pagination_limit=pagination_limit, pagination_offset=pagination_offset, + order_by=order_by, ) ) @@ -155,8 +166,7 @@ async def search_users_accounts( product_name: ProductName | None = None, include_products: bool = False, ) -> list[UserAccountGet]: - """ - WARNING: this information is reserved for admin users. Note that the returned model include UserForAdminGet + """WARNING: this information is reserved for admin users. Note that the returned model include UserForAdminGet NOTE: Functions in the service layer typically validate the caller's access rights using parameters like product_name and user_id. However, this function skips @@ -238,18 +248,14 @@ async def approve_user_account( pre_registration_email: LowerCaseEmailStr, product_name: ProductName, reviewer_id: UserID, - invitation_extras: dict[str, Any] | None = None, -) -> int: + invitation_extras: Annotated[ + dict[str, Any] | None, doc("Optional invitation data to store in extras field") + ] = None, +) -> Annotated[int, doc("The ID of the approved pre-registration record")]: """Approve a user account based on their pre-registration email. - Args: - app: The web application instance - pre_registration_email: Email of the pre-registered user to approve - product_name: Product name for which the user is being approved - reviewer_id: ID of the user approving the account - Returns: - int: The ID of the approved pre-registration record + The ID of the approved pre-registration record Raises: PendingPreRegistrationNotFoundError: If no pre-registration is found for the email/product @@ -291,17 +297,9 @@ async def reject_user_account( pre_registration_email: LowerCaseEmailStr, product_name: ProductName, reviewer_id: UserID, -) -> int: +) -> Annotated[int, doc("The ID of the rejected pre-registration record")]: """Reject a user account based on their pre-registration email. - Args: - app: The web application instance - pre_registration_email: Email of the pre-registered user to reject - product_name: Product name for which the user is being rejected - reviewer_id: ID of the user rejecting the account - - Returns: - int: The ID of the rejected pre-registration record Raises: PendingPreRegistrationNotFoundError: If no pre-registration is found for the email/product @@ -343,9 +341,17 @@ def _create_product_and_user_data( user_email: LowerCaseEmailStr, first_name: str, last_name: str, -): +) -> Annotated[ + tuple[Any, Any], + doc("Tuple containing (ProductData, UserData) objects for email rendering"), +]: """Create ProductData and UserData objects for email rendering.""" - from notifications_library._models import ProductData, ProductUIData, UserData + + from notifications_library._models import ( # noqa: PLC0415 + ProductData, + ProductUIData, + UserData, + ) # Get product data from the app product = get_product(app, product_name=product_name) @@ -399,13 +405,13 @@ async def send_approval_email_to_user( first_name: str, last_name: str, ) -> None: - from notifications_library._email import compose_email - from notifications_library._email_render import ( + from notifications_library._email import compose_email # noqa: PLC0415 + from notifications_library._email_render import ( # noqa: PLC0415 get_support_address, get_user_address, render_email_parts, ) - from notifications_library._render import ( + from notifications_library._render import ( # noqa: PLC0415 create_render_environment_from_notifications_library, ) @@ -456,13 +462,13 @@ async def send_rejection_email_to_user( last_name: str, host: str, ) -> None: - from notifications_library._email import compose_email - from notifications_library._email_render import ( + from notifications_library._email import compose_email # noqa: PLC0415 + from notifications_library._email_render import ( # noqa: PLC0415 get_support_address, get_user_address, render_email_parts, ) - from notifications_library._render import ( + from notifications_library._render import ( # noqa: PLC0415 create_render_environment_from_notifications_library, ) From 82316589dd81017c7454645850d8555413e44bc7 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Tue, 21 Oct 2025 18:53:36 +0200 Subject: [PATCH 16/23] Refactor check_ordering_list to enhance parameter documentation and type annotations; remove unused functions and imports --- .../src/models_library/list_operations.py | 85 ++------------- .../tests/test_list_operations.py | 103 ------------------ .../tests/test_rest_ordering.py | 3 - 3 files changed, 12 insertions(+), 179 deletions(-) diff --git a/packages/models-library/src/models_library/list_operations.py b/packages/models-library/src/models_library/list_operations.py index 7f86570ec812..eb0451940872 100644 --- a/packages/models-library/src/models_library/list_operations.py +++ b/packages/models-library/src/models_library/list_operations.py @@ -6,8 +6,9 @@ """ from enum import Enum -from typing import TYPE_CHECKING, Any, Generic, Literal, TypeVar, get_args, get_origin +from typing import TYPE_CHECKING, Annotated, Generic, TypeVar +from annotated_types import doc from pydantic.generics import GenericModel @@ -29,34 +30,24 @@ def __str__(self) -> str: ... TField = TypeVar("TField", bound=str) -def get_literal_values(tfield: Any) -> tuple[str, ...] | None: - """Return Literal values if TField is a Literal, else None.""" - if get_origin(tfield) is Literal: - return get_args(tfield) - return None - - class OrderClause(GenericModel, Generic[TField]): field: TField direction: OrderDirection = OrderDirection.ASC def check_ordering_list( - order_by: list[tuple[TField, OrderDirection]], -) -> list[tuple[TField, OrderDirection]]: + order_by: Annotated[ + list[tuple[TField, OrderDirection]], + doc( + "Duplicates with same direction dropped, conflicting directions raise ValueError" + ), + ], +) -> Annotated[ + list[tuple[TField, OrderDirection]], + doc("Duplicates removed, preserving first occurrence order"), +]: """Validates ordering list and removes duplicate entries. - Ensures that each field appears at most once. If a field is repeated: - - With the same direction: silently drops the duplicate - - With different directions: raises ValueError - - - Args: - order_by: List of (field, direction) tuples - - Returns: - List with duplicates removed, preserving order of first occurrence - Raises: ValueError: If a field appears with conflicting directions """ @@ -77,55 +68,3 @@ def check_ordering_list( unique_order_by.append((field, direction)) return unique_order_by - - -def map_order_fields( - order_clauses: list[OrderClause[TField]], field_mapping: dict[str, str] -) -> list[tuple[str, OrderDirection]]: - """Map order clause fields using a field mapping dictionary. - - Args: - order_clauses: List of OrderClause objects with API field names - field_mapping: Dictionary mapping API field names to domain/DB field names - - Returns: - List of tuples with mapped field names and directions - - Example: - >>> clauses = [OrderClause(field="email", direction=OrderDirection.ASC)] - >>> mapping = {"email": "user_email", "created_at": "created"} - >>> map_order_fields(clauses, mapping) - [("user_email", OrderDirection.ASC)] - """ - return [ - (field_mapping[str(clause.field)], clause.direction) for clause in order_clauses - ] - - -def validate_order_fields_with_literals( - order_by: list[tuple[str, str]], - valid_fields: set[str], -) -> None: - """Validate order_by list with string field names and directions. - - Args: - order_by: List of (field_name, direction) tuples with string values - valid_fields: Set of allowed field names - valid_directions: Set of allowed direction values - - Raises: - ValueError: If any field or direction is invalid - """ - valid_directions = {OrderDirection.ASC.value, OrderDirection.DESC.value} - - invalid_fields = {field for field, _ in order_by if field not in valid_fields} - if invalid_fields: - msg = f"Invalid order_by field(s): {invalid_fields}. Valid fields are: {valid_fields}" - raise ValueError(msg) - - invalid_directions = { - direction for _, direction in order_by if direction not in valid_directions - } - if invalid_directions: - msg = f"Invalid order direction(s): {invalid_directions}. Must be one of: {valid_directions}" - raise ValueError(msg) diff --git a/packages/models-library/tests/test_list_operations.py b/packages/models-library/tests/test_list_operations.py index 624489312f89..61ff91aa5d4d 100644 --- a/packages/models-library/tests/test_list_operations.py +++ b/packages/models-library/tests/test_list_operations.py @@ -1,12 +1,7 @@ -from typing import Literal - import pytest from models_library.list_operations import ( - OrderClause, OrderDirection, check_ordering_list, - map_order_fields, - validate_order_fields_with_literals, ) @@ -73,101 +68,3 @@ def test_check_ordering_list_no_duplicates(): # Should return the same list assert result == order_by - - -def test_map_order_fields(): - """Test that map_order_fields correctly maps field names using provided mapping""" - - ValidField = Literal["email", "created_at", "name"] - - order_clauses = [ - OrderClause[ValidField](field="email", direction=OrderDirection.ASC), - OrderClause[ValidField](field="created_at", direction=OrderDirection.DESC), - OrderClause[ValidField](field="name", direction=OrderDirection.ASC), - ] - - field_mapping = { - "email": "user_email", - "created_at": "created_timestamp", - "name": "display_name", - } - - result = map_order_fields(order_clauses, field_mapping) - - expected = [ - ("user_email", OrderDirection.ASC), - ("created_timestamp", OrderDirection.DESC), - ("display_name", OrderDirection.ASC), - ] - - assert result == expected - - -def test_map_order_fields_with_unmapped_field(): - """Test that map_order_fields raises KeyError when field is not in mapping""" - - ValidField = Literal["email", "unknown"] - - order_clauses = [ - OrderClause[ValidField](field="email", direction=OrderDirection.ASC), - OrderClause[ValidField](field="unknown", direction=OrderDirection.DESC), - ] - - field_mapping = { - "email": "user_email", - # "unknown" is missing from mapping - } - - with pytest.raises(KeyError): - map_order_fields(order_clauses, field_mapping) - - -def test_validate_order_fields_with_literals_valid(): - """Test that validate_order_fields_with_literals passes with valid fields and directions""" - - order_by = [ - ("email", "asc"), - ("created", "desc"), - ("name", "asc"), - ] - - valid_fields = {"email", "created", "name"} - - # Should not raise any exception - validate_order_fields_with_literals(order_by, valid_fields) - - -def test_validate_order_fields_with_literals_invalid_field(): - """Test that validate_order_fields_with_literals raises ValueError for invalid fields""" - - order_by = [ - ("email", "asc"), - ("invalid_field", "desc"), - ] - - valid_fields = {"email", "created"} - - with pytest.raises(ValueError, match="Invalid order_by field") as exc_info: - validate_order_fields_with_literals(order_by, valid_fields) - - error_msg = str(exc_info.value) - assert "invalid_field" in error_msg - assert "Valid fields are" in error_msg - - -def test_validate_order_fields_with_literals_invalid_direction(): - """Test that validate_order_fields_with_literals raises ValueError for invalid directions""" - - order_by = [ - ("email", "ascending"), # Invalid direction - ("created", "desc"), - ] - - valid_fields = {"email", "created"} - - with pytest.raises(ValueError, match="Invalid order direction") as exc_info: - validate_order_fields_with_literals(order_by, valid_fields) - - error_msg = str(exc_info.value) - assert "ascending" in error_msg - assert "Must be one of" in error_msg diff --git a/packages/models-library/tests/test_rest_ordering.py b/packages/models-library/tests/test_rest_ordering.py index 2b214d218be1..6feda4731df6 100644 --- a/packages/models-library/tests/test_rest_ordering.py +++ b/packages/models-library/tests/test_rest_ordering.py @@ -4,7 +4,6 @@ import pytest from common_library.json_serialization import json_dumps from models_library.basic_types import IDStr -from models_library.list_operations import get_literal_values from models_library.rest_ordering import ( OrderBy, OrderClause, @@ -260,8 +259,6 @@ class TestOrderingParams(OrderingQueryParams[ValidField]): OrderClause[ValidField](field="gender", direction=OrderDirection.ASC), ] - assert get_literal_values(ValidField) == ("created_at", "name", "gender") - def test_ordering_query_params_validation_error_with_invalid_fields(): """Test that OrderingQueryParams raises ValidationError when invalid fields are used""" From a9c5cad518bf8da5242563cdb48dcfac4fa4f78e Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Tue, 21 Oct 2025 18:53:45 +0200 Subject: [PATCH 17/23] Refactor user pre-registration functions to improve parameter documentation and remove unused arguments; enhance user account listing with order mapping --- .../users/_accounts_repository.py | 47 +++---------------- .../users/_controller/rest/accounts_rest.py | 19 +++++++- 2 files changed, 23 insertions(+), 43 deletions(-) diff --git a/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py b/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py index f3b72322e2ef..452a514c95ad 100644 --- a/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py +++ b/services/web/server/src/simcore_service_webserver/users/_accounts_repository.py @@ -45,15 +45,6 @@ async def create_user_pre_registration( ) -> int: """Creates a user pre-registration entry. - Args: - engine: Database engine - connection: Optional existing connection - email: Email address for the pre-registration - created_by: ID of the user creating the pre-registration (None for anonymous) - product_name: Product name the user is requesting access to - link_to_existing_user: Whether to link the pre-registration to an existing user with the same email - **other_values: Additional values to insert in the pre-registration entry - Returns: ID of the created pre-registration """ @@ -104,15 +95,6 @@ async def list_user_pre_registrations( ) -> tuple[list[dict[str, Any]], int]: """Lists user pre-registrations with optional filters. - Args: - engine: Database engine - connection: Optional existing connection - filter_by_pre_email: Filter by email pattern (SQL LIKE pattern) - filter_by_product_name: Filter by product name - filter_by_account_request_status: Filter by account request status - pagination_limit: Maximum number of results to return - pagination_offset: Number of results to skip (for pagination) - Returns: Tuple of (list of pre-registration records, total count) """ @@ -216,16 +198,7 @@ async def review_user_pre_registration( new_status: AccountRequestStatus, invitation_extras: dict[str, Any] | None = None, ) -> None: - """Updates the account request status of a pre-registered user. - - Args: - engine: The database engine - connection: Optional existing connection - pre_registration_id: ID of the pre-registration record - reviewed_by: ID of the user who reviewed the request - new_status: New status (APPROVED or REJECTED) - invitation_extras: Optional invitation data to store in extras field - """ + """Updates the account request status of a pre-registered user.""" if new_status not in (AccountRequestStatus.APPROVED, AccountRequestStatus.REJECTED): msg = f"Invalid status for review: {new_status}. Must be APPROVED or REJECTED." raise ValueError(msg) @@ -482,30 +455,22 @@ async def list_merged_pre_and_registered_users( product_name: ProductName, filter_any_account_request_status: Annotated[ list[AccountRequestStatus] | None, - doc( - "If provided, only returns users with account request status in this list (only pre-registered users with any of these statuses will be included)" - ), + doc("Only returns users with these statuses (pre-registered users only)"), ] = None, filter_include_deleted: bool = False, pagination_limit: int = 50, pagination_offset: int = 0, order_by: Annotated[ list[tuple[OrderKeys, OrderDirection]] | None, - doc( - 'Valid fields: "email", "current_status_created". ' - 'Default: [("email", OrderDirection.ASC), ("is_pre_registered", OrderDirection.DESC), ("current_status_created", OrderDirection.DESC)]' - ), + doc('Valid fields: "email", "current_status_created"'), ] = None, ) -> tuple[list[MergedUserData], int]: """Retrieves and merges users from both users and pre-registration tables. - This returns: - 1. Users who are registered with the platform (in users table) - 2. Users who are pre-registered (in users_pre_registration_details table) - 3. Users who are both registered and pre-registered - Returns: - Tuple of (list of merged user data, total count) + 1. Users registered with the platform (users table) + 2. Users pre-registered (users_pre_registration_details table) + 3. Users both registered and pre-registered """ # Base where conditions for both queries pre_reg_query_conditions = [ diff --git a/services/web/server/src/simcore_service_webserver/users/_controller/rest/accounts_rest.py b/services/web/server/src/simcore_service_webserver/users/_controller/rest/accounts_rest.py index c5eb281cfe22..9a358c74cd43 100644 --- a/services/web/server/src/simcore_service_webserver/users/_controller/rest/accounts_rest.py +++ b/services/web/server/src/simcore_service_webserver/users/_controller/rest/accounts_rest.py @@ -7,6 +7,7 @@ from models_library.api_schemas_webserver.users import ( UserAccountApprove, UserAccountGet, + UserAccountOrderFields, UserAccountReject, UserAccountSearchQueryParams, UsersAccountListQueryParams, @@ -21,6 +22,7 @@ from servicelib.logging_utils import log_context from servicelib.rest_constants import RESPONSE_MODEL_POLICY from servicelib.utils import fire_and_forget_task +from simcore_service_webserver.users._accounts_repository import OrderKeys from ...._meta import API_VTAG from ....constants import APP_FIRE_AND_FORGET_TASKS_KEY @@ -63,16 +65,26 @@ async def list_users_accounts(request: web.Request) -> web.Response: AccountRequestStatus.APPROVED, AccountRequestStatus.REJECTED, ] - else: - # ALL + else: # ALL filter_any_account_request_status = None + # Map API field names to service/repository field names + api_to_service_field_mapping: dict[UserAccountOrderFields, OrderKeys] = { + "email": "email", + "created_at": "current_status_created", + } + user_accounts, total_count = await _accounts_service.list_user_accounts( request.app, product_name=req_ctx.product_name, filter_any_account_request_status=filter_any_account_request_status, pagination_limit=query_params.limit, pagination_offset=query_params.offset, + order_by=[ + (api_to_service_field_mapping[clause.field], clause.direction) + for clause in query_params.order_by + ] + or None, ) def _to_domain_model(account_details: dict[str, Any]) -> UserAccountGet: @@ -108,6 +120,7 @@ async def search_user_accounts(request: web.Request) -> web.Response: req_ctx = UsersRequestContext.model_validate(request) assert req_ctx.product_name # nosec + # TODO: add sorting options query_params: UserAccountSearchQueryParams = parse_request_query_parameters_as( UserAccountSearchQueryParams, request ) @@ -201,6 +214,8 @@ async def approve_user_account(request: web.Request) -> web.Response: product_name=req_ctx.product_name, reviewer_id=req_ctx.user_id, invitation_extras=( + # TODO: remove invitation link + # TODO: use created as a reference to check whether it is expired or not {"invitation": invitation_result.model_dump(mode="json")} if invitation_result else None From 5a93bc9933c8d0ac71b5ada7a46878e63d614e28 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Tue, 21 Oct 2025 18:59:12 +0200 Subject: [PATCH 18/23] updates doc rules --- .github/instructions/python.instructions.md | 57 +++++++++++++++++---- 1 file changed, 46 insertions(+), 11 deletions(-) diff --git a/.github/instructions/python.instructions.md b/.github/instructions/python.instructions.md index 0527fbec2234..a7f008830277 100644 --- a/.github/instructions/python.instructions.md +++ b/.github/instructions/python.instructions.md @@ -1,28 +1,65 @@ --- applyTo: '**/*.py' --- -Provide project context and coding guidelines that AI should follow when generating code, answering questions, or reviewing changes. ## 🛠️Coding Instructions for Python in This Repository Follow these rules **strictly** when generating Python code: -### 1. Python Version +### Python Version * Use Python 3.13: Ensure all code uses features and syntax compatible with Python 3.13. -### 2. **Type Annotations** +### Type Annotations * Always use full type annotations for all functions and class attributes. * ❗ **Exception**: Do **not** add return type annotations in `test_*` functions. -### 3. **Code Style & Formatting** +### Documentation with Annotated Types + +* Use `annotated_types.doc()` for parameter and return type documentation instead of traditional docstring Args/Returns sections +* **Apply documentation only for non-obvious parameters/returns**: + - Document complex behaviors that can't be deduced from parameter name and type + - Document validation rules, side effects, or special handling + - Skip documentation for self-explanatory parameters (e.g., `engine: AsyncEngine`, `product_name: ProductName`) +* **Import**: Always add `from annotated_types import doc` when using documentation annotations + +**Examples:** +```python +from typing import Annotated +from annotated_types import doc + +async def process_users( + engine: AsyncEngine, # No doc needed - self-explanatory + filter_statuses: Annotated[ + list[Status] | None, + doc("Only returns users with these statuses") + ] = None, + limit: int = 50, # No doc needed - obvious +) -> Annotated[ + tuple[list[dict], int], + doc("(user records, total count)") +]: + """Process users with filtering. + + Raises: + ValueError: If no filters provided + """ +``` + +* **Docstring conventions**: + - Keep docstrings **concise**, focusing on overall function purpose + - Include `Raises:` section for exceptions + - Avoid repeating information already captured in type annotations + - Most information should be deducible from function name, parameter names, types, and annotations + +### Code Style & Formatting * Follow [Python Coding Conventions](../../docs/coding-conventions.md) **strictly**. * Format code with `black` and `ruff`. * Lint code with `ruff` and `pylint`. -### 4. **Library Compatibility** +### Library Compatibility Ensure compatibility with the following library versions: @@ -30,8 +67,7 @@ Ensure compatibility with the following library versions: * `pydantic` ≥ 2.x * `fastapi` ≥ 0.100 - -### 5. **Code Practices** +### Code Practices * Use `f-string` formatting for all string interpolation except for logging message strings. * Use **relative imports** within the same package/module. @@ -40,13 +76,12 @@ Ensure compatibility with the following library versions: * Place **all imports at the top** of the file. * Document functions when the code is not self-explanatory or if asked explicitly. - -### 6. **JSON Serialization** +### JSON Serialization * Prefer `json_dumps` / `json_loads` from `common_library.json_serialization` instead of the built-in `json.dumps` / `json.loads`. * When using Pydantic models, prefer methods like `model.model_dump_json()` for serialization. -### 7. **aiohttp Framework** +### aiohttp Framework * **Application Keys**: Always use `web.AppKey` for type-safe application storage instead of string keys - Define keys with specific types: `APP_MY_KEY: Final = web.AppKey("APP_MY_KEY", MySpecificType)` @@ -58,6 +93,6 @@ Ensure compatibility with the following library versions: * **Error Handling**: Use the established exception handling decorators and patterns * **Route Definitions**: Use `web.RouteTableDef()` and organize routes logically within modules -### 8. **Running tests** +### Running tests * Use `--keep-docker-up` flag when testing to keep docker containers up between sessions. * Always activate the python virtual environment before running pytest. From 513de41442fab0b339697f1add41c4115660f62c Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Tue, 21 Oct 2025 19:35:22 +0200 Subject: [PATCH 19/23] mionr --- .../users/_accounts_service.py | 2 ++ .../users/_controller/rest/accounts_rest.py | 14 ++++++++------ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/services/web/server/src/simcore_service_webserver/users/_accounts_service.py b/services/web/server/src/simcore_service_webserver/users/_accounts_service.py index bfe225633333..0dfb26b1f7a8 100644 --- a/services/web/server/src/simcore_service_webserver/users/_accounts_service.py +++ b/services/web/server/src/simcore_service_webserver/users/_accounts_service.py @@ -172,6 +172,8 @@ async def search_users_accounts( using parameters like product_name and user_id. However, this function skips such checks as it is designed for scenarios (e.g., background tasks) where no caller or context is available. + + NOTE: list is limited to a maximum of 50 entries """ if ( diff --git a/services/web/server/src/simcore_service_webserver/users/_controller/rest/accounts_rest.py b/services/web/server/src/simcore_service_webserver/users/_controller/rest/accounts_rest.py index 9a358c74cd43..3358d2d6b50f 100644 --- a/services/web/server/src/simcore_service_webserver/users/_controller/rest/accounts_rest.py +++ b/services/web/server/src/simcore_service_webserver/users/_controller/rest/accounts_rest.py @@ -80,11 +80,14 @@ async def list_users_accounts(request: web.Request) -> web.Response: filter_any_account_request_status=filter_any_account_request_status, pagination_limit=query_params.limit, pagination_offset=query_params.offset, - order_by=[ - (api_to_service_field_mapping[clause.field], clause.direction) - for clause in query_params.order_by - ] - or None, + order_by=( + [ + (api_to_service_field_mapping[clause.field], clause.direction) + for clause in query_params.order_by + ] + if query_params.order_by + else None + ), ) def _to_domain_model(account_details: dict[str, Any]) -> UserAccountGet: @@ -120,7 +123,6 @@ async def search_user_accounts(request: web.Request) -> web.Response: req_ctx = UsersRequestContext.model_validate(request) assert req_ctx.product_name # nosec - # TODO: add sorting options query_params: UserAccountSearchQueryParams = parse_request_query_parameters_as( UserAccountSearchQueryParams, request ) From 9f4145526fc4ec302001f52212eef897a2cacc9c Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Tue, 21 Oct 2025 19:40:39 +0200 Subject: [PATCH 20/23] fixes mypy after merge --- .../src/models_library/api_schemas_webserver/users.py | 4 ++-- packages/models-library/src/models_library/list_operations.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/models-library/src/models_library/api_schemas_webserver/users.py b/packages/models-library/src/models_library/api_schemas_webserver/users.py index b07f49fdf20a..a4f45a07c4e3 100644 --- a/packages/models-library/src/models_library/api_schemas_webserver/users.py +++ b/packages/models-library/src/models_library/api_schemas_webserver/users.py @@ -1,7 +1,7 @@ import re from datetime import date, datetime from enum import Enum -from typing import Annotated, Any, Literal, Self +from typing import Annotated, Any, Literal, Self, TypeAlias import annotated_types from common_library.basic_types import DEFAULT_FACTORY @@ -27,12 +27,12 @@ from ..groups import AccessRightsDict, Group, GroupID, GroupsByTypeTuple, PrimaryGroupID from ..products import ProductName from ..rest_base import RequestParameters +from ..rest_ordering import OrderingQueryParams from ..string_types import ( GlobPatternSafeStr, SearchPatternSafeStr, validate_input_xss_safety, ) -from ..rest_ordering import OrderingQueryParams from ..users import ( FirstNameStr, LastNameStr, diff --git a/packages/models-library/src/models_library/list_operations.py b/packages/models-library/src/models_library/list_operations.py index eb0451940872..0d09c27986c8 100644 --- a/packages/models-library/src/models_library/list_operations.py +++ b/packages/models-library/src/models_library/list_operations.py @@ -51,7 +51,7 @@ def check_ordering_list( Raises: ValueError: If a field appears with conflicting directions """ - seen_fields = {} + seen_fields: dict[TField, OrderDirection] = {} unique_order_by = [] for field, direction in order_by: From f2162bddfbca5dea11d48487b1057ca6b28d7077 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Tue, 21 Oct 2025 19:44:43 +0200 Subject: [PATCH 21/23] drop Generic --- packages/models-library/src/models_library/list_operations.py | 4 ++-- packages/models-library/src/models_library/rest_ordering.py | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/models-library/src/models_library/list_operations.py b/packages/models-library/src/models_library/list_operations.py index 0d09c27986c8..263cc129ccaa 100644 --- a/packages/models-library/src/models_library/list_operations.py +++ b/packages/models-library/src/models_library/list_operations.py @@ -9,7 +9,7 @@ from typing import TYPE_CHECKING, Annotated, Generic, TypeVar from annotated_types import doc -from pydantic.generics import GenericModel +from pydantic import BaseModel class OrderDirection(str, Enum): @@ -30,7 +30,7 @@ def __str__(self) -> str: ... TField = TypeVar("TField", bound=str) -class OrderClause(GenericModel, Generic[TField]): +class OrderClause(BaseModel, Generic[TField]): field: TField direction: OrderDirection = OrderDirection.ASC diff --git a/packages/models-library/src/models_library/rest_ordering.py b/packages/models-library/src/models_library/rest_ordering.py index 753bea42cb1f..7e2d829d926c 100644 --- a/packages/models-library/src/models_library/rest_ordering.py +++ b/packages/models-library/src/models_library/rest_ordering.py @@ -9,7 +9,6 @@ Field, field_validator, ) -from pydantic.generics import GenericModel from .basic_types import IDStr from .list_operations import OrderClause, OrderDirection, TField, check_ordering_list @@ -160,7 +159,7 @@ def _parse_order_by(v): ] -class OrderingQueryParams(GenericModel, Generic[TField]): +class OrderingQueryParams(BaseModel, Generic[TField]): """ This class is designed to parse query parameters for ordering results in an API request. From de47d01372a22d2978a5b43ac0c1f8292e9a6539 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Tue, 21 Oct 2025 19:56:58 +0200 Subject: [PATCH 22/23] adds link to Review Users section provided in https://github.com/ITISFoundation/osparc-simcore/pull/8537 --- .../templates/on_account_requested.email.content.html | 1 + .../templates/on_account_requested.email.content.txt | 2 +- .../templates/common/request_account.jinja2 | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.html b/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.html index 5a317574c958..9661b0b3d19e 100644 --- a/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.html +++ b/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.html @@ -8,6 +8,7 @@
  1. Product: {{ product.display_name }}
  2. Host: {{ host }}
    3. +
  3. PO center: https://{{host}}/#/review-users

diff --git a/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.txt b/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.txt index 67b1801b9123..54dcd0579703 100644 --- a/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.txt +++ b/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.txt @@ -3,7 +3,7 @@ Dear Support team, We have received the following request form for an account in : - Product: **{{ product.display_name }}** - Host: **{{ host }}** - +- PO center: https://{{host}}/#/review-users {{ dumps(request_form) }} Some details about the requested product follow: diff --git a/services/web/server/src/simcore_service_webserver/templates/common/request_account.jinja2 b/services/web/server/src/simcore_service_webserver/templates/common/request_account.jinja2 index bc85afb0559a..688dfdd918b9 100644 --- a/services/web/server/src/simcore_service_webserver/templates/common/request_account.jinja2 +++ b/services/web/server/src/simcore_service_webserver/templates/common/request_account.jinja2 @@ -15,6 +15,7 @@ Dear {{ name }},
  1. Product: {{ product.display_name }}
  2. Host: {{ host }}
    3. +
  3. PO center: https://{{host}}/#/review-users

From edc226057b590e71c81ec34d6345ddc3223003ba Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Tue, 21 Oct 2025 20:02:21 +0200 Subject: [PATCH 23/23] fix: ensure host URLs are properly formatted by stripping trailing slashes --- .../templates/on_account_requested.email.content.html | 6 +++--- .../templates/on_account_requested.email.content.txt | 4 ++-- .../templates/common/request_account.jinja2 | 8 ++++---- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.html b/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.html index 9661b0b3d19e..d922a71ad6c2 100644 --- a/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.html +++ b/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.html @@ -1,14 +1,14 @@ {% extends 'base.html' %} {% block title %} {% include 'on_account_requested.email.subject.txt' %} {% endblock %} {% block content %} -

Dear Support team +

Dear Support team

We have received the following request form for an account in :

  1. Product: {{ product.display_name }}
    2. -
  2. Host: {{ host }}
    3. -
  3. PO center: https://{{host}}/#/review-users
    4. +
  4. Host: {{ host|rstrip('/') }}
    5. +
  5. PO center: https://{{ host|rstrip('/') }}/#/review-users

diff --git a/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.txt b/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.txt index 54dcd0579703..f8acd5c97c40 100644 --- a/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.txt +++ b/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.txt @@ -2,8 +2,8 @@ Dear Support team, We have received the following request form for an account in : - Product: **{{ product.display_name }}** -- Host: **{{ host }}** -- PO center: https://{{host}}/#/review-users +- Host: **{{ host|rstrip('/') }}** +- PO center: https://{{ host|rstrip('/') }}/#/review-users {{ dumps(request_form) }} Some details about the requested product follow: diff --git a/services/web/server/src/simcore_service_webserver/templates/common/request_account.jinja2 b/services/web/server/src/simcore_service_webserver/templates/common/request_account.jinja2 index 688dfdd918b9..1228638df215 100644 --- a/services/web/server/src/simcore_service_webserver/templates/common/request_account.jinja2 +++ b/services/web/server/src/simcore_service_webserver/templates/common/request_account.jinja2 @@ -1,7 +1,7 @@ -Request for an Account in {{ host }} +Request for an Account in {{ host|rstrip('/') }}