Add uniq account_number error handler #467
Conversation
Default Project
|
||||||||||||||||||||||||||||
| Project |
Default Project
|
| Branch Review |
fix/issue-354
|
| Run status |
|
| Run duration | 00m 40s |
| Commit |
|
| Committer | tgaripov |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
5
|
| View all changes introduced in this branch ↗︎ | |
|
@claude do review |
|
Claude finished @danil-bragin's task —— View job Code Review: Add uniq account_number error handlerTasks
Analysis SummaryThis PR properly addresses issue #354 by implementing comprehensive duplicate account number error handling: ✅ What Works Well:
✅ Additional Improvements Made:
Detailed Review✅ Service Layer (
|
| return s.repo.Count(ctx, params) | ||
| } | ||
|
|
||
| func (s *MoneyAccountService) saveErrTransform(err error) error { |
There was a problem hiding this comment.
remove this just check it on repo layer and throw custom exception
|
@TVolly just add method in repo exists for check on duplicate aftr use this check into service layer on save/create/update ... |
WalkthroughThis PR enhances duplicate account number error handling across the money account feature by introducing explicit error detection, transformation, and localized UI feedback. Mandatory DTO validation for Name and CurrencyCode fields is added, along with dedicated error handling paths that render drawers with localized error messages instead of generic server errors. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/finance/services/money_account_service.go (1)
8-8: Consider repository-layer duplicate check as previously suggested.A past review comment recommended checking for duplicates at the repository layer rather than transforming database errors. The suggested approach would:
- Add a method like
repo.ExistsByAccountNumber(ctx, tenantID, accountNumber)to the repository interface- Check for duplicates before calling
repo.Createorrepo.Update- Return
ErrMoneyAccountDuplicateAccountNumberdirectly from the service without needing pgconn dependencyThis approach is more explicit, follows the repository pattern better, and decouples the service from database implementation details.
Based on past review feedback from danil-bragin.
Also applies to: 16-16
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
modules/finance/presentation/controllers/dtos/money_account_dto.go(1 hunks)modules/finance/presentation/controllers/money_account_controller.go(3 hunks)modules/finance/presentation/locales/en.json(1 hunks)modules/finance/presentation/locales/ru.json(1 hunks)modules/finance/presentation/locales/uz.json(1 hunks)modules/finance/services/money_account_service.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
modules/*/presentation/locales/*.json
📄 CodeRabbit inference engine (CLAUDE.md)
Translation files must be validated using
make check trbefore committing
Files:
modules/finance/presentation/locales/en.jsonmodules/finance/presentation/locales/uz.jsonmodules/finance/presentation/locales/ru.json
**/modules/*/presentation/locales/*.json
📄 CodeRabbit inference engine (AGENTS.md)
Add translations to all locale files in
modules/{module}/presentation/locales/(en.json, ru.json, uz.json) including NavigationLinks, Meta (titles), List, and Single sections
Files:
modules/finance/presentation/locales/en.jsonmodules/finance/presentation/locales/uz.jsonmodules/finance/presentation/locales/ru.json
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Use// TODOcomments for unimplemented parts or future enhancements in Go code
Rungo vet ./...after Go code changes (prefer overgo build)
Import organization must follow Go standards and should be formatted usingmake fix imports
Multi-tenant applications must use organization_id for data isolation in all queries and database operations
Use Cookie-based sessions with RBAC for authentication and authorization across all Go controllers and services
**/*.go: Error handling: usepkg/serrorsfor standard error types in Go code
When writing a mapper function in Go, always use utilities frompkg/mappingto ensure consistency
Use Go v1.23.2 and follow standard Go idioms
Naming: use camelCase for variables, PascalCase for exported functions/types in Go
Type safety: use strong typing and avoidinterface{}/anywhere possible in Go
**/*.go: DO NOT COMMENT EXCESSIVELY. Instead, write clear and concise code that is self-explanatory.
Usego fmtfor formatting. Do not indent code manually.
Use Go v1.23.2 and follow standard Go idioms, naming: use camelCase for variables, PascalCase for exported functions/types
Usepkg/serrorsfor standard error types in error handling
Use strong typing and avoidinterface{}where possible
**/*.go: Every database query MUST include tenant filtering using organization_id
Usecomposables.UseTenantID(ctx)to retrieve the current tenant ID for multi-tenant isolation
Always wrap errors with operation context using serrors.Op() and serrors.E()
Usedi.Hpattern for dependency injection when constructing service dependencies
Use parameterized queries ($1, $2, etc.) - never use string concatenation for SQL queries
Files:
modules/finance/presentation/controllers/money_account_controller.gomodules/finance/services/money_account_service.gomodules/finance/presentation/controllers/dtos/money_account_dto.go
modules/*/presentation/controllers/*_controller.go
📄 CodeRabbit inference engine (CLAUDE.md)
Controller files should handle HTTP requests/responses and delegate business logic to services
Files:
modules/finance/presentation/controllers/money_account_controller.go
**/modules/*/presentation/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Create DTOs in
modules/{module}/presentation/controllers/dtos/{entity_name}_dto.go, controllers inmodules/{module}/presentation/controllers/{entity_name}_controller.go, and viewmodels inmodules/{module}/presentation/viewmodels/{entity_name}_viewmodel.go
Files:
modules/finance/presentation/controllers/money_account_controller.gomodules/finance/presentation/controllers/dtos/money_account_dto.go
**/modules/*/presentation/controllers/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use
htmx.IsHxRequest(r)to check if a request is from HTMX andhtmx.SetTrigger(w, "eventName", payload)for setting HTMX response triggers
Files:
modules/finance/presentation/controllers/money_account_controller.gomodules/finance/presentation/controllers/dtos/money_account_dto.go
modules/*/presentation/controllers/**/*.go
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
modules/*/presentation/controllers/**/*.go: Apply auth middleware viamiddleware.Authorize()in all controller routes
Parse form data usingcomposables.UseForm[DTO](r)in controllers
Parse query parameters usingcomposables.UseQuery[DTO](r)in controllers
Use functions frompkg/htmxpackage for HTMX response helpers
Files:
modules/finance/presentation/controllers/money_account_controller.gomodules/finance/presentation/controllers/dtos/money_account_dto.go
modules/*/services/*_service.go
📄 CodeRabbit inference engine (CLAUDE.md)
Service files should contain business logic and orchestrate repository and domain operations
Files:
modules/finance/services/money_account_service.go
**/modules/*/services/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Create service in
modules/{module}/services/{entity_name}_service.gowith event publishing and business logic methods, following constructor pattern:NewEntityService(repo, eventPublisher)
Files:
modules/finance/services/money_account_service.go
🧠 Learnings (6)
📚 Learning: 2025-11-13T06:55:06.385Z
Learnt from: Adam-xuya
Repo: iota-uz/iota-sdk PR: 487
File: modules/core/presentation/locales/zh.json:400-400
Timestamp: 2025-11-13T06:55:06.385Z
Learning: In modules/core/presentation/locales/*.json files, the duplicate "PermissionSets" key is intentional: a string value appears first (around line 397) followed by an object definition (around line 414). This pattern exists consistently across all locale files (en.json, ru.json, uz.json, zh.json) and should not be flagged as an issue.
Applied to files:
modules/finance/presentation/locales/en.jsonmodules/finance/presentation/locales/uz.jsonmodules/finance/presentation/locales/ru.json
📚 Learning: 2025-11-30T12:05:15.579Z
Learnt from: CR
Repo: iota-uz/iota-sdk PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T12:05:15.579Z
Learning: Applies to modules/billing/presentation/controllers/*_controller.go : Billing module controllers must handle Stripe subscription integration and payment processing
Applied to files:
modules/finance/presentation/controllers/money_account_controller.go
📚 Learning: 2025-12-13T00:56:30.707Z
Learnt from: CR
Repo: iota-uz/iota-sdk PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-13T00:56:30.707Z
Learning: Applies to modules/*/presentation/locales/{en,ru,uz}.toml : Update all 3 locale files (en.toml, ru.toml, uz.toml) when adding translations
Applied to files:
modules/finance/presentation/locales/uz.json
📚 Learning: 2025-11-30T12:06:22.341Z
Learnt from: CR
Repo: iota-uz/iota-sdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-30T12:06:22.341Z
Learning: Applies to **/modules/*/presentation/**/*.go : Create DTOs in `modules/{module}/presentation/controllers/dtos/{entity_name}_dto.go`, controllers in `modules/{module}/presentation/controllers/{entity_name}_controller.go`, and viewmodels in `modules/{module}/presentation/viewmodels/{entity_name}_viewmodel.go`
Applied to files:
modules/finance/presentation/controllers/dtos/money_account_dto.go
📚 Learning: 2025-11-30T12:06:03.232Z
Learnt from: CR
Repo: iota-uz/iota-sdk PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-30T12:06:03.232Z
Learning: After changes to Go code: run `go vet ./...`
Applied to files:
modules/finance/presentation/controllers/dtos/money_account_dto.go
📚 Learning: 2025-11-30T12:06:03.232Z
Learnt from: CR
Repo: iota-uz/iota-sdk PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-30T12:06:03.232Z
Learning: Create DTOs in `modules/{module}/presentation/controllers/dtos/{entity_name}_dto.go` and controllers in `modules/{module}/presentation/controllers/{entity_name}_controller.go`
Applied to files:
modules/finance/presentation/controllers/dtos/money_account_dto.go
🧬 Code graph analysis (2)
modules/finance/presentation/controllers/money_account_controller.go (3)
modules/finance/services/money_account_service.go (1)
ErrMoneyAccountDuplicateAccountNumber(16-16)pkg/composables/request.go (1)
UsePageCtx(95-105)modules/finance/presentation/mappers/mappers.go (2)
MoneyAccountToViewModel(50-64)MoneyAccountToViewUpdateModel(66-76)
modules/finance/presentation/controllers/dtos/money_account_dto.go (3)
modules/finance/domain/aggregates/payment_category/payment_category_repository.go (1)
Name(14-14)modules/finance/domain/aggregates/money_account/account_repository.go (4)
Name(14-14)Balance(16-16)AccountNumber(15-15)CurrencyCode(18-18)modules/finance/domain/aggregates/expense_category/expense_category_repository.go (1)
Name(14-14)
🔇 Additional comments (4)
modules/finance/presentation/controllers/dtos/money_account_dto.go (1)
26-30: Good addition of required validation for Name and CurrencyCode.The validation changes prevent clearing Name and CurrencyCode during updates, which aligns with the PR objectives.
However, note that AccountNumber (line 28) does not have
requiredvalidation. Based on the PR description mentioning "added 'required' validation for these fields" for both "Название" (Name) and "Номер счета" (Account number), consider whether AccountNumber should also be marked as required if it should not be clearable during edits.modules/finance/presentation/controllers/money_account_controller.go (2)
5-5: Good addition of errors package for type checking.The import enables proper error type checking using
errors.Isfor handling duplicate account number errors.
423-446: Good duplicate error handling in Create flow.The implementation correctly renders the Create drawer with a localized duplicate error message when in drawer mode, providing good user experience. The conditional logic properly distinguishes between duplicate errors (which can be handled in the UI) and other errors (which return HTTP 500).
modules/finance/presentation/locales/uz.json (1)
254-254: Add the missing translation to zh.jsonThe "DuplicateAccountNumber" translation is missing from
modules/finance/presentation/locales/zh.jsonwhile present in en.json, ru.json, and uz.json. This will causemake check trvalidation to fail. Add the corresponding Chinese translation to zh.json to maintain consistency across all locale files.⛔ Skipped due to learnings
Learnt from: Adam-xuya Repo: iota-uz/iota-sdk PR: 487 File: modules/core/presentation/locales/zh.json:400-400 Timestamp: 2025-11-13T06:55:06.385Z Learning: In modules/core/presentation/locales/*.json files, the duplicate "PermissionSets" key is intentional: a string value appears first (around line 397) followed by an object definition (around line 414). This pattern exists consistently across all locale files (en.json, ru.json, uz.json, zh.json) and should not be flagged as an issue.Learnt from: CR Repo: iota-uz/iota-sdk PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-30T12:05:15.579Z Learning: Applies to modules/*/presentation/locales/*.json : Translation files must be validated using `make check tr` before committingLearnt from: CR Repo: iota-uz/iota-sdk PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-12-13T00:56:30.707Z Learning: Run `make check tr` to validate translation file syntax and completenessLearnt from: CR Repo: iota-uz/iota-sdk PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-12-13T00:56:30.707Z Learning: Applies to modules/*/presentation/locales/{en,ru,uz}.toml : Update all 3 locale files (en.toml, ru.toml, uz.toml) when adding translationsLearnt from: CR Repo: iota-uz/iota-sdk PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-11-30T12:06:22.341Z Learning: Applies to **/modules/*/presentation/locales/*.json : Add translations to all locale files in `modules/{module}/presentation/locales/` (en.json, ru.json, uz.json) including NavigationLinks, Meta (titles), List, and Single sections
| if _, err := c.moneyAccountService.Update(r.Context(), entity); err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| if !errors.Is(err, services.ErrMoneyAccountDuplicateAccountNumber) { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| currencies, err := c.viewModelCurrencies(r) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| pageCtx := composables.UsePageCtx(r.Context()) | ||
| props := &moneyaccounts.DrawerEditProps{ | ||
| Account: mappers.MoneyAccountToViewModel(entity), | ||
| UpdateData: mappers.MoneyAccountToViewUpdateModel(entity), | ||
| Currencies: currencies, | ||
| Errors: map[string]string{ | ||
| "AccountNumber": pageCtx.T("MoneyAccounts.Single.DuplicateAccountNumber"), | ||
| }, | ||
| } | ||
| templ.Handler(moneyaccounts.EditDrawer(props), templ.WithStreaming()).ServeHTTP(w, r) | ||
| return | ||
| } |
There was a problem hiding this comment.
Missing transactions when rendering error drawer.
The duplicate error handling renders the Edit drawer with localized error messages (good UX improvement), but it doesn't fetch transactions like the normal edit flow does (lines 256-260). This creates inconsistency since DrawerEditProps expects a Transactions field (line 266 uses it, line 347 omits it).
Apply this fix to fetch transactions before rendering the error drawer:
if _, err := c.moneyAccountService.Update(r.Context(), entity); err != nil {
if !errors.Is(err, services.ErrMoneyAccountDuplicateAccountNumber) {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
currencies, err := c.viewModelCurrencies(r)
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
+
+ transactions, err := c.getAccountTransactions(r.Context(), id, 10)
+ if err != nil {
+ http.Error(w, "Error retrieving account transactions", http.StatusInternalServerError)
+ return
+ }
pageCtx := composables.UsePageCtx(r.Context())
props := &moneyaccounts.DrawerEditProps{
Account: mappers.MoneyAccountToViewModel(entity),
UpdateData: mappers.MoneyAccountToViewUpdateModel(entity),
Currencies: currencies,
+ Transactions: transactions,
Errors: map[string]string{
"AccountNumber": pageCtx.T("MoneyAccounts.Single.DuplicateAccountNumber"),
},
}
templ.Handler(moneyaccounts.EditDrawer(props), templ.WithStreaming()).ServeHTTP(w, r)
return
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In modules/finance/presentation/controllers/money_account_controller.go around
lines 328 to 351, the duplicate-account error branch renders the Edit drawer but
does not fetch Transactions (unlike the normal edit flow at lines ~256-260),
causing a missing Transactions field in DrawerEditProps; fix by fetching
transactions the same way as the normal flow (call the viewModelTransactions
helper with the request context and the account/ID, check and handle its error
returning HTTP 500 on failure), then include the resulting transactions in the
props passed to moneyaccounts.EditDrawer before rendering.
Issue: #354
По таску:
Не по таску, но похожие баги:
Summary by CodeRabbit
Bug Fixes
Localization
✏️ Tip: You can customize this high-level summary in your review settings.