Skip to content

Conversation

@sipmine
Copy link
Contributor

@sipmine sipmine commented Sep 22, 2025

@sipmine sipmine self-assigned this Sep 22, 2025
@sipmine sipmine added ci:covered If coverage checks should be run on the PR ci:deployable If deployment of the PR should be attempted ci:migrated If migration checks should be run on the PR labels Sep 22, 2025
@sipmine sipmine force-pushed the feat/subscription-service branch 2 times, most recently from ae7b5d2 to bd0fb09 Compare September 23, 2025 13:41
@sipmine sipmine force-pushed the feat/subscription-service branch from bd0fb09 to 6bf3c9c Compare September 23, 2025 14:42
Copy link
Member

@taaylor taaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Оставил комменты, если будут вопросы - спрашивай и пингуй в дс

Copy link
Contributor

@ByrDen ByrDen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Выглядит неплохо :)
Старался подробно расписывать, если что-то не понятно, буду рад объяснить в треде таски

Copy link
Member

@taaylor taaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Кайф! У меня усе :)

Copy link
Contributor

@ByrDen ByrDen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Хорошо идём :)

@sipmine sipmine force-pushed the feat/subscription-service branch from 42c024e to 6f05218 Compare October 11, 2025 04:03
Copy link
Contributor

@ByrDen ByrDen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

чуть-чуть нашлось новенького)

Copy link
Contributor

@ByrDen ByrDen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

забыл моментик

@sipmine sipmine force-pushed the feat/subscription-service branch 2 times, most recently from 57f39b9 to 4dc388c Compare October 18, 2025 02:20
Copy link
Contributor

@ByrDen ByrDen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Финишная прямая)

@sipmine sipmine force-pushed the feat/subscription-service branch 2 times, most recently from 05fb9ab to b513496 Compare October 25, 2025 05:16
@sipmine sipmine force-pushed the feat/subscription-service branch from a612b74 to 09635bc Compare November 3, 2025 04:51
Copy link
Contributor

@ByrDen ByrDen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Один тест тебя подвёл, но больше вопросов у меня точно к тебе не будет:)

@sipmine sipmine force-pushed the feat/subscription-service branch from 09635bc to 2866825 Compare November 4, 2025 06:18
@sipmine sipmine force-pushed the feat/subscription-service branch from 2866825 to 9c31fea Compare November 4, 2025 06:24
Copy link
Contributor

@ByrDen ByrDen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Со старыми коментами - всё супер. И парочка последних, которые я мог написать раньше, но не подумал (не заметил этого), сори)
Для консистентности с другими сервисами, есть предложение сделать фикстуру promocode - параметризованной, как это сделано с фикстурой file из сервиса storage_v2, чтобы не делать параметризацию для каждого теста, где у тебя используется promocode

@sipmine sipmine force-pushed the feat/subscription-service branch 2 times, most recently from dfaf1ae to ee36b16 Compare November 5, 2025 13:07
Copy link
Contributor

@ByrDen ByrDen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Всё круто, больше комментов нет)

Copy link
Member

@niqzart niqzart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Для начала так, после этих фиксов можно будет просмотреть и тесты

@sipmine sipmine force-pushed the feat/subscription-service branch from ee36b16 to bec016a Compare November 10, 2025 14:29
@sipmine sipmine force-pushed the feat/subscription-service branch 3 times, most recently from 51e5e18 to 6f3020f Compare November 11, 2025 16:25
@sipmine sipmine force-pushed the feat/subscription-service branch from 6f3020f to 7eaed2d Compare November 11, 2025 16:38
@sipmine sipmine force-pushed the feat/subscription-service branch 2 times, most recently from 59243bd to c1a8ac5 Compare November 12, 2025 12:49
@niqzart niqzart removed the ci:deployable If deployment of the PR should be attempted label Nov 12, 2025
@niqzart niqzart force-pushed the feat/subscription-service branch from c1a8ac5 to 6d72584 Compare November 12, 2025 14:10
Copy link
Member

@niqzart niqzart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В тестах тоже много осталось

Comment on lines 22 to 26
@pytest.fixture()
def promocode_id(promocode: Promocode) -> int:
return promocode.id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Неконсистентно с другими частями проекта, лишняя фикстура

Comment on lines 31 to 32

class InvalidPromocodePeriodInputFactory(BaseModelFactory[PromocodeInputSchema]):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

polish: "период с невалидный промокодом". Правильно: "промокод с невалидным периодом", т.е. InvalidPeriodPromocode, а не наоборот

Comment on lines 25 to 26
**PromocodeInputFactory.build_python(code=faker.pystr(max_chars=4 + i))
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: max_chars не гарантирует защиту от коллиции, т.к. может сгенерить и меньше, а там и совпасть просто

Comment on lines 14 to 15

class PromocodeInputFactory(BaseModelFactory[PromocodeInputSchema]):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

polish: не логично, что одна просто, другая — unlimited. Значит первая — limited

Comment on lines 197 to 198
@freeze_time()
async def test_promocode_requesting_exist_code(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: невалидный нейминг

issue: бесполезная заморозка времени

Comment on lines 206 to 178
async with active_session():
existing_promocode: Promocode = await Promocode.create(**promocode_put_data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: для такого существуют фикстуры

Comment on lines 176 to 168
@pytest.mark.parametrize(
("method", "path", "body_factory"),
[
pytest.param("POST", "", PromocodeInputFactory, id="create_promocode"),
pytest.param(
"POST", "", UnlimitedPromocodeInputFactory, id="create_unlimited_promocode"
),
pytest.param(
"PUT",
lfc(lambda promocode_id: f"{promocode_id}/"),
PromocodeInputFactory,
id="update_promocode",
),
pytest.param(
"PUT",
lfc(lambda promocode_id: f"{promocode_id}/"),
UnlimitedPromocodeInputFactory,
id="update_unlimited_promocode",
),
],
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: период никак не влияет на ошибку коллизии кода

issue: переусложнённая параметризация. Куда проще сделать два теста

Comment on lines +223 to +205
@pytest.mark.parametrize(
("path", "deleted_field"),
[
pytest.param("by-id", lf("deleted_promocode_id"), id="promocode_by_id"),
pytest.param("by-code", lf("deleted_promocode_code"), id="promocode_by_code"),
],
)
async def test_promocode_retrieving_not_finding(
mub_client: TestClient, path: str, deleted_field: str
) -> None:
assert_response(
mub_client.get(
f"/mub/subscription-service/promocodes/{path}/{deleted_field}/",
),
expected_code=status.HTTP_404_NOT_FOUND,
expected_json={"detail": "Promocode not found"},
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: невалидное название теста, некорректный нейминг

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: неконсистентный порядок тестов

@sipmine sipmine force-pushed the feat/subscription-service branch from 6d72584 to e783caf Compare December 6, 2025 05:49
@sipmine sipmine force-pushed the feat/subscription-service branch from e783caf to 1ee4ca9 Compare December 29, 2025 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:covered If coverage checks should be run on the PR ci:migrated If migration checks should be run on the PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants