feat(storage): more tests #245
Conversation
WalkthroughThis pull request introduces comprehensive changes to the internal storage and migration processes, focusing on enhancing test coverage, refactoring migration logic, and standardizing timestamp handling across various database-related components. The modifications span multiple files in the Changes
Assessment against linked issues
Possibly Related PRs
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #245 +/- ##
==========================================
+ Coverage 61.93% 66.47% +4.54%
==========================================
Files 533 541 +8
Lines 27449 27379 -70
==========================================
+ Hits 17000 18200 +1200
+ Misses 9319 7985 -1334
- Partials 1130 1194 +64 ☔ View full report in Codecov by Sentry. |
paul-nicolas
force-pushed
the
feat/more-tests
branch
from
196fb92 to
ea5ca51
Compare
paul-nicolas
force-pushed
the
feat/more-tests
branch
from
da0e4c9 to
bc07df8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (14)
internal/storage/migrations/main_test.go (1)
12-12: Consider scoping the PostgreSQL server instance to test suitesThe global
srvvariable could cause issues with parallel test execution. Consider scoping it to test suites or using a test fixture pattern.internal/storage/migrations/2-migrate-accounts-events-from-v2.go (1)
Line range hint
69-76: Consider adding error logging for failed insertionsThe batch insert operation could benefit from detailed error logging to help diagnose migration issues.
if len(events) > 0 { + logger := logging.FromContext(ctx) _, err = db.NewInsert(). Model(&events). On("conflict (id) do nothing"). Exec(ctx) if err != nil { + logger.Error().Err(err).Int("count", len(events)).Msg("Failed to insert events") return err } }internal/storage/migrations/1-migrate-connectors-from-v2.go (2)
Line range hint
92-96: Enhance error context for config transformationConsider adding more context to the error returned from
transformV2ConfigToV3Configto help diagnose migration issues.shouldInsert, v3Config, err := transformV2ConfigToV3Config(connector.Provider, connector.Config) if err != nil { + return fmt.Errorf("failed to transform config for connector %s: %w", connector.ID, err) }
Line range hint
13-13: Consider making encryption options configurableThe
encryptionOptionsconstant could be made configurable to support different encryption requirements.-const ( - encryptionOptions = "compress-algo=1, cipher-algo=aes256" -) +var encryptionOptions = getEncryptionOptions() + +func getEncryptionOptions() string { + if opts := os.Getenv("ENCRYPTION_OPTIONS"); opts != "" { + return opts + } + return "compress-algo=1, cipher-algo=aes256" +}internal/storage/migrations/10-migrate-payments-reversal.go (2)
112-112: Consider making the status offset adjustment more explicit.The comment indicates adding 1 to handle the unknown status, but this approach might be fragile. Consider using a more explicit mapping function.
-status := models.PaymentInitiationReversalAdjustmentStatus(int(reversal.Status) + 1) +func mapV2ToV3Status(status v2TransferReversalStatus) models.PaymentInitiationReversalAdjustmentStatus { + switch status { + case TransferReversalStatusProcessing: + return models.PAYMENT_INITIATION_REVERSAL_STATUS_PROCESSING + case TransferReversalStatusProcessed: + return models.PAYMENT_INITIATION_REVERSAL_STATUS_PROCESSED + case TransferReversalStatusFailed: + return models.PAYMENT_INITIATION_REVERSAL_STATUS_FAILED + default: + return models.PAYMENT_INITIATION_REVERSAL_STATUS_UNKNOWN + } +} +status := mapV2ToV3Status(reversal.Status)
Line range hint
149-156: Consider standardizing error handling.The error handling logic could be extracted into a helper function for consistency and reusability.
+func getErrorPtr(err string) *string { + if err == "" { + return nil + } + return &err +} - Error: func() *string { - if reversal.Error == "" { - return nil - } - return &reversal.Error - }(), + Error: getErrorPtr(reversal.Error),internal/storage/migrations/migrations_test.go (1)
83-351: LGTM! Thorough migration validation.The migration hooks provide excellent coverage:
- Comprehensive validation of migrated data across different entities
- Proper verification of data integrity and relationships
- Good use of assertions to ensure correct migration outcomes
Consider adding comments to explain the expected data state for complex validations, especially in hooks like
testPaymentReversalsMigrations.internal/storage/balances_test.go (1)
553-629: LGTM! Consider adding more edge cases.The new test function comprehensively tests balance retrieval at specific timestamps. Consider adding test cases for:
- Concurrent balance updates at the same timestamp
- Retrieving balances at the exact timestamp of an update
internal/storage/migrations/v2_migrations_test.sql (3)
13-18: Consider adding comments to document enum values.The enum types (
account_type,connector_provider,payment_status,payment_type,task_status,transfer_status) would benefit from documentation explaining the purpose and usage of each value.Also applies to: 20-32, 34-46, 48-53, 55-61, 63-67
137-147: Consider adding foreign key for payment asset references.The
payments.paymentandpayments.adjustmenttables store asset information but lack referential integrity. Consider creating an assets table and adding foreign key constraints.Also applies to: 149-156, 158-173
220-237: Use enum type for transfer initiation type.The
transfers.transfer_initiation.typecolumn uses integer but could benefit from an enum type like other status fields for better type safety and documentation.Apply this diff:
- type integer NOT NULL, +CREATE TYPE public.transfer_initiation_type AS ENUM ( + 'TRANSFER', + 'PAYOUT' +); + type public.transfer_initiation_type NOT NULL,internal/storage/migrations/migrations.go (1)
24-126: Refactor migration registration to reduce code duplicationThe
registerMigrationsfunction contains repetitive code for registering migrations, which can be streamlined to enhance maintainability and readability.Consider introducing a helper function to encapsulate the common logic:
func newMigration(name string, upFunc func(ctx context.Context, db bun.IDB) error) migrations.Migration { return migrations.Migration{ Name: name, Up: func(ctx context.Context, db bun.IDB) error { return db.RunInTx(ctx, &sql.TxOptions{}, func(ctx context.Context, tx bun.Tx) error { return upFunc(ctx, tx) }) }, } }Then, update the
registerMigrationsfunction:func registerMigrations(migrator *migrations.Migrator, encryptionKey string) { migrator.RegisterMigrations( newMigration("init schema", func(ctx context.Context, db bun.IDB) error { _, err := db.ExecContext(ctx, initSchema) return err }), newMigration("migrate connectors from v2", func(ctx context.Context, db bun.IDB) error { return MigrateConnectorsFromV2(ctx, db, encryptionKey) }), // ...additional migrations ) }internal/storage/migrations/8-migrate-transfer-initiation-adjustments-from-v2.go (2)
14-29: Implement string representation for TransferInitiationStatusCurrently,
TransferInitiationStatusis an integer type, which may not provide meaningful output when logged or printed. Implementing theStringerinterface will enhance debugging and logging.Add the following method to provide string representations:
func (s TransferInitiationStatus) String() string { switch s { case TransferInitiationStatusWaitingForValidation: return "WaitingForValidation" case TransferInitiationStatusProcessing: return "Processing" case TransferInitiationStatusProcessed: return "Processed" case TransferInitiationStatusFailed: return "Failed" case TransferInitiationStatusRejected: return "Rejected" case TransferInitiationStatusValidated: return "Validated" case TransferInitiationStatusAskRetried: return "AskRetried" case TransferInitiationStatusAskReversed: return "AskReversed" case TransferInitiationStatusReverseProcessing: return "ReverseProcessing" case TransferInitiationStatusReverseFailed: return "ReverseFailed" case TransferInitiationStatusPartiallyReversed: return "PartiallyReversed" case TransferInitiationStatusReversed: return "Reversed" default: return "Unknown" } }
141-145: Avoid redundancy in CreatedAt field assignmentThe
CreatedAtfield is assigned both in theIDstruct and at the top level ofv3PaymentInitiationAdjustment. Unless there is a specific reason, this duplication might be unnecessary.Consider removing the redundant
CreatedAtassignment:v3Adjs = append(v3Adjs, v3PaymentInitiationAdjustment{ ID: models.PaymentInitiationAdjustmentID{ PaymentInitiationID: adjustment.TransferInitiationID, CreatedAt: adjustment.CreatedAt.UTC(), Status: status, }, PaymentInitiationID: adjustment.TransferInitiationID, Status: status, Error: &adjustment.Error, Amount: amount, Asset: &asset, Metadata: adjustment.Metadata, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.codecov.ymlis excluded by!**/*.ymlgo.modis excluded by!**/*.mod
📒 Files selected for processing (22)
Earthfile(1 hunks)internal/storage/accounts_test.go(2 hunks)internal/storage/balances_test.go(3 hunks)internal/storage/bank_accounts_test.go(3 hunks)internal/storage/migrations.go(1 hunks)internal/storage/migrations/1-migrate-connectors-from-v2.go(1 hunks)internal/storage/migrations/10-migrate-payments-reversal.go(4 hunks)internal/storage/migrations/2-migrate-accounts-events-from-v2.go(1 hunks)internal/storage/migrations/3-migrate-payments-events-from-v2.go(2 hunks)internal/storage/migrations/4-migrate-balances-events-from-v2.go(2 hunks)internal/storage/migrations/8-migrate-transfer-initiation-adjustments-from-v2.go(3 hunks)internal/storage/migrations/main_test.go(1 hunks)internal/storage/migrations/migrations.go(1 hunks)internal/storage/migrations/migrations_test.go(1 hunks)internal/storage/migrations/v2_migrations_test.sql(1 hunks)internal/storage/payment_initiation_reversals_test.go(3 hunks)internal/storage/payment_initiations_test.go(4 hunks)internal/storage/payments_test.go(3 hunks)internal/storage/pools_test.go(2 hunks)internal/storage/schedules_test.go(2 hunks)internal/storage/webhooks_configs_test.go(1 hunks)internal/storage/workflow_instances_test.go(2 hunks)
🔇 Additional comments (38)
internal/storage/migrations/main_test.go (1)
14-20: Verify cleanup of Docker resourcesEnsure that Docker resources are properly cleaned up after tests complete.
✅ Verification successful
Docker resources are properly managed
The cleanup of Docker resources is handled automatically by the testing framework through:
- The
WithTestMainwrapper from go-libs testing utilities- The
pgtesting.PostgresServerimplementation- The deferred resource management pattern used in the test suite
The code follows established patterns for test resource management and cleanup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for resource cleanup handling rg -A 5 "defer.*Clean|defer.*Close|defer.*Stop" internal/storage/migrations/Length of output: 78
Script:
#!/bin/bash # Check WithTestMain implementation ast-grep --pattern 'func WithTestMain($_) { $$$ }' # Check CreatePostgresServer implementation ast-grep --pattern 'func CreatePostgresServer($_) $_$ { $$$ }' # Check for cleanup in docker pool ast-grep --pattern 'func NewPool($_) $_$ { $$$ }' # Broader search for cleanup patterns rg -g '!{vendor,third_party}/**' "(WithTestMain|CreatePostgresServer|NewPool).*cleanup"Length of output: 265
Script:
#!/bin/bash # Find relevant files fd -t f -e go pgtesting fd -t f -e go docker.go # Look for Docker cleanup patterns more broadly rg -g '*.go' "func.*Clean|func.*Stop|func.*Close" --type go # Check test utilities rg -g '*.go' "type TestingTForMain|func.*TestMain" --type goLength of output: 844
Script:
#!/bin/bash # Check Docker-specific cleanup rg -g '*.go' "docker.*Clean|docker.*Stop|docker.*Close" -B 2 -A 5 # Find TestingTForMain implementation and usage rg -g '*.go' "TestingTForMain" -B 2 -A 5 # Check pgtesting package usage rg -g '*.go' "pgtesting" -B 2 -A 5Length of output: 6405
internal/storage/migrations/2-migrate-accounts-events-from-v2.go (1)
61-65: LGTM! Improved type safety and timestamp consistencyGood improvements:
- Using
models.IdempotencyKeytype conversion instead of string- Ensuring UTC timezone for timestamps
internal/storage/migrations/4-migrate-balances-events-from-v2.go (2)
65-66: LGTM! Consistent timestamp handlingGood standardization of UTC timezone across all timestamp fields:
- CreatedAt
- LastUpdatedAt
- SentAt
Also applies to: 77-77
Line range hint
39-43: Verify pagination size differencesThe page size (100) differs from other migrations (1000). Consider standardizing or documenting the reason for different page sizes.
✅ Verification successful
Different page sizes appear intentional based on migration patterns
Event-related migrations (like this one) consistently use smaller page sizes (100), while other migrations typically use 1000. This pattern suggests a deliberate choice rather than an inconsistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check pagination sizes across migrations rg "PageSize.*[0-9]+" internal/storage/migrations/Length of output: 1973
internal/storage/migrations/1-migrate-connectors-from-v2.go (1)
87-87: LGTM! Consistent timestamp handlingGood standardization of UTC timezone for CreatedAt field.
internal/storage/webhooks_configs_test.go (1)
98-121: LGTM! Well-structured test cases.The test function thoroughly covers both positive and negative scenarios for retrieving webhook configurations. The use of
ElementsMatchis appropriate for comparing slices without order dependency.internal/storage/migrations/3-migrate-payments-events-from-v2.go (2)
77-86: Consistent timestamp handling in payment adjustments migration.The changes ensure consistent UTC timestamp handling and proper idempotency key construction.
159-168: Consistent timestamp handling in payments migration.The changes maintain consistency with the adjustments migration, ensuring UTC timestamp handling and proper idempotency key construction.
Earthfile (1)
80-81: LGTM! Simplified coverage configuration.The change to use
-coverpkg=./...simplifies the configuration while ensuring comprehensive coverage across all packages.internal/storage/migrations/10-migrate-payments-reversal.go (1)
14-20: LGTM! Clear status enumeration.The introduction of
v2TransferReversalStatuswith clear constants improves code readability and type safety.internal/storage/schedules_test.go (1)
179-189: LGTM! Good test coverage for error handling.The added test cases thoroughly verify the error handling for invalid query conditions:
- Testing invalid operator (
Lt) withconnector_idfield- Testing unknown key in query builder
Both cases properly assert that an error is returned and the cursor is nil.
Also applies to: 226-236
internal/storage/workflow_instances_test.go (1)
196-206: LGTM! Consistent error handling test coverage.The added test cases maintain consistency with other test files in verifying error handling:
- Testing invalid operator (
Lt) withschedule_idfield- Testing unknown key in query builder
Both cases properly assert that an error is returned and the cursor is nil.
Also applies to: 276-286
internal/storage/pools_test.go (1)
283-293: LGTM! Consistent test coverage across modules.The added test cases maintain consistency with other modules in verifying error handling:
- Testing invalid operator (
Lt) withnamefield- Testing unknown key in query builder
Both cases properly assert that an error is returned and the cursor is nil.
Also applies to: 326-336
internal/storage/migrations/migrations_test.go (1)
1-81: LGTM! Well-structured test setup.The test setup is comprehensive and well-organized:
- Clear separation of test cases for with/without V2 data
- Proper use of test database and connection handling
- Good use of test hooks for different migration scenarios
internal/storage/accounts_test.go (3)
140-142: LGTM! Good edge case test.The test case verifies that upserting an empty list of accounts is handled gracefully.
519-529: LGTM! Good validation test.The test case ensures that using an invalid operator (
Lt) with metadata fields returns an error, preventing potential misuse of the query builder.
531-541: LGTM! Good validation test.The test case verifies that using an unknown key in the query builder returns an error, ensuring robust error handling.
internal/storage/balances_test.go (3)
79-81: LGTM! Good edge case test.The test case verifies that upserting empty balances is handled gracefully.
83-99: LGTM! Good validation test.The test case ensures that attempting to upsert a balance with an unknown connector ID returns an error, preventing data inconsistency.
101-135: LGTM! Good business logic test.The test case verifies that upserting a balance with an older timestamp doesn't modify existing balances, maintaining data integrity.
internal/storage/bank_accounts_test.go (3)
336-346: LGTM! Good validation test.The test case ensures that using an invalid operator (
Lt) with the name field returns an error, preventing potential misuse of the query builder.
444-454: LGTM! Good validation test.The test case verifies that using an invalid operator (
Lt) with metadata fields returns an error, maintaining query builder integrity.
487-497: LGTM! Good validation test.The test case ensures that using an unknown key in the query builder returns an error, providing robust error handling.
internal/storage/payment_initiation_reversals_test.go (3)
200-210: LGTM! Good validation test.The test case ensures that using an invalid operator (
Lt) with the reference field returns an error, preventing potential misuse of the query builder.
365-375: LGTM! Good validation test.The test case verifies that using an invalid operator (
Lt) with metadata fields returns an error, maintaining query builder integrity.
417-426: LGTM! Good validation test.The test case ensures that using an unknown key in the query builder returns an error, providing robust error handling.
internal/storage/migrations/v2_migrations_test.sql (2)
413-425: Encrypt sensitive configuration data.Good use of encryption for the
configcolumn in theconnectors.connectortable. The test data shows encrypted values which is the correct approach.
449-452:⚠️ Potential issueVerify encryption key management for bank account data.
The bank account data is encrypted using a symmetric key 'default-encryption-key'. Ensure this is replaced with proper key management in production.
internal/storage/migrations/migrations.go (1)
12-22: Ensure embedded SQL files are present and correctly referencedThe migration scripts are embedded using
go:embed. Verify that the referenced SQL files (0-init-schema.sql,5-migrate-bank-accounts-from-v2.sql,7-migrate-transfer-initiations-from-v2.sql,9-migrate-pools-from-v2.sql) exist in the correct directory to prevent runtime errors during compilation.Run the following script to confirm the presence of the embedded SQL files:
✅ Verification successful
SQL migration files are correctly embedded and accessible
All referenced SQL migration files exist in the correct directory, are readable, and contain content, ensuring successful compilation and runtime operation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the presence of embedded SQL files. required_files=( "0-init-schema.sql" "5-migrate-bank-accounts-from-v2.sql" "7-migrate-transfer-initiations-from-v2.sql" "9-migrate-pools-from-v2.sql" ) for file in "${required_files[@]}"; do if [ ! -f "internal/storage/migrations/$file" ]; then echo "Missing file: $file" fi doneLength of output: 604
Script:
#!/bin/bash # Verify SQL files are non-empty and readable ls -l internal/storage/migrations/*.sqlLength of output: 833
internal/storage/payments_test.go (3)
647-657: LGTM! Good test coverage for invalid reference query operator.The test case properly validates that using an incorrect operator (
Lt) for the reference field returns an error and nil cursor.
932-942: LGTM! Good test coverage for invalid metadata query operator.The test case properly validates that using an incorrect operator (
Lt) for metadata fields returns an error and nil cursor.
984-994: LGTM! Good test coverage for unknown query key.The test case properly validates that using an unknown query builder key returns an error and nil cursor.
internal/storage/payment_initiations_test.go (6)
291-301: LGTM! Good test coverage for invalid reference query operator.The test case properly validates that using an incorrect operator (
Lt) for the reference field returns an error and nil cursor.
525-535: LGTM! Good test coverage for invalid metadata query operator.The test case properly validates that using an incorrect operator (
Lt) for metadata fields returns an error and nil cursor.
577-587: LGTM! Good test coverage for unknown query key.The test case properly validates that using an unknown query builder key returns an error and nil cursor.
1020-1030: LGTM! Good test coverage for invalid status query operator.The test case properly validates that using an incorrect operator (
Lt) for the status field returns an error and nil cursor.
1046-1056: LGTM! Good test coverage for invalid metadata query operator.The test case properly validates that using an incorrect operator (
Lt) for metadata fields returns an error and nil cursor.
1072-1082: LGTM! Good test coverage for unknown query key.The test case properly validates that using an unknown query builder key returns an error and nil cursor.
| CreatedAt: reversal.CreatedAt.UTC(), | ||
| Status: models.PAYMENT_INITIATION_REVERSAL_STATUS_PROCESSING, | ||
| }, | ||
| PaymentInitiationReversalID: reversal.ID, | ||
| CreatedAt: reversal.CreatedAt, | ||
| CreatedAt: reversal.CreatedAt.UTC(), | ||
| Status: models.PAYMENT_INITIATION_REVERSAL_STATUS_PROCESSING, | ||
| Metadata: reversal.Metadata, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate status and timestamp assignments.
The initial adjustment has redundant assignments of CreatedAt and Status.
- CreatedAt: reversal.CreatedAt.UTC(),
- Status: models.PAYMENT_INITIATION_REVERSAL_STATUS_PROCESSING,
+ CreatedAt: createdAtUTC,
+ Status: initialStatus,
},
PaymentInitiationReversalID: reversal.ID,
- CreatedAt: reversal.CreatedAt.UTC(),
- Status: models.PAYMENT_INITIATION_REVERSAL_STATUS_PROCESSING,
+ CreatedAt: createdAtUTC,
+ Status: initialStatus,Consider extracting these values to variables at the beginning of the loop to improve maintainability and reduce duplication.
Committable suggestion skipped: line range outside the PR's diff.
| func shouldSkipStatus(status TransferInitiationStatus) (bool, models.PaymentInitiationAdjustmentStatus) { | ||
| switch status { | ||
| case TransferInitiationStatusWaitingForValidation: | ||
| return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_WAITING_FOR_VALIDATION | ||
| case TransferInitiationStatusProcessing: | ||
| return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_PROCESSING | ||
| case TransferInitiationStatusProcessed: | ||
| return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_PROCESSED | ||
| case TransferInitiationStatusFailed: | ||
| return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_FAILED | ||
| case TransferInitiationStatusRejected: | ||
| return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_REJECTED | ||
| case TransferInitiationStatusReverseProcessing: | ||
| return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_REVERSE_PROCESSING | ||
| case TransferInitiationStatusReverseFailed: | ||
| return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_REVERSE_FAILED | ||
| case TransferInitiationStatusReversed: | ||
| return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_REVERSED | ||
| } | ||
| return true, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_UNKNOWN | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle all statuses in shouldSkipStatus function
The shouldSkipStatus function does not cover all possible TransferInitiationStatus values, which could lead to unexpected behavior if unhandled statuses are encountered.
Include all statuses in the switch statement to ensure comprehensive handling:
func shouldSkipStatus(status TransferInitiationStatus) (bool, models.PaymentInitiationAdjustmentStatus) {
switch status {
case TransferInitiationStatusWaitingForValidation:
return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_WAITING_FOR_VALIDATION
case TransferInitiationStatusProcessing:
return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_PROCESSING
case TransferInitiationStatusProcessed:
return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_PROCESSED
case TransferInitiationStatusFailed:
return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_FAILED
case TransferInitiationStatusRejected:
return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_REJECTED
case TransferInitiationStatusReverseProcessing:
return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_REVERSE_PROCESSING
case TransferInitiationStatusReverseFailed:
return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_REVERSE_FAILED
case TransferInitiationStatusReversed:
return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_REVERSED
case TransferInitiationStatusValidated:
// Decide whether to skip or map to an existing status
// For example:
return false, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_VALIDATED
case TransferInitiationStatusAskRetried:
// Similarly handle or skip
case TransferInitiationStatusAskReversed:
// Similarly handle or skip
case TransferInitiationStatusPartiallyReversed:
// Similarly handle or skip
default:
return true, models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_UNKNOWN
}
}Ensure that corresponding statuses exist in models.PaymentInitiationAdjustmentStatus, or document why certain statuses are intentionally skipped.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/storage/migrations/8-migrate-transfer-initiation-adjustments-from-v2.go (1)
142-146: Optimize duplicate UTC conversion.The UTC conversion is performed twice for the same timestamp. Consider storing the UTC timestamp in a variable to avoid duplicate conversions:
+ createdAtUTC := adjustment.CreatedAt.UTC() v3Adjs = append(v3Adjs, v3PaymentInitiationAdjustment{ ID: models.PaymentInitiationAdjustmentID{ PaymentInitiationID: adjustment.TransferInitiationID, - CreatedAt: adjustment.CreatedAt.UTC(), + CreatedAt: createdAtUTC, Status: status, }, PaymentInitiationID: adjustment.TransferInitiationID, - CreatedAt: adjustment.CreatedAt.UTC(), + CreatedAt: createdAtUTC,internal/storage/migrations/10-migrate-payments-reversal.go (1)
126-149: Optimize duplicate status assignments.The status assignments are duplicated in the struct initialization. Consider storing the initial status in a variable:
createdAt := reversal.CreatedAt.UTC() + initialStatus := models.PAYMENT_INITIATION_REVERSAL_STATUS_PROCESSING v3ReversalAdjustments = append(v3ReversalAdjustments, v3PaymentInitiationReversalAdjustment{ ID: models.PaymentInitiationReversalAdjustmentID{ PaymentInitiationReversalID: reversal.ID, CreatedAt: createdAt, - Status: models.PAYMENT_INITIATION_REVERSAL_STATUS_PROCESSING, + Status: initialStatus, }, PaymentInitiationReversalID: reversal.ID, CreatedAt: createdAt, - Status: models.PAYMENT_INITIATION_REVERSAL_STATUS_PROCESSING, + Status: initialStatus,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/storage/migrations/10-migrate-payments-reversal.go(4 hunks)internal/storage/migrations/8-migrate-transfer-initiation-adjustments-from-v2.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (5)
internal/storage/migrations/8-migrate-transfer-initiation-adjustments-from-v2.go (3)
14-29: LGTM! Well-structured status enum.The
TransferInitiationStatustype and its constants are well-defined, using iota for sequential values and following consistent naming conventions.
37-37: LGTM! Improved type safety.Changing the Status field type from string to TransferInitiationStatus enhances type safety and prevents invalid status values.
42-63: Handle all statuses in shouldSkipStatus function.The function does not cover all possible
TransferInitiationStatusvalues, which could lead to unexpected behavior if unhandled statuses are encountered.internal/storage/migrations/10-migrate-payments-reversal.go (2)
14-20: LGTM! Well-structured status enum.The
v2TransferReversalStatustype and its constants are well-defined, using iota for sequential values and following consistent naming conventions.
37-38: LGTM! Improved type safety.Changing the Status field type to v2TransferReversalStatus enhances type safety and prevents invalid status values.
Fixes ENG-1580