Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR moves account billing Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
feat: add new post-paid-types
AndrewHanasiro
force-pushed
the
feature/new-post-paid-types
branch
from
e6dc943 to
8bbdd19
Compare
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/factory/helpers.py (1)
19-70: Avoid partial commits + don’t overwritetype_during unpack.Right now
create_account()commits theaccount_tableinsert before thesubscription_tableinsert; if the second insert fails, tests can leak orphaned accounts into the DB and cause follow-on flakiness. Also, unpacking the subscription row intotype_overwrites the input parameter.Proposed fix (single transaction + stable created_at + clearer names)
def create_account( session: Session, external_id: UUID, type_: AccountType, created_at: Optional[datetime.datetime] = None, ): + created_at_value = created_at if created_at is not None else datetime.datetime.now() - account_insert_line = ( - insert(account_table) - .values( - external_id=external_id, - created_at=( - created_at if created_at is not None else datetime.datetime.now() - ), - ) - .returning( - account_table.c.id, account_table.c.created_at, account_table.c.deleted_at - ) - ) - cursor_user = session.execute(account_insert_line).first() - session.commit() - if cursor_user is None: - raise SystemError("test: create_account something went wrong") - (account_id_, created_at_, deleted_at_) = deepcopy(cursor_user) - subscription_insert_line = ( - insert(subscription_table) - .values( - account_id=account_id_, - type=type_, - created_at=( - created_at if created_at is not None else datetime.datetime.now() - ), - ) - .returning( - subscription_table.c.id, - subscription_table.c.type, - subscription_table.c.created_at, - subscription_table.c.deleted_at, - ) - ) - cursor_subscription = session.execute(subscription_insert_line).first() - if cursor_subscription is None: - raise SystemError("test: create_account something went wrong") - (subscription_id, type_, subscription_created_at_, subscription_deleted_at_) = ( - deepcopy(cursor_subscription) - ) - session.commit() + with session.begin(): + account_insert_line = ( + insert(account_table) + .values(external_id=external_id, created_at=created_at_value) + .returning( + account_table.c.id, + account_table.c.created_at, + account_table.c.deleted_at, + ) + ) + cursor_user = session.execute(account_insert_line).first() + if cursor_user is None: + raise AssertionError("test: create_account insert(account_table) returned no row") + (account_id_, created_at_, deleted_at_) = deepcopy(cursor_user) + + subscription_insert_line = ( + insert(subscription_table) + .values(account_id=account_id_, type=type_, created_at=created_at_value) + .returning( + subscription_table.c.id, + subscription_table.c.type, + subscription_table.c.created_at, + subscription_table.c.deleted_at, + ) + ) + cursor_subscription = session.execute(subscription_insert_line).first() + if cursor_subscription is None: + raise AssertionError("test: create_account insert(subscription_table) returned no row") + ( + subscription_id, + subscription_type, + subscription_created_at_, + subscription_deleted_at_, + ) = deepcopy(cursor_subscription) subscription = Subscription( - subscription_id, type_, subscription_created_at_, subscription_deleted_at_ + subscription_id, + subscription_type, + subscription_created_at_, + subscription_deleted_at_, ) return Account(account_id_, subscription, external_id, created_at_, deleted_at_)tests/core/usecase/test_receive_credit.py (1)
7-28: Renamefetch_by_account_idtofetch_by_external_idfor clarity.The method in
BillingFetchUser(and its implementation inBillingService) is misnamed. It acceptsexternal_idas a parameter and uses it directly in the API request (/user/{external_id}), yet the method name suggests it takes anaccount_id. All call sites correctly passexternal_id, confirming the method signature is right but the name is misleading. Rename the method throughout the codebase to match its actual behavior.
🤖 Fix all issues with AI agents
In @db/migrations/20260107214140_update-subscription-account.sql:
- Around line 1-2: The migration drops the account.type column without migrating
existing values, causing data loss; update the migrate:up block to first INSERT
existing account.type rows into the subscription table (e.g., INSERT INTO
subscription (account_id, type, created_at, updated_at) SELECT id, type,
CURRENT_TIMESTAMP, CURRENT_TIMESTAMP FROM account WHERE type IS NOT NULL) then
run ALTER TABLE "account" DROP COLUMN "type"; ensure the INSERT matches the
actual subscription schema and satisfies constraints (defaults/nullability, FK
to account.id) and handle duplicates or conflicts as needed.
In @src/config/database.py:
- Line 5: The create_engine call currently sets echo=True unconditionally
(engine = create_engine(EnvVars.DATABASE_HOST, echo=True)), which can leak
sensitive SQL; change this to read a configuration flag (e.g., EnvVars.SQL_ECHO
or EnvVars.DEBUG) and pass a boolean to echo so SQL logging is configurable and
defaults to False in production; ensure the flag is parsed as a boolean and used
in the create_engine(...) call for engine.
In @src/core/repository/account.py:
- Around line 68-76: The code unpacks subscription fields from the wrong
variable: after executing query_subscription_insert you must use
subscription_row.first() (or the appropriate fetch method) instead of
account_row; replace the line that assigns (subscription_id,
subscription_created_at) = deepcopy(account_row) with extracting values from
subscription_row (e.g., row = subscription_row.first(); if row is None raise;
then assign subscription_id and subscription_created_at from that row) so
query_subscription_insert, subscription_row, subscription_id and
subscription_created_at reflect the inserted subscription row.
- Line 5: Remove the unused import "engine" from the top of the module (the line
"from src.config.database import engine") in src/core/repository/account.py;
delete that import so only required imports remain, verify no code references
the symbol "engine" in functions or classes in this file (e.g., any repository
functions), and run linters/tests to confirm the unused-import warning is
resolved.
- Around line 221-236: The loop currently pulls a SQLAlchemy Row from
subscription_list into the variable subscription and passes it to the Account
constructor, but Account expects a Subscription dataclass; convert the Row into
a Subscription dataclass before constructing Account. Locate the filter that
assigns subscription (variable subscription, subscription_list) and replace the
raw Row with a dataclass instance by extracting the Row's values (e.g. via
row._mapping or row._asdict()) and calling Subscription(...) with the matching
fields, or add a small helper like subscription_from_row(row) to perform this
mapping; then pass that Subscription instance into Account(...) instead of the
Row.
- Around line 209-236: The code incorrectly references account.id when building
the Account object even though account was unpacked into account_id,
external_id, account_created_at, account_deleted_at; change that constructor
call to use account_id (i.e., pass account_id instead of account.id) in the
Account(...) instantiation so the correct ID variable is used.
In @src/core/usecase/receive_credit.py:
- Around line 34-35: The current check only rejects AccountType.POST_PAID_MONTH
in receive_credit but must block all post-paid variants; update the validation
in the receive_credit use case to either check for all post-paid enums
(AccountType.POST_PAID_MONTH, AccountType.POST_PAID_SEMESTER,
AccountType.POST_PAID_ANNUAL) and raise FlowPrePaidError, or invert the logic to
allow only pre-paid types (e.g., check subscription.type is a PRE_PAID enum) and
raise FlowPrePaidError otherwise so no post-paid account can receive credit.
In @src/core/usecase/receive_event.py:
- Around line 43-45: The billing check in receive_event.py only bills when
account.subscription.type == AccountType.POST_PAID_MONTH, so POST_PAID_SEMESTER
and POST_PAID_ANNUAL are skipped; update the condition around
account.subscription.type to include all post-paid types (e.g., check membership
in (AccountType.POST_PAID_MONTH, AccountType.POST_PAID_SEMESTER,
AccountType.POST_PAID_ANNUAL) or add/call an is_post_paid() helper) and keep the
existing call to self.billing_updating_invoice.add_item(external_id, item_list)
unchanged so all post-paid subscriptions are billed on event receipt.
In @src/core/usecase/transform_to_post_paid.py:
- Around line 34-35: The current guard in transform_to_post_paid.py only checks
account.subscription.type against AccountType.POST_PAID_MONTH; update the
validation to treat all post-paid variants as already post-paid by either
checking membership against the set {AccountType.POST_PAID_MONTH,
AccountType.POST_PAID_SEMESTER, AccountType.POST_PAID_ANNUAL} or by using a
predicate (e.g., string/prefix check) that detects any POST_PAID variant, and
raise the same SystemError when account.subscription.type matches any of those
values (referencing account.subscription.type and the AccountType enum values).
In @src/core/usecase/transform_to_pre_paid.py:
- Around line 38-39: The condition is inverted: change the check in the
transform-to-pre-paid logic so it raises when the account is already PRE_PAID
instead of when it is POST_PAID_MONTH; replace the current check of
account.subscription.type is AccountType.POST_PAID_MONTH with
account.subscription.type is AccountType.PRE_PAID (or equivalent enum value) and
update the error message to something like "This account already is PrePaid" in
the same raise SystemError call within the transform_to_pre_paid use case.
In @tests/core/usecase/test_transform_to_pre_paid.py:
- Around line 29-30: The test creates a Subscription with AccountType.PRE_PAID
but is meant to validate transformation to pre-paid; update the Subscription
instantiation so it starts as a post-paid type (e.g., use
AccountType.POST_PAID_MONTH or AccountType.POST_PAID) when creating the
Subscription passed into Account (symbols: Subscription, Account, AccountType)
so the test exercises the transformation from post-paid to pre-paid.
In @tests/factory/helpers.py:
- Around line 40-64: The test helper currently raises SystemError when database
fetches fail (see cursor_user and cursor_subscription in the create_account
helper), which is misleading and triggers lint rules; replace these with
assertions or AssertionError (e.g., assert cursor_user is not None, assert
cursor_subscription is not None or raise AssertionError with a clear message) so
failures are reported as test failures rather than system errors; update both
the cursor_user and cursor_subscription checks and keep the same error
messages/content for clarity.
In @tests/presentation/test_worker.py:
- Around line 21-23: The bug is that one_month_ago is assigned twice,
overwriting the month adjustment; compute the target month and year first (e.g.,
prev_month = now.month - 1 if now.month > 1 else 12 and prev_year = now.year if
now.month > 1 else now.year - 1) and then set one_month_ago =
now.replace(month=prev_month, year=prev_year) so both adjustments are applied in
a single replace call using the existing now and one_month_ago variable names.
🧹 Nitpick comments (6)
tests/presentation/test_server.py (1)
40-44: Consider verifying subscription data.The test correctly unpacks only account table columns now that
typehas been moved to the subscription table. However, the test doesn't verify that a subscription record was created. Consider adding a query tosubscription_tableto validate the complete account+subscription creation.💡 Suggested enhancement
# After line 44, add: from src.core.repository.account import subscription_table # Query and verify subscription subscription_query = ( select(subscription_table) .where(subscription_table.c.account_id == id_) .limit(1) ) subscription_cursor = session.execute(subscription_query).first() assert subscription_cursor is not None (sub_id, account_id, sub_type, sub_created_at, sub_deleted_at) = deepcopy(subscription_cursor) assert sub_type.value == "PRE_PAID" assert sub_deleted_at is Nonesrc/core/usecase/transform_to_post_paid.py (1)
38-38: Consider parameterizing the target post-paid type.The transformation hardcodes
POST_PAID_MONTHas the target. With multiple post-paid variants now available (MONTH, SEMESTER, ANNUAL), consider accepting the target type as a parameter to allow users to choose their desired post-paid plan.♻️ Suggested enhancement
- def transform_to_post_paid(self, external_id: UUID): + def transform_to_post_paid(self, external_id: UUID, target_type: AccountType = AccountType.POST_PAID_MONTH): account = self.reading_account.by_external_id(external_id) if account.subscription.type != AccountType.PRE_PAID: raise SystemError("This account already is PostPaid") total_credit = self._calculate_total_credit(account) self._should_create_discount(account.id, total_credit) - self.update_account.change_type(account.id, AccountType.POST_PAID_MONTH) + self.update_account.change_type(account.id, target_type)tests/core/usecase/test_receive_event.py (1)
7-30: Make test time + constructors more deterministic/explicit.The model update looks consistent, but the test currently mixes multiple
datetime.now()/datetime.today()calls and uses positional args forSubscription(...)/Account(...), which makes failures harder to reason about.Proposed tweak (single timestamp + keyword args)
def test_should_receive_event(session: Session): + now = datetime.now() account_id = uuid4() external_id = uuid4() - subscription = Subscription( - uuid4(), AccountType.POST_PAID_MONTH, datetime.now(), None - ) - account = Account(account_id, subscription, external_id, datetime.now(), None) + subscription = Subscription( + id=uuid4(), + type=AccountType.POST_PAID_MONTH, + created_at=now, + deleted_at=None, + ) + account = Account( + id=account_id, + subscription=subscription, + external_id=external_id, + created_at=now, + deleted_at=None, + )tests/core/usecase/test_account_create.py (1)
7-30: Simplify assertion to compare enums directly.The updated test matches the new
Account(subscription=...)model. Since both sides areAccountType, you can compare enums directly rather than.value.Proposed change
- assert result.subscription.type.value == type_.value + assert result.subscription.type == type_tests/core/repository/test_account.py (2)
60-67: LGTM! Test correctly queries account table after type change.The test properly verifies the account row after calling
change_type, using the newPOST_PAID_MONTHsubscription variant.Optional: Consider extracting error message to a constant.
Static analysis suggests avoiding long messages in exception constructors. You could optionally extract the error message to a constant for better maintainability, though this is a minor style consideration.
68-86: LGTM! Subscription query and validation correctly implemented.The test properly validates the refactored subscription model by:
- Querying the subscription table for the active (non-deleted) subscription
- Verifying both account and subscription row integrity
- Confirming the subscription type was updated to
POST_PAID_MONTH- Ensuring the subscription is not marked as deleted
Optional: Consider extracting error message to a constant.
Similar to Line 66, static analysis suggests avoiding long messages in exception constructors (Line 78). Extracting error messages to constants could improve maintainability, though this is a minor style consideration.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
README.mddb/migrations/20260107214140_update-subscription-account.sqlmakefilepyproject.tomlsrc/config/database.pysrc/core/entity/account.pysrc/core/repository/account.pysrc/core/usecase/charge_debit.pysrc/core/usecase/receive_credit.pysrc/core/usecase/receive_event.pysrc/core/usecase/transform_to_post_paid.pysrc/core/usecase/transform_to_pre_paid.pytests/core/repository/test_account.pytests/core/usecase/test_account_create.pytests/core/usecase/test_charge_debit.pytests/core/usecase/test_get_total_credit.pytests/core/usecase/test_receive_credit.pytests/core/usecase/test_receive_event.pytests/core/usecase/test_transform_to_post_paid.pytests/core/usecase/test_transform_to_pre_paid.pytests/factory/helpers.pytests/presentation/test_server.pytests/presentation/test_worker.py
🧰 Additional context used
🧬 Code graph analysis (17)
src/core/usecase/receive_event.py (1)
src/core/entity/account.py (1)
AccountType(8-12)
tests/core/repository/test_account.py (3)
src/core/repository/account.py (3)
AccountRepository(54-236)by_external_id(123-160)change_type(162-173)tests/factory/helpers.py (2)
delete_account(72-79)create_account(20-69)src/core/entity/account.py (1)
AccountType(8-12)
src/core/usecase/transform_to_pre_paid.py (1)
src/core/entity/account.py (1)
AccountType(8-12)
src/core/usecase/charge_debit.py (1)
src/core/entity/account.py (1)
AccountType(8-12)
tests/presentation/test_worker.py (3)
tests/factory/helpers.py (1)
create_account(20-69)tests/conftest.py (1)
session(24-26)src/core/entity/account.py (1)
AccountType(8-12)
src/core/usecase/receive_credit.py (1)
src/core/entity/account.py (1)
AccountType(8-12)
tests/core/usecase/test_get_total_credit.py (1)
src/core/entity/account.py (3)
Account(24-29)AccountType(8-12)Subscription(16-20)
src/config/database.py (1)
src/config/envvar.py (1)
EnvVars(8-14)
src/core/usecase/transform_to_post_paid.py (3)
src/core/entity/account.py (1)
AccountType(8-12)src/core/repository/account.py (1)
change_type(162-173)src/core/usecase/driven/update_account.py (1)
change_type(9-10)
tests/core/usecase/test_account_create.py (5)
src/core/entity/account.py (3)
Account(24-29)AccountType(8-12)Subscription(16-20)src/core/usecase/driven/creating_account.py (1)
CreatingAccount(7-10)src/core/repository/account.py (1)
create(58-81)tests/conftest.py (1)
session(24-26)src/core/usecase/account_create.py (1)
AccountCreate(7-16)
tests/core/usecase/test_receive_event.py (1)
src/core/entity/account.py (3)
Account(24-29)AccountType(8-12)Subscription(16-20)
tests/core/usecase/test_transform_to_post_paid.py (1)
src/core/entity/account.py (3)
Account(24-29)AccountType(8-12)Subscription(16-20)
src/core/repository/account.py (4)
src/core/entity/account.py (3)
Account(24-29)AccountType(8-12)Subscription(16-20)tests/conftest.py (1)
session(24-26)src/core/usecase/driven/reading_account.py (4)
by_id(14-15)AccountNotFoundException(8-9)by_external_id(18-19)by_subscription_period(22-23)src/core/usecase/driven/update_account.py (1)
change_type(9-10)
tests/core/usecase/test_receive_credit.py (1)
src/core/entity/account.py (3)
Account(24-29)AccountType(8-12)Subscription(16-20)
tests/core/usecase/test_transform_to_pre_paid.py (1)
src/core/entity/account.py (3)
Account(24-29)AccountType(8-12)Subscription(16-20)
tests/core/usecase/test_charge_debit.py (1)
src/core/entity/account.py (3)
Account(24-29)AccountType(8-12)Subscription(16-20)
tests/factory/helpers.py (2)
src/core/entity/account.py (3)
Account(24-29)AccountType(8-12)Subscription(16-20)tests/conftest.py (1)
session(24-26)
🪛 GitHub Actions: Continuous Integration on Pull Request
src/core/repository/account.py
[error] 5-5: F401 'src.config.database.engine' imported but unused
🪛 Ruff (0.14.10)
tests/core/repository/test_account.py
66-66: Avoid specifying long messages outside the exception class
(TRY003)
78-78: Avoid specifying long messages outside the exception class
(TRY003)
src/core/usecase/transform_to_post_paid.py
35-35: Avoid specifying long messages outside the exception class
(TRY003)
src/core/repository/account.py
66-66: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Avoid specifying long messages outside the exception class
(TRY003)
100-100: Avoid specifying long messages outside the exception class
(TRY003)
139-139: Avoid specifying long messages outside the exception class
(TRY003)
tests/factory/helpers.py
61-61: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (21)
makefile (1)
27-29: LGTM! Good addition of code quality checks for tests.Extending code quality tools (black, flake8, isort) to the
tests/directory ensures consistency and maintainability across both source and test code.README.md (1)
79-79: LGTM! Documentation improvements align with PR changes.The updated test reference and expanded dev hints provide valuable guidance for developers working with the codebase.
Also applies to: 89-123
db/migrations/20260107214140_update-subscription-account.sql (1)
1-6: All references to account.type have been successfully migrated.The Account entity no longer has a direct
typefield, and the account table schema has been updated accordingly. All code now correctly accesses the type throughaccount.subscription.type, and repository queries properly join the subscription table. The migration is complete and verified.pyproject.toml (1)
15-16: All specified dependency versions are valid and currently available on PyPI:
- fastapi 0.128.0 (released Dec 27, 2025)
- uvicorn 0.40.0 (no known security vulnerabilities)
- coverage 7.13.1
- mypy 1.19.1 (released Dec 15, 2025)
- types-requests 2.32.4.20260107 (valid PEP 440 format, uploaded Jan 7, 2026)
No known security advisories exist for any of these versions.
src/core/entity/account.py (2)
10-20: LGTM! Clean expansion of subscription types.The refactoring from a single
POST_PAIDtype to three granular subscription periods (MONTH, SEMESTER, ANNUAL) and the introduction of theSubscriptiondataclass provide better modeling of subscription billing cycles.
26-26: LGTM! Improved separation of concerns.Moving the type information into a nested
Subscriptionobject provides better encapsulation and allows for tracking subscription-specific metadata (creation, deletion timestamps) independently from the account.src/core/repository/account.py (3)
84-121: LGTM! Proper join with subscription table.The method correctly joins
account_tablewithsubscription_tableand filters out deleted subscriptions, ensuring only active subscription data is returned.
123-160: LGTM! Consistent implementation.This method mirrors the
by_idimplementation correctly, joining with the subscription table and constructing the nestedSubscriptionobject.
162-173: LGTM! Proper subscription history management.The method correctly soft-deletes the existing subscription and creates a new one, maintaining a complete audit trail of subscription type changes.
src/core/usecase/charge_debit.py (1)
37-38: LGTM! Correct adaptation to nested subscription model.The prepaid check correctly accesses the subscription type through the nested
subscriptionobject.tests/presentation/test_worker.py (1)
25-27: LGTM: Account creation properly updated to use POST_PAID_MONTH.The test correctly creates an account with the new
AccountType.POST_PAID_MONTHvariant.tests/core/usecase/test_charge_debit.py (1)
7-30: LGTM: test updated cleanly forAccount(subscription=...).The Subscription construction and updated
Account(...)call align with the new model and keep the test intent intact.tests/core/usecase/test_transform_to_post_paid.py (3)
7-7: LGTM! Import updated correctly.The
Subscriptionimport is required for the refactored Account constructor pattern.
24-25: LGTM! Account and Subscription construction updated correctly.The test correctly constructs a
Subscriptioninstance and passes it to theAccountconstructor, aligning with the refactored data model.
64-64: LGTM! Assertion updated to new AccountType.The expected type
POST_PAID_MONTHcorrectly reflects the expanded subscription variants introduced in this refactoring.tests/core/usecase/test_get_total_credit.py (2)
7-7: LGTM! Import updated correctly.The
Subscriptionimport is required for constructing test accounts with the new data model.
20-21: LGTM! Account construction updated correctly.The test correctly constructs a
Subscriptionand passes it to theAccountconstructor, consistent with the refactored entity model.tests/core/repository/test_account.py (4)
9-13: LGTM! Import updated to include subscription_table.The
subscription_tableimport is required for the refactoredtest_should_update_typetest that now queries the subscription table directly.
25-25: LGTM! Assertion updated to access nested subscription.The assertion correctly accesses
result.subscription.typeto validate the account's subscription type in the refactored model.
37-37: LGTM! Assertion updated to access nested subscription.The assertion correctly accesses
result.subscription.typefor comparison with the expected subscription type.
45-45: LGTM! Test updated to use new subscription variant.The test correctly uses
POST_PAID_MONTH(one of the new subscription variants) and accesses the subscription type via the nestedsubscription.typefield.Also applies to: 50-50
| -- migrate:up | ||
| ALTER TABLE "account" DROP COLUMN "type"; |
There was a problem hiding this comment.
Missing data migration before dropping column.
The migration drops the account.type column without first migrating existing data to the new subscription table structure. This will result in permanent data loss if existing accounts have type information.
📋 Recommended approach
Before dropping the column, add a data migration step:
-- migrate:up
-- First, migrate existing account type data to subscription table
INSERT INTO subscription (account_id, type, created_at, updated_at)
SELECT id, type, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP
FROM account
WHERE type IS NOT NULL;
-- Then drop the column
ALTER TABLE "account" DROP COLUMN "type";Note: Adjust the INSERT statement based on your actual subscription table schema and constraints.
🤖 Prompt for AI Agents
In @db/migrations/20260107214140_update-subscription-account.sql around lines 1
- 2, The migration drops the account.type column without migrating existing
values, causing data loss; update the migrate:up block to first INSERT existing
account.type rows into the subscription table (e.g., INSERT INTO subscription
(account_id, type, created_at, updated_at) SELECT id, type, CURRENT_TIMESTAMP,
CURRENT_TIMESTAMP FROM account WHERE type IS NOT NULL) then run ALTER TABLE
"account" DROP COLUMN "type"; ensure the INSERT matches the actual subscription
schema and satisfies constraints (defaults/nullability, FK to account.id) and
handle duplicates or conflicts as needed.
| -- migrate:down | ||
|
|
||
| ALTER TABLE "account" ADD "type" VARCHAR(20) NOT NULL No newline at end of file |
There was a problem hiding this comment.
Incomplete rollback: Data cannot be restored.
The rollback adds the type column back with a NOT NULL constraint, but doesn't restore the original data. This will either fail (if there are existing accounts) or result in data loss.
♻️ Proposed fix for rollback
-- migrate:down
-ALTER TABLE "account" ADD "type" VARCHAR(20) NOT NULL
+-- Restore the type column (nullable first to allow data migration)
+ALTER TABLE "account" ADD "type" VARCHAR(20);
+
+-- Migrate data back from subscription table
+UPDATE account
+SET type = subscription.type
+FROM subscription
+WHERE account.id = subscription.account_id;
+
+-- Make NOT NULL after data is restored
+ALTER TABLE "account" ALTER COLUMN "type" SET NOT NULL;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -- migrate:down | |
| ALTER TABLE "account" ADD "type" VARCHAR(20) NOT NULL | |
| -- migrate:down | |
| -- Restore the type column (nullable first to allow data migration) | |
| ALTER TABLE "account" ADD "type" VARCHAR(20); | |
| -- Migrate data back from subscription table | |
| UPDATE account | |
| SET type = subscription.type | |
| FROM subscription | |
| WHERE account.id = subscription.account_id; | |
| -- Make NOT NULL after data is restored | |
| ALTER TABLE "account" ALTER COLUMN "type" SET NOT NULL; |
| from src.config.envvar import EnvVars | ||
|
|
||
| engine = create_engine(EnvVars.DATABASE_HOST, echo=False) | ||
| engine = create_engine(EnvVars.DATABASE_HOST, echo=True) |
There was a problem hiding this comment.
Hardcoded SQL logging may leak sensitive data in production.
Setting echo=True unconditionally logs all SQL statements, which can expose PII (user identifiers, emails) in queries and potentially sensitive connection details. This creates compliance risks (GDPR/CCPA) and performance overhead.
🔒 Proposed fix: Make SQL logging configurable
+import os
from sqlalchemy import create_engine
from src.config.envvar import EnvVars
-engine = create_engine(EnvVars.DATABASE_HOST, echo=True)
+# Enable SQL echo only in development environments
+enable_echo = os.getenv("PYTHON_ENV", "production") == "development"
+engine = create_engine(EnvVars.DATABASE_HOST, echo=enable_echo)🤖 Prompt for AI Agents
In @src/config/database.py at line 5, The create_engine call currently sets
echo=True unconditionally (engine = create_engine(EnvVars.DATABASE_HOST,
echo=True)), which can leak sensitive SQL; change this to read a configuration
flag (e.g., EnvVars.SQL_ECHO or EnvVars.DEBUG) and pass a boolean to echo so SQL
logging is configurable and defaults to False in production; ensure the flag is
parsed as a boolean and used in the create_engine(...) call for engine.
| query_subscription_insert = ( | ||
| insert(subscription_table) | ||
| .values(account_id=account_id, type=type_) | ||
| .returning(subscription_table.c.id, subscription_table.c.created_at) | ||
| ) | ||
| self.session.execute(query_subscription_insert) | ||
| subscription_row = self.session.execute(query_subscription_insert) | ||
| if subscription_row is None: | ||
| raise SystemError("Something on database did not return") | ||
| (subscription_id, subscription_created_at) = deepcopy(account_row) |
There was a problem hiding this comment.
Critical bug: Wrong variable used to unpack subscription data.
Line 76 uses account_row instead of subscription_row.first(), causing the subscription to be constructed with account data instead of subscription data. This will result in incorrect subscription_id and subscription_created_at values.
🐛 Proposed fix
query_subscription_insert = (
insert(subscription_table)
.values(account_id=account_id, type=type_)
.returning(subscription_table.c.id, subscription_table.c.created_at)
)
- subscription_row = self.session.execute(query_subscription_insert)
+ subscription_row = self.session.execute(query_subscription_insert).first()
if subscription_row is None:
raise SystemError("Something on database did not return")
- (subscription_id, subscription_created_at) = deepcopy(account_row)
+ (subscription_id, subscription_created_at) = deepcopy(subscription_row)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| query_subscription_insert = ( | |
| insert(subscription_table) | |
| .values(account_id=account_id, type=type_) | |
| .returning(subscription_table.c.id, subscription_table.c.created_at) | |
| ) | |
| self.session.execute(query_subscription_insert) | |
| subscription_row = self.session.execute(query_subscription_insert) | |
| if subscription_row is None: | |
| raise SystemError("Something on database did not return") | |
| (subscription_id, subscription_created_at) = deepcopy(account_row) | |
| query_subscription_insert = ( | |
| insert(subscription_table) | |
| .values(account_id=account_id, type=type_) | |
| .returning(subscription_table.c.id, subscription_table.c.created_at) | |
| ) | |
| subscription_row = self.session.execute(query_subscription_insert).first() | |
| if subscription_row is None: | |
| raise SystemError("Something on database did not return") | |
| (subscription_id, subscription_created_at) = deepcopy(subscription_row) |
🧰 Tools
🪛 Ruff (0.14.10)
75-75: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @src/core/repository/account.py around lines 68 - 76, The code unpacks
subscription fields from the wrong variable: after executing
query_subscription_insert you must use subscription_row.first() (or the
appropriate fetch method) instead of account_row; replace the line that assigns
(subscription_id, subscription_created_at) = deepcopy(account_row) with
extracting values from subscription_row (e.g., row = subscription_row.first();
if row is None raise; then assign subscription_id and subscription_created_at
from that row) so query_subscription_insert, subscription_row, subscription_id
and subscription_created_at reflect the inserted subscription row.
| if account.subscription.type is AccountType.POST_PAID_MONTH: | ||
| raise SystemError("This account already is PostPaid") |
There was a problem hiding this comment.
Incomplete post-paid validation: Only checks POST_PAID_MONTH.
The guard condition only checks for POST_PAID_MONTH, but accounts could already be POST_PAID_SEMESTER or POST_PAID_ANNUAL. These should also raise the "already is PostPaid" error.
🔧 Proposed fix
- if account.subscription.type is AccountType.POST_PAID_MONTH:
+ if account.subscription.type != AccountType.PRE_PAID:
raise SystemError("This account already is PostPaid")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if account.subscription.type is AccountType.POST_PAID_MONTH: | |
| raise SystemError("This account already is PostPaid") | |
| if account.subscription.type != AccountType.PRE_PAID: | |
| raise SystemError("This account already is PostPaid") |
🧰 Tools
🪛 Ruff (0.14.10)
35-35: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @src/core/usecase/transform_to_post_paid.py around lines 34 - 35, The current
guard in transform_to_post_paid.py only checks account.subscription.type against
AccountType.POST_PAID_MONTH; update the validation to treat all post-paid
variants as already post-paid by either checking membership against the set
{AccountType.POST_PAID_MONTH, AccountType.POST_PAID_SEMESTER,
AccountType.POST_PAID_ANNUAL} or by using a predicate (e.g., string/prefix
check) that detects any POST_PAID variant, and raise the same SystemError when
account.subscription.type matches any of those values (referencing
account.subscription.type and the AccountType enum values).
| if account.subscription.type is AccountType.POST_PAID_MONTH: | ||
| raise SystemError("This account already is PostPaid") |
There was a problem hiding this comment.
Critical logic error: Condition and error message are inverted.
The condition checks if the account IS POST_PAID_MONTH and raises an error saying "already is PostPaid". However, this use case transforms post-paid accounts TO pre-paid (per the docstring on line 18), so it EXPECTS post-paid accounts. The logic should instead check if the account is ALREADY PRE_PAID and raise an error.
🐛 Proposed fix
- if account.subscription.type is AccountType.POST_PAID_MONTH:
- raise SystemError("This account already is PostPaid")
+ if account.subscription.type == AccountType.PRE_PAID:
+ raise SystemError("This account is already PrePaid")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if account.subscription.type is AccountType.POST_PAID_MONTH: | |
| raise SystemError("This account already is PostPaid") | |
| if account.subscription.type == AccountType.PRE_PAID: | |
| raise SystemError("This account is already PrePaid") |
🧰 Tools
🪛 Ruff (0.14.10)
39-39: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @src/core/usecase/transform_to_pre_paid.py around lines 38 - 39, The
condition is inverted: change the check in the transform-to-pre-paid logic so it
raises when the account is already PRE_PAID instead of when it is
POST_PAID_MONTH; replace the current check of account.subscription.type is
AccountType.POST_PAID_MONTH with account.subscription.type is
AccountType.PRE_PAID (or equivalent enum value) and update the error message to
something like "This account already is PrePaid" in the same raise SystemError
call within the transform_to_pre_paid use case.
| subscription = Subscription(uuid4(), AccountType.PRE_PAID, datetime.now(), None) | ||
| account = Account(account_id, subscription, external_id, account_created_at, None) |
There was a problem hiding this comment.
Critical test data error: Account should start as POST_PAID, not PRE_PAID.
The test creates an account with AccountType.PRE_PAID, but this test validates transformation TO pre-paid. The account should start with a post-paid type (e.g., AccountType.POST_PAID_MONTH) to properly test the transformation logic.
🐛 Proposed fix
- subscription = Subscription(uuid4(), AccountType.PRE_PAID, datetime.now(), None)
+ subscription = Subscription(uuid4(), AccountType.POST_PAID_MONTH, datetime.now(), None)
account = Account(account_id, subscription, external_id, account_created_at, None)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| subscription = Subscription(uuid4(), AccountType.PRE_PAID, datetime.now(), None) | |
| account = Account(account_id, subscription, external_id, account_created_at, None) | |
| subscription = Subscription(uuid4(), AccountType.POST_PAID_MONTH, datetime.now(), None) | |
| account = Account(account_id, subscription, external_id, account_created_at, None) |
🤖 Prompt for AI Agents
In @tests/core/usecase/test_transform_to_pre_paid.py around lines 29 - 30, The
test creates a Subscription with AccountType.PRE_PAID but is meant to validate
transformation to pre-paid; update the Subscription instantiation so it starts
as a post-paid type (e.g., use AccountType.POST_PAID_MONTH or
AccountType.POST_PAID) when creating the Subscription passed into Account
(symbols: Subscription, Account, AccountType) so the test exercises the
transformation from post-paid to pre-paid.
| if cursor_user is None: | ||
| raise SystemError("test: create_account something went wrong") | ||
| (id_, created_at_, deleted_at_) = deepcopy(cursor_user) | ||
| subscription_insert_line = insert(subscription_table).values( | ||
| account_id=id_, type=type_ | ||
| (account_id_, created_at_, deleted_at_) = deepcopy(cursor_user) | ||
| subscription_insert_line = ( | ||
| insert(subscription_table) | ||
| .values( | ||
| account_id=account_id_, | ||
| type=type_, | ||
| created_at=( | ||
| created_at if created_at is not None else datetime.datetime.now() | ||
| ), | ||
| ) | ||
| .returning( | ||
| subscription_table.c.id, | ||
| subscription_table.c.type, | ||
| subscription_table.c.created_at, | ||
| subscription_table.c.deleted_at, | ||
| ) | ||
| ) | ||
| cursor_subscription = session.execute(subscription_insert_line) | ||
| cursor_subscription = session.execute(subscription_insert_line).first() | ||
| if cursor_subscription is None: | ||
| raise SystemError("test: create_account something went wrong") | ||
| (subscription_id, type_, subscription_created_at_, subscription_deleted_at_) = ( | ||
| deepcopy(cursor_subscription) | ||
| ) |
There was a problem hiding this comment.
Prefer AssertionError (or a local test helper exception) over SystemError for test helpers.
This is test-only plumbing; raising SystemError reads like “interpreter/system is broken” and also triggers Ruff TRY003 guidance. A direct assert cursor_user is not None / AssertionError is a better fit.
🧰 Tools
🪛 Ruff (0.14.10)
41-41: Avoid specifying long messages outside the exception class
(TRY003)
61-61: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @tests/factory/helpers.py around lines 40 - 64, The test helper currently
raises SystemError when database fetches fail (see cursor_user and
cursor_subscription in the create_account helper), which is misleading and
triggers lint rules; replace these with assertions or AssertionError (e.g.,
assert cursor_user is not None, assert cursor_subscription is not None or raise
AssertionError with a clear message) so failures are reported as test failures
rather than system errors; update both the cursor_user and cursor_subscription
checks and keep the same error messages/content for clarity.
tests/presentation/test_worker.py
Outdated
| now = datetime.datetime.now() | ||
| one_month_ago = now.replace(month=now.month - 1 if now.month > 1 else 12 ) | ||
| one_month_ago = now.replace(year=now.year if now.month > 1 else now.year -1) |
There was a problem hiding this comment.
Critical bug: Line 23 overwrites the month adjustment from line 22.
The variable one_month_ago is assigned on line 22 with the adjusted month, but line 23 immediately overwrites it with only the year adjustment. This means the month adjustment is lost, and one_month_ago will have the same month as now.
🐛 Proposed fix
now = datetime.datetime.now()
- one_month_ago = now.replace(month=now.month - 1 if now.month > 1 else 12 )
- one_month_ago = now.replace(year=now.year if now.month > 1 else now.year -1)
+ one_month_ago = now.replace(
+ month=now.month - 1 if now.month > 1 else 12,
+ year=now.year if now.month > 1 else now.year - 1
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| now = datetime.datetime.now() | |
| one_month_ago = now.replace(month=now.month - 1 if now.month > 1 else 12 ) | |
| one_month_ago = now.replace(year=now.year if now.month > 1 else now.year -1) | |
| now = datetime.datetime.now() | |
| one_month_ago = now.replace( | |
| month=now.month - 1 if now.month > 1 else 12, | |
| year=now.year if now.month > 1 else now.year - 1 | |
| ) |
🤖 Prompt for AI Agents
In @tests/presentation/test_worker.py around lines 21 - 23, The bug is that
one_month_ago is assigned twice, overwriting the month adjustment; compute the
target month and year first (e.g., prev_month = now.month - 1 if now.month > 1
else 12 and prev_year = now.year if now.month > 1 else now.year - 1) and then
set one_month_ago = now.replace(month=prev_month, year=prev_year) so both
adjustments are applied in a single replace call using the existing now and
one_month_ago variable names.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.