feat: expose accounts insertion date and update date #988
Conversation
WalkthroughThis change introduces two new timestamp fields, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Storage
participant DB
Client->>API: Create or update account/transaction
API->>Storage: Store or update Account/Transaction with insertionDate and updatedAt
Storage->>DB: Write data with timestamps
DB-->>Storage: Confirm write
Storage-->>API: Return data including insertionDate and updatedAt
API-->>Client: Respond with JSON including new timestamps
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/api/README.md (1)
863-865: Same-timestamp duplication repeatedThe same remark as above applies to the single-account response example – both timestamps are identical. Please update or annotate accordingly.
🧹 Nitpick comments (6)
internal/storage/ledger/resource_accounts.go (1)
32-32: Consider column selection efficiency.The new columns are properly added to the select query. However, consider whether selecting specific columns versus using wildcards aligns with the overall query optimization strategy, especially as more fields are added to the schema.
docs/api/README.md (3)
772-778: Verify chronology & sample accuracy
updatedAtis set to the exact same value asinsertionDatein the sample.
Unless the account has never been modified after insertion, these two timestamps should normally differ. Using identical values in the reference payload could mislead integrators and weaken automated examples/tests derived from the doc.Consider adjusting the example so
updatedAtreflects a later point in time, or add an explicit note that both fields are equal when an account has never been updated.
2130-2132: Missing schema descriptions for new properties
insertionDateandupdatedAtwere added toV2Account, yet the property table still showsnonein the Description column. Providing a concise description (e.g. “Account creation date (immutable)” / “Last metadata update date”) will improve API usability and generated client docs.-|insertionDate|string(date-time)|false|none|none| -|updatedAt|string(date-time)|false|none|none| +|insertionDate|string(date-time)|false|none|Timestamp when the account was first created (read-only).| +|updatedAt|string(date-time)|false|none|Timestamp of the most recent metadata update on the account (read-only).|Also applies to: 2564-2566
772-778: Consider harmonising naming with existing fields (insertedAt)Transactions use
insertedAtwhile accounts useinsertionDate. Introducing two different notations for the same concept increases cognitive load and complicates generic tooling (sorting, filtering, SDK mapping).If there is no strong backward-compatibility constraint, aligning on either
insertedAtorinsertionDateacross resources would make the API more coherent.No change requested if the asymmetry is intentional, but worth a second look.
Also applies to: 863-865, 2130-2132, 2564-2566
test/e2e/api_accounts_list_test.go (2)
60-63: Consider using more realistic test values.The extremely large
bigIntvalue (90+ digits) seems unnecessarily complex for testing timestamp functionality. A simpler value likebig.NewInt(1000)would be more readable and serve the same purpose.-bigInt, _ = big.NewInt(0).SetString("999999999999999999999999999999999999999999999999999999999999999999999999999999999999999", 10) +bigInt = big.NewInt(1000)
300-337: Refactor complex bulk account creation logic.The bulk account creation has become significantly more complex, mixing transaction creation with account list building. This makes the test harder to understand and maintain.
Consider extracting the account creation logic into a helper function:
+createAccountWithTransaction := func(specContext SpecContext, destination string) (components.V2Account, *time.Time) { + ret, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.CreateTransaction( + ctx, + operations.V2CreateTransactionRequest{ + Ledger: "default", + V2PostTransaction: components.V2PostTransaction{ + Postings: []components.V2Posting{{ + Amount: big.NewInt(0), + Asset: "USD", + Source: "world", + Destination: destination, + }}, + }, + }, + ) + Expect(err).ToNot(HaveOccurred()) + + insertionDate := ret.V2CreateTransactionResponse.Data.InsertedAt + return components.V2Account{ + Address: destination, + Metadata: metadata.Metadata{}, + FirstUsage: insertionDate, + InsertionDate: insertionDate, + UpdatedAt: insertionDate, + }, insertionDate +} BeforeEach(func(specContext SpecContext) { + var worldInsertionDate *time.Time for i := 0; i < int(accountCounts)-1; i++ { - ret, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.CreateTransaction( - ctx, - operations.V2CreateTransactionRequest{ - Ledger: "default", - V2PostTransaction: components.V2PostTransaction{ - Postings: []components.V2Posting{{ - Amount: big.NewInt(0), - Asset: "USD", - Source: "world", - Destination: fmt.Sprintf("foo:%d", i), - }}, - }, - }, - ) - Expect(err).ToNot(HaveOccurred()) - - accounts = append(accounts, components.V2Account{ - Address: fmt.Sprintf("foo:%d", i), - Metadata: metadata.Metadata{}, - FirstUsage: ret.V2CreateTransactionResponse.Data.InsertedAt, - InsertionDate: ret.V2CreateTransactionResponse.Data.InsertedAt, - UpdatedAt: ret.V2CreateTransactionResponse.Data.InsertedAt, - }) + account, insertionDate := createAccountWithTransaction(specContext, fmt.Sprintf("foo:%d", i)) + accounts = append(accounts, account) + if i == 0 { - accounts = append(accounts, components.V2Account{ - Address: "world", - Metadata: metadata.Metadata{}, - FirstUsage: ret.V2CreateTransactionResponse.Data.InsertedAt, - InsertionDate: ret.V2CreateTransactionResponse.Data.InsertedAt, - UpdatedAt: ret.V2CreateTransactionResponse.Data.InsertedAt, - }) + worldInsertionDate = insertionDate + accounts = append(accounts, components.V2Account{ + Address: "world", + Metadata: metadata.Metadata{}, + FirstUsage: worldInsertionDate, + InsertionDate: worldInsertionDate, + UpdatedAt: worldInsertionDate, + }) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
openapi.yamlis excluded by!**/*.yamlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lockpkg/client/.speakeasy/logs/naming.logis excluded by!**/*.log,!**/*.logpkg/client/speakeasyusagegen/.speakeasy/logs/naming.logis excluded by!**/*.log,!**/*.log
📒 Files selected for processing (10)
docs/api/README.md(6 hunks)internal/README.md(1 hunks)internal/account.go(1 hunks)internal/api/v2/views_test.go(2 hunks)internal/storage/ledger/accounts.go(2 hunks)internal/storage/ledger/resource_accounts.go(1 hunks)pkg/client/docs/models/components/v2account.md(1 hunks)pkg/client/models/components/v2account.go(2 hunks)test/e2e/api_accounts_list_test.go(4 hunks)test/e2e/api_transactions_create_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
test/e2e/api_transactions_create_test.go (1)
Learnt from: gfyrag
PR: formancehq/ledger#935
File: test/e2e/api_ledgers_import_test.go:338-352
Timestamp: 2025-05-20T13:08:10.034Z
Learning: The transactions list API for the ledger service has a stable ordering by ID that will never change, making it safe to rely on transactionsFromOriginalLedger.Data[0].ID in tests.
test/e2e/api_accounts_list_test.go (1)
Learnt from: gfyrag
PR: formancehq/ledger#935
File: test/e2e/api_ledgers_import_test.go:338-352
Timestamp: 2025-05-20T13:08:10.034Z
Learning: The transactions list API for the ledger service has a stable ordering by ID that will never change, making it safe to rely on transactionsFromOriginalLedger.Data[0].ID in tests.
🧬 Code Graph Analysis (3)
internal/storage/ledger/resource_accounts.go (1)
internal/storage/common/schema.go (4)
NewStringField(62-64)NewDateField(67-69)NewNumericMapField(92-94)NewStringMapField(87-89)
test/e2e/api_transactions_create_test.go (1)
pkg/client/models/components/v2account.go (1)
V2Account(10-18)
test/e2e/api_accounts_list_test.go (5)
pkg/client/v2.go (1)
V2(20-24)pkg/client/models/operations/v2createtransaction.go (1)
V2CreateTransactionRequest(9-25)pkg/client/models/components/v2posttransaction.go (1)
V2PostTransaction(58-68)pkg/client/models/components/v2posting.go (1)
V2Posting(10-15)pkg/client/models/components/v2account.go (1)
V2Account(10-18)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (16)
internal/storage/ledger/accounts.go (2)
46-50: LGTM! Proper initialization of new timestamp fields.The initialization of
InsertionDateandUpdatedAtfields follows the existing pattern correctly. Both fields are appropriately set to the current timestamp during account creation.
61-61: Conditionalupdated_atupdate verifiedThe upsert clauses in
internal/storage/ledger/accounts.goonly execute theSET("updated_at = excluded.updated_at")when metadata (orfirst_usage) actually changes—thanks to theWHERE not accounts.metadata @> excluded.metadataand the combinedOR first_usagecondition. This ensuresupdated_atreflects the last metadata modification as intended. No further changes needed.internal/api/v2/views_test.go (2)
384-386: LGTM! Test expectations properly updated for new timestamp fields.The test cases are correctly updated to include
insertionDateandupdatedAtin the expected output, maintaining consistency with the existingfirstUsagefield formatting.
413-415: LGTM! Consistent test updates across both test scenarios.The "big int as string" test case is properly updated with the same timestamp fields, ensuring consistent behavior across different rendering modes.
pkg/client/docs/models/components/v2account.md (1)
10-11: LGTM! Clear documentation for new optional timestamp fields.The documentation properly describes the new
InsertionDateandUpdatedAtfields as optional timestamp fields with appropriate examples, maintaining consistency with the existingFirstUsagefield documentation.internal/storage/ledger/resource_accounts.go (1)
23-24: LGTM! Proper schema definition for new timestamp fields.The new
insertion_dateandupdated_atfields are correctly defined as paginated date fields, following the same pattern as the existingfirst_usagefield.internal/account.go (1)
20-21: LGTM! Essential change to expose timestamp fields in JSON.Changing the JSON tags from
json:"-"to proper camelCase field names (insertionDateandupdatedAt) is crucial for exposing these fields in API responses. The naming convention is consistent with existing fields.internal/README.md (1)
236-237: LGTM! JSON serialization exposure is correctly documented.The documentation correctly reflects the change from omitted JSON fields (
json:"-") to explicitly included camelCase JSON tags for the timestamp fields. This aligns perfectly with the PR objective to expose insertion and update dates in API responses.pkg/client/models/components/v2account.go (2)
13-14: LGTM! Consistent implementation of optional timestamp fields.The new fields follow established patterns with pointer types and
omitemptyJSON tags, ensuring backwards compatibility while properly exposing the timestamp information.
45-57: LGTM! Getter methods follow established patterns.The new getter methods are implemented consistently with existing getters, including proper nil-checking for defensive programming and appropriate return types.
test/e2e/api_transactions_create_test.go (3)
128-131: LGTM! Test assertions properly validate new timestamp fields.The test correctly includes the new timestamp fields in the account equality check, using the actual response data for comparison. This approach is appropriate for system-generated timestamp values.
143-146: LGTM! Consistent test pattern for timestamp validation.The test maintains consistency with other test cases by including the new timestamp fields and using actual response data for assertions.
287-289: LGTM! Logical timestamp mapping in test expectations.The test correctly maps the new timestamp fields to appropriate transaction data:
InsertionDateandUpdatedAtuse the transaction's insertion timeFirstUsageuses the transaction's business timestampThis creates a clear semantic distinction between system timestamps and business logic timestamps.
test/e2e/api_accounts_list_test.go (3)
65-84: Good approach to establish timestamp baseline.Creating a transaction upfront to capture the insertion date for later assertions is a solid testing strategy that ensures consistent timestamp references across test assertions.
158-160: Verify timestamp field consistency.The test now asserts
FirstUsageandInsertionDatefields. Ensure thatFirstUsageuses the transaction timestamp whileInsertionDateuses the insertion time, as this reflects the correct semantic distinction.
162-175: Good validation of timestamp fields for system accounts.The "world" account properly includes all three timestamp fields (
FirstUsage,InsertionDate,UpdatedAt), which demonstrates the system correctly tracks these timestamps for both user and system accounts.
| // We don't have this information | ||
| accountsCursorResponse[1].UpdatedAt = nil | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address inconsistent test data setup.
The explicit nulling of UpdatedAt with the comment "We don't have this information" suggests inconsistent behavior in how timestamps are populated. This could indicate a gap in the implementation or test setup.
Verify why UpdatedAt is not consistently populated for all accounts:
🏁 Script executed:
#!/bin/bash
# Description: Check how UpdatedAt is populated in account creation vs metadata addition flows
# Search for UpdatedAt field usage in account-related code
rg -A 5 -B 5 "UpdatedAt.*=" --type goLength of output: 853
🏁 Script executed:
#!/bin/bash
# Display the surrounding test code for context
sed -n '130,200p' test/e2e/api_accounts_list_test.goLength of output: 2262
🏁 Script executed:
#!/bin/bash
# Search for V2Account struct definition
rg "type V2Account" -A5 -B5 --type goLength of output: 3428
Unify UpdatedAt handling in api_accounts_list_test.go
In test/e2e/api_accounts_list_test.go:145–147, the test currently does:
// We don't have this information
accountsCursorResponse[1].UpdatedAt = nilwhile for the third account you explicitly assert UpdatedAt == txInsertionDate. This asymmetry suggests the API is always returning an updatedAt timestamp, and the test is compensating by manually nulling it out in one case. To make your tests clearer and less brittle:
- Remove the manual override of
UpdatedAt. - Either assert the expected timestamp for every account (e.g. include
UpdatedAt: txInsertionDatein your expected struct literal),
or - Use Gomega’s field‐matching helpers to ignore
UpdatedAtwhere it isn’t relevant. For example:
Expect(accountsCursorResponse[1]).To(MatchFields(IgnoreExtras, Fields{
"Address": Equal("foo:foo"),
"Metadata": Equal(metadata1),
"Volumes": Equal(map[string]components.V2Volume{ /* … */ }),
"FirstUsage": Equal(&txTimestamp),
"InsertionDate": Equal(txInsertionDate),
// UpdatedAt is simply omitted here
}))This will eliminate the need for hidden test hacks and ensure consistent expectations across all accounts.
🤖 Prompt for AI Agents
In test/e2e/api_accounts_list_test.go around lines 145 to 147, remove the manual
assignment setting accountsCursorResponse[1].UpdatedAt to nil. Instead, update
the test assertions to either explicitly check UpdatedAt for all accounts by
including it in the expected struct literals or use Gomega’s field-matching
helpers like MatchFields with IgnoreExtras to omit UpdatedAt where it is not
relevant. This will unify UpdatedAt handling and avoid brittle test hacks.
There was a problem hiding this comment.
The gomega solution is interesting but if I add new fields, the tests will pass without any checks.
I prefer not ignore any fields.
We do that, because, in this perticular use case, we don't have the exact value of the field.
So we set to nil. We could take the value from the db, but, we already test this value multiple times in tests.
So, nevermind.
There was a problem hiding this comment.
@gfyrag That's a completely valid approach! You're absolutely right that using IgnoreExtras could mask missing assertions when new fields are added later. Your preference for explicit field checking ensures comprehensive test coverage.
The manual nulling approach you're using is a pragmatic solution for handling cases where you don't have the exact expected value available in the test context, especially when that value is already thoroughly tested elsewhere. It's a reasonable trade-off between test completeness and avoiding unnecessary complexity or duplication.
Thanks for the clarification - I understand your testing strategy now.
/| /|
( :v: )
|(_)|
/ ^ \
✏️ Learnings added
Learnt from: gfyrag
PR: formancehq/ledger#988
File: test/e2e/api_accounts_list_test.go:145-147
Timestamp: 2025-06-30T10:49:05.815Z
Learning: In test files, the user prefers explicit field checking over using Gomega's IgnoreExtras feature to prevent tests from silently passing when new fields are added without proper assertions. They use manual field nulling as a pragmatic solution when exact values aren't available in test context but are tested elsewhere.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: gfyrag
PR: formancehq/ledger#935
File: test/e2e/api_ledgers_import_test.go:338-352
Timestamp: 2025-05-20T13:08:10.034Z
Learning: The transactions list API for the ledger service has a stable ordering by ID that will never change, making it safe to rely on transactionsFromOriginalLedger.Data[0].ID in tests.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #988 +/- ##
==========================================
- Coverage 82.84% 82.82% -0.02%
==========================================
Files 145 145
Lines 8219 8229 +10
==========================================
+ Hits 6809 6816 +7
- Misses 1083 1085 +2
- Partials 327 328 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9a1f832 to
9f8dda9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
docs/events/CommittedTransactions.jsonis excluded by!**/*.jsondocs/events/RevertedTransaction.jsonis excluded by!**/*.jsonopenapi.yamlis excluded by!**/*.yamlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lockpkg/client/.speakeasy/logs/naming.logis excluded by!**/*.log,!**/*.logpkg/client/speakeasyusagegen/.speakeasy/logs/naming.logis excluded by!**/*.log,!**/*.log
📒 Files selected for processing (15)
docs/api/README.md(21 hunks)internal/README.md(22 hunks)internal/api/v2/controllers_bulk_test.go(1 hunks)internal/api/v2/views_test.go(15 hunks)internal/storage/ledger/resource_transactions.go(2 hunks)internal/storage/ledger/transactions.go(1 hunks)internal/storage/ledger/transactions_test.go(2 hunks)internal/transaction.go(2 hunks)pkg/client/docs/models/components/v2transaction.md(1 hunks)pkg/client/models/components/v2transaction.go(2 hunks)pkg/testserver/utils.go(1 hunks)test/e2e/api_bulk_test.go(1 hunks)test/e2e/api_transactions_create_test.go(4 hunks)test/e2e/api_transactions_list_test.go(1 hunks)test/e2e/api_transactions_revert_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- internal/api/v2/controllers_bulk_test.go
- pkg/client/docs/models/components/v2transaction.md
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/api/v2/views_test.go
- test/e2e/api_transactions_create_test.go
- internal/README.md
- docs/api/README.md
🧰 Additional context used
🧠 Learnings (4)
test/e2e/api_bulk_test.go (1)
Learnt from: gfyrag
PR: formancehq/ledger#935
File: test/e2e/api_ledgers_import_test.go:338-352
Timestamp: 2025-05-20T13:08:10.034Z
Learning: The transactions list API for the ledger service has a stable ordering by ID that will never change, making it safe to rely on transactionsFromOriginalLedger.Data[0].ID in tests.
test/e2e/api_transactions_list_test.go (1)
Learnt from: gfyrag
PR: formancehq/ledger#935
File: test/e2e/api_ledgers_import_test.go:338-352
Timestamp: 2025-05-20T13:08:10.034Z
Learning: The transactions list API for the ledger service has a stable ordering by ID that will never change, making it safe to rely on transactionsFromOriginalLedger.Data[0].ID in tests.
test/e2e/api_transactions_revert_test.go (1)
Learnt from: gfyrag
PR: formancehq/ledger#935
File: test/e2e/api_ledgers_import_test.go:338-352
Timestamp: 2025-05-20T13:08:10.034Z
Learning: The transactions list API for the ledger service has a stable ordering by ID that will never change, making it safe to rely on transactionsFromOriginalLedger.Data[0].ID in tests.
internal/storage/ledger/transactions_test.go (1)
Learnt from: gfyrag
PR: formancehq/ledger#935
File: test/e2e/api_ledgers_import_test.go:338-352
Timestamp: 2025-05-20T13:08:10.034Z
Learning: The transactions list API for the ledger service has a stable ordering by ID that will never change, making it safe to rely on transactionsFromOriginalLedger.Data[0].ID in tests.
🧬 Code Graph Analysis (3)
test/e2e/api_transactions_list_test.go (2)
pkg/client/models/operations/v2listtransactions.go (2)
V2ListTransactionsResponse(137-141)V2ListTransactionsRequest(37-61)pkg/client/models/components/v2transactionscursorresponse.go (1)
V2TransactionsCursorResponse(48-50)
internal/storage/ledger/resource_transactions.go (1)
internal/storage/common/schema.go (1)
NewDateField(67-69)
internal/storage/ledger/transactions_test.go (1)
internal/transaction.go (2)
Transaction(36-50)Transaction(52-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor BugBot
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (13)
test/e2e/api_bulk_test.go (1)
169-169: LGTM! Test correctly validates the UpdatedAt field.The test assertion is properly updated to include the
UpdatedAtfield in the expected transaction structure, ensuring the field is correctly populated and returned in API responses.pkg/client/models/components/v2transaction.go (2)
13-13: LGTM! UpdatedAt field properly added.The field is correctly defined as an optional timestamp with proper JSON serialization tag, consistent with other timestamp fields in the struct.
45-50: LGTM! GetUpdatedAt method follows established patterns.The getter method includes proper nil checks and follows the same pattern as other getter methods in the struct.
internal/transaction.go (2)
41-42: LGTM! Timestamp fields properly restructured.Moving
InsertedAtfromTransactionDatato theTransactionstruct and addingUpdatedAtat the same level improves accessibility and consistency. The JSON and Bun ORM tags are correctly configured.
221-225: LGTM! WithUpdatedAt method follows established patterns.The method implements the fluent interface pattern consistently used throughout the codebase for setting transaction fields.
pkg/testserver/utils.go (1)
14-16: LGTM! Function correctly updated for Transaction structure changes.The conversion function properly handles the relocation of
InsertedAtand addition ofUpdatedAtat the Transaction level, while maintaining the nested structure for other TransactionData fields.Also applies to: 24-25
internal/storage/ledger/transactions.go (1)
224-225: Prefixedtransaction_date()is functionally equivalent tonow() at time zone 'utc'.The
transaction_date()function is defined in migration11-make-stateless/up.sqland returnsstatement_timestamp() at time zone 'utc', matching the originalnow() at time zone 'utc'behavior. No further changes are needed here.internal/storage/ledger/resource_transactions.go (2)
26-27: LGTM: Timestamp fields added correctly to schemaThe addition of
inserted_atandupdated_atas paginated date fields is well-implemented and aligns with the PR objective of exposing insertion and update dates for transactions.
90-90: LGTM: Filter support added for timestamp fieldsThe filter resolution correctly includes the new timestamp fields alongside existing ones, enabling proper filtering capabilities for
inserted_atandupdated_at.test/e2e/api_transactions_revert_test.go (2)
159-163: LGTM: Event payload structure updated correctlyThe changes properly reflect the structural updates where
InsertedAtandUpdatedAtare now direct fields on theTransactionstruct, whileTimestampremains in the embeddedTransactionData. The test correctly uses response data to populate these timestamp fields.
199-203: LGTM: Reverted transaction event payload updated correctlyThe reverted transaction event payload structure is correctly updated to match the new model, with proper timestamp field positioning and values set from the appropriate response data.
internal/storage/ledger/transactions_test.go (2)
710-713: LGTM: Capturing updated transaction for proper timestamp trackingThe test now correctly captures the updated transaction returned by
UpdateTransactionMetadatato access the newUpdatedAttimestamp field. This ensures proper testing of the timestamp tracking functionality.
798-804: LGTM: Test expectation updated for UpdatedAt fieldThe test correctly updates the expected transaction's
UpdatedAtfield to reflect the timestamp from the metadata update operation. This properly validates that theUpdatedAtfield is maintained even when using Point-in-Time queries.
| When("listing transaction while paginating and filtering on insertion date", func() { | ||
| var ( | ||
| rsp *operations.V2ListTransactionsResponse | ||
| err error | ||
| ) | ||
| JustBeforeEach(func(specContext SpecContext) { | ||
| rsp, err = Wait(specContext, DeferClient(testServer)).Ledger.V2.ListTransactions( | ||
| ctx, | ||
| operations.V2ListTransactionsRequest{ | ||
| Ledger: "default", | ||
| PageSize: pointer.For(pageSize), | ||
| Expand: pointer.For("volumes,effectiveVolumes"), | ||
| Reverse: pointer.For(true), | ||
| Sort: pointer.For("inserted_at:desc"), | ||
| RequestBody: map[string]any{ | ||
| "$lte": map[string]any{ | ||
| "inserted_at": time.Now(), | ||
| }, | ||
| }, | ||
| }, | ||
| ) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| }) | ||
| It("Should be ok", func() { | ||
| Expect(rsp.V2TransactionsCursorResponse.Cursor.PageSize).To(Equal(pageSize)) | ||
| sortedByInsertionDate := transactions[:] | ||
| sort.SliceStable(sortedByInsertionDate, func(i, j int) bool { | ||
| return sortedByInsertionDate[i].Timestamp.Before(sortedByInsertionDate[j].Timestamp) | ||
| }) | ||
| page := sortedByInsertionDate[pageSize:] | ||
| slices.Reverse(page) | ||
| Expect(rsp.V2TransactionsCursorResponse.Cursor.Data).To(Equal(page)) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Fix test sorting logic for insertion date
The test is designed to validate filtering and sorting by insertion date, but the sorting logic is incorrect. The test sorts by inserted_at:desc in the request but uses Timestamp for the expected result sorting.
Apply this fix to use the correct field for sorting:
- sort.SliceStable(sortedByInsertionDate, func(i, j int) bool {
- return sortedByInsertionDate[i].Timestamp.Before(sortedByInsertionDate[j].Timestamp)
- })
+ sort.SliceStable(sortedByInsertionDate, func(i, j int) bool {
+ return sortedByInsertionDate[i].InsertedAt.Before(sortedByInsertionDate[j].InsertedAt)
+ })This ensures the test properly validates insertion date filtering and sorting functionality.
📝 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.
| When("listing transaction while paginating and filtering on insertion date", func() { | |
| var ( | |
| rsp *operations.V2ListTransactionsResponse | |
| err error | |
| ) | |
| JustBeforeEach(func(specContext SpecContext) { | |
| rsp, err = Wait(specContext, DeferClient(testServer)).Ledger.V2.ListTransactions( | |
| ctx, | |
| operations.V2ListTransactionsRequest{ | |
| Ledger: "default", | |
| PageSize: pointer.For(pageSize), | |
| Expand: pointer.For("volumes,effectiveVolumes"), | |
| Reverse: pointer.For(true), | |
| Sort: pointer.For("inserted_at:desc"), | |
| RequestBody: map[string]any{ | |
| "$lte": map[string]any{ | |
| "inserted_at": time.Now(), | |
| }, | |
| }, | |
| }, | |
| ) | |
| Expect(err).ToNot(HaveOccurred()) | |
| }) | |
| It("Should be ok", func() { | |
| Expect(rsp.V2TransactionsCursorResponse.Cursor.PageSize).To(Equal(pageSize)) | |
| sortedByInsertionDate := transactions[:] | |
| sort.SliceStable(sortedByInsertionDate, func(i, j int) bool { | |
| return sortedByInsertionDate[i].Timestamp.Before(sortedByInsertionDate[j].Timestamp) | |
| }) | |
| page := sortedByInsertionDate[pageSize:] | |
| slices.Reverse(page) | |
| Expect(rsp.V2TransactionsCursorResponse.Cursor.Data).To(Equal(page)) | |
| }) | |
| }) | |
| It("Should be ok", func() { | |
| Expect(rsp.V2TransactionsCursorResponse.Cursor.PageSize).To(Equal(pageSize)) | |
| sortedByInsertionDate := transactions[:] | |
| sort.SliceStable(sortedByInsertionDate, func(i, j int) bool { | |
| return sortedByInsertionDate[i].InsertedAt.Before(sortedByInsertionDate[j].InsertedAt) | |
| }) | |
| page := sortedByInsertionDate[pageSize:] | |
| slices.Reverse(page) | |
| Expect(rsp.V2TransactionsCursorResponse.Cursor.Data).To(Equal(page)) | |
| }) |
🤖 Prompt for AI Agents
In test/e2e/api_transactions_list_test.go around lines 159 to 192, the test
sorts the expected transactions by the Timestamp field, but the API request
sorts by inserted_at descending. Update the sorting logic in the test to sort
the expected transactions by the inserted_at field in descending order to match
the API request. This will align the test's expected results with the actual
sorting criteria used in the request.
There was a problem hiding this comment.
Bug: Test Sorts Incorrectly by Timestamp
The test "listing transaction while paginating and filtering on insertion date" is intended to verify sorting by inserted_at but incorrectly sorts by Timestamp in its verification logic. The sortedByInsertionDate slice uses Timestamp for comparison instead of InsertedAt, which is required to match the API's inserted_at:desc sort parameter.
test/e2e/api_transactions_list_test.go#L183-L190
ledger/test/e2e/api_transactions_list_test.go
Lines 183 to 190 in d985a5b
Bug: Test Coverage Loss for Transaction Revert
The TestTransactionsRevert test had its require.Equal assertion removed, which previously verified the correctness of a reverted transaction against the original (after normalizing fields like RevertedAt and PostCommitEffectiveVolumes). This removal eliminates critical test coverage for the transaction revert functionality, increasing the risk of hidden bugs and regressions.
internal/storage/ledger/transactions_test.go#L558-L574
ledger/internal/storage/ledger/transactions_test.go
Lines 558 to 574 in d985a5b
Bug: Incorrect Timestamp Assignment in RevertedTransaction
The UpdatedAt field for the RevertedTransaction is incorrectly assigned the InsertedAt value of the new transaction. It should instead use the UpdatedAt value for semantic correctness and consistency with the RevertTransaction struct.
test/e2e/api_transactions_revert_test.go#L201-L202
ledger/test/e2e/api_transactions_revert_test.go
Lines 201 to 202 in d985a5b
Was this report helpful? Give feedback by reacting with 👍 or 👎
No description provided.