sts-sponsors team mailing list archive

[Thorsten Merten <mp+442472@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code

One idea for the future, one idea for naming and one improvement for the tests inline.

Diff comments:

> diff --git a/backend/msm/db/queries.py b/backend/msm/db/queries.py
> index 3e51fbb..b46f105 100644
> --- a/backend/msm/db/queries.py
> +++ b/backend/msm/db/queries.py
> @@ -202,6 +212,34 @@ async def get_pending_sites(
>      return count, [PendingSiteSchema(**row._asdict()) for row in result.all()]
>  
>  
> +async def accept_reject_pending_sites(
> +    session: AsyncSession,
> +    ids: list[int],
> +    accept: bool,
> +) -> None:
> +    site_ids = set(ids)
> +    stmt = (
> +        select(Site.c.id)
> +        .select_from(Site)
> +        .where(
> +            Site.c.id.in_(site_ids),
> +            Site.c.accepted == False,  # noqa
> +        )
> +    )
> +    result = await session.execute(stmt)
> +    pending_ids = set(row[0] for row in result.all())
> +    if unknown_ids := site_ids - pending_ids:
> +        raise InvalidPendingSites(unknown_ids)
> +
> +    if accept:
> +        await session.execute(
> +            update(Site).where(Site.c.id.in_(site_ids)).values(accepted=True)
> +        )
> +    else:
> +        await session.execute(delete(Site).where(Site.c.id.in_(site_ids)))

I think we should log this somewhere when we start logging things :)

> +    return None
> +
> +
>  async def get_tokens(
>      session: AsyncSession,
>      offset: int = 0,
> diff --git a/backend/msm/user_api/_schema.py b/backend/msm/user_api/_schema.py
> index af1da4f..f481522 100644
> --- a/backend/msm/user_api/_schema.py
> +++ b/backend/msm/user_api/_schema.py
> @@ -31,6 +31,13 @@ class CreateTokensResponse(BaseModel):
>      tokens: list[UUID]
>  
>  
> +class PendingSitesActionRequest(BaseModel):

Should we make these kinds of models more consistent with the action? E.g. PendingSitesPostRequest

> +    """Request to accept/reject sites."""
> +
> +    ids: list[int]
> +    accept: bool
> +
> +
>  class PaginatedSites(PaginatedResults):
>      items: list[Site]
>  
> diff --git a/backend/tests/user_api/test_handlers.py b/backend/tests/user_api/test_handlers.py
> index a1301f0..e31c0bf 100644
> --- a/backend/tests/user_api/test_handlers.py
> +++ b/backend/tests/user_api/test_handlers.py
> @@ -290,6 +290,45 @@ async def test_list_pending_sites(
>  
>  
>  @pytest.mark.asyncio
> +async def test_accept_pending_sites(
> +    authenticated_user_app_client: AuthAsyncClient, fixture: Fixture
> +) -> None:
> +    site = {
> +        "name": "LondonHQ",
> +        "url": "https://londoncalling.example.com";,
> +        "accepted": False,
> +    }
> +    [pending_site] = await fixture.create("site", [site])
> +
> +    response = await authenticated_user_app_client.post(
> +        "/requests",
> +        json={"ids": [pending_site["id"]], "accept": True},
> +    )
> +    assert response.status_code == 204
> +    [created_site] = await fixture.get("site")
> +    assert created_site["accepted"]
> +
> +
> +@pytest.mark.asyncio
> +async def test_reject_pending_sites(
> +    authenticated_user_app_client: AuthAsyncClient, fixture: Fixture
> +) -> None:
> +    site = {
> +        "name": "LondonHQ",
> +        "url": "https://londoncalling.example.com";,
> +        "accepted": False,
> +    }
> +    [pending_site] = await fixture.create("site", [site])
> +
> +    response = await authenticated_user_app_client.post(
> +        "/requests",
> +        json={"ids": [pending_site["id"]], "accept": False},
> +    )
> +    assert response.status_code == 204
> +    assert await fixture.get("site") == []
> +

Would be good to have a test that raises the invalid ID error. This could also test more than one ID in the array at the same time.

> +
> +@pytest.mark.asyncio
>  @pytest.mark.parametrize("time_format", ["ISO 8601", "Float"])
>  async def test_token_time_format(
>      time_format: str, authenticated_user_app_client: AuthAsyncClient


-- 
https://code.launchpad.net/~ack/maas-site-manager/+git/site-manager/+merge/442472
Your team MAAS Committers is subscribed to branch ~ack/maas-site-manager:pending-sites-action.