From e86fd1818346121c3eadfe7469c52fb000d61488 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 17 Nov 2025 10:36:00 +0100 Subject: [PATCH 1/8] updates instructions for doc --- .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 6799564a10f91ec05efafc46e8b60a77ad90f208 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 17 Nov 2025 10:39:01 +0100 Subject: [PATCH 2/8] cleanup --- api/specs/web-server/_common.py | 2 +- .../src/models_library/invitations.py | 76 ++++++++++------- .../users/_accounts_service.py | 81 ++++++++++--------- 3 files changed, 90 insertions(+), 69 deletions(-) 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, 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/web/server/src/simcore_service_webserver/users/_accounts_service.py b/services/web/server/src/simcore_service_webserver/users/_accounts_service.py index 0baf07c9a1f2..a7ff44a5e511 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,19 @@ 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]: +) -> 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) """ @@ -155,13 +161,14 @@ 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 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 ( @@ -238,18 +245,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 +294,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 +338,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 +402,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 +459,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 9443b7dbb39117d31189cc7209bd5f5afdc774bb Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Mon, 17 Nov 2025 10:40:10 +0100 Subject: [PATCH 3/8] test --- .../templates/on_account_requested.email.content.html | 5 +++-- .../templates/on_account_requested.email.content.txt | 4 ++-- .../templates/common/request_account.jinja2 | 7 ++++--- 3 files changed, 9 insertions(+), 7 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 5a317574c958..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,13 +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. Host: {{ host|rstrip('/') }}
    4. +
  4. 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 67b1801b9123..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 }}** - +- 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 bc85afb0559a..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('/') }}