feat: limit model with WEEKLY, CUSTOM types and time window support#69
Open
alexgarzao wants to merge 1 commit intodevelopfrom
Open
feat: limit model with WEEKLY, CUSTOM types and time window support#69alexgarzao wants to merge 1 commit intodevelopfrom
alexgarzao wants to merge 1 commit intodevelopfrom
Conversation
Add domain constructors and validation for new limit types: - NewLimitWithTimeWindow, NewLimitWithCustomPeriod, NewLimitWithCustomPeriodAndTimeWindow constructors - Time window validation (mismatch, zero-width, overnight support) - Custom period validation (dates required, order, max 5 years) - WEEKLY resetAt calculation (next Monday 00:00 UTC) - DB model conversion for new columns (active_time_start/end, custom_start_date/end_date) - Create/update command handlers for new limit types - MustNewTimeOfDay test helper in internal/testhelper Note: CalculatePeriodKey for WEEKLY/CUSTOM will be added in PR 3 (validation pipeline) along with its tests.
WalkthroughThis PR extends the limit model with time window and custom period support. Four new database fields (ActiveTimeStart, ActiveTimeEnd, CustomStartDate, CustomEndDate) are added to the PostgreSQL schema and repository queries. The domain model introduces WEEKLY and CUSTOM LimitType values, adds validation for time windows and custom periods, and provides methods to check if timestamps fall within these constraints. Limit creation now supports four constructor paths based on the presence of time window and/or custom period. The update flow is enhanced to modify these fields with proper validation and ResetAt recalculation. Comprehensive test coverage validates all combinations and edge cases. Sequence DiagramsequenceDiagram
participant Client
participant CreateCmd as CreateLimitCommand
participant Validator as Validation
participant Constructor as Model Constructor
participant Repository
participant Database
Client->>CreateCmd: Execute(CreateLimitInput)
CreateCmd->>Validator: Check time window/custom period presence
alt Partial time window
Validator-->>CreateCmd: ErrLimitTimeWindowMismatch
CreateCmd-->>Client: Error
else Partial custom period
Validator-->>CreateCmd: ErrLimitCustomDatesRequired
CreateCmd-->>Client: Error
else Standard limit
Validator->>Constructor: NewLimit()
Constructor->>Constructor: Initialize core fields
Constructor-->>CreateCmd: *Limit
else Time window only
Validator->>Constructor: NewLimitWithTimeWindow()
Constructor->>Constructor: Validate & set ActiveTimeStart/End
Constructor-->>CreateCmd: *Limit
else Custom period only
Validator->>Constructor: NewLimitWithCustomPeriod()
Constructor->>Constructor: Parse & validate CustomStartDate/End
Constructor->>Constructor: Calculate ResetAt
Constructor-->>CreateCmd: *Limit
else Custom period + time window
Validator->>Constructor: NewLimitWithCustomPeriodAndTimeWindow()
Constructor->>Constructor: Validate both time window & custom period
Constructor->>Constructor: Calculate ResetAt for CUSTOM
Constructor-->>CreateCmd: *Limit
end
CreateCmd->>Repository: Create(limit)
Repository->>Database: INSERT with all fields
Database-->>Repository: Success
Repository-->>CreateCmd: Limit with ID
CreateCmd-->>Client: Created limit
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
gandalf-at-lerian
approved these changes
Mar 16, 2026
gandalf-at-lerian
left a comment
gandalf-at-lerian
left a comment
There was a problem hiding this comment.
Solid PR. The model decomposition is clean — newLimitBase + specialized constructors avoids repetition without over-abstracting. A few observations:
Half-open interval consistency — both IsWithinTimeWindow and IsWithinCustomPeriod use [start, end) semantics, well-documented and well-tested. The overnight window logic (startMins >= endMins path) is correct. Good.
Update partial semantics — if someone sends only customEndDate (without customStartDate), ValidateCustomPeriod catches it as ErrLimitCustomDatesRequired. Same for time windows. This means partial updates require both fields in the pair, which is the right call for data integrity. Worth a note in the API docs (PR 6) that these are atomic pairs.
Minor asymmetry — CreateLimitInput takes *model.TimeOfDay for time window fields (pre-parsed) but *string for custom dates (parsed in command layer). Not a problem — TimeOfDay is a value object, custom dates are standard RFC3339 — but worth noting for consistency if the team prefers one pattern.
Validate() skips expiry check (comment says "done in constructors") — correct, avoids breaking reads of historical data from DB. Clean separation of creation-time vs read-time validation.
Test coverage is thorough — boundary conditions, overnight windows, partial inputs, defensive nil checks. LGTM 👍
|
This PR is very large (11 files, 2243 lines changed). Consider breaking it into smaller PRs for easier review. |
🔒 Security Scan Results —
|
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 83.1% |
| Threshold | 85% |
Coverage by Package
| Package | Coverage |
|---|---|
tracer/internal/adapters/cel |
81.9% |
tracer/internal/adapters/http/in/middleware |
62.0% |
tracer/internal/adapters/http/in |
81.8% |
tracer/internal/adapters/postgres/db |
0.0% |
tracer/internal/adapters/postgres |
74.9% |
tracer/internal/services/cache |
95.6% |
tracer/internal/services/command |
81.5% |
tracer/internal/services/query |
78.3% |
tracer/internal/services/workers |
79.7% |
tracer/internal/services |
40.2% |
tracer/internal/testhelper |
0.0% |
tracer/pkg/clock |
50.0% |
tracer/pkg/contextutil |
100.0% |
tracer/pkg/logging |
100.0% |
tracer/pkg/migration |
89.0% |
tracer/pkg/model |
96.1% |
tracer/pkg/net/http |
88.3% |
tracer/pkg/resilience |
93.2% |
tracer/pkg/sanitize |
87.1% |
tracer/pkg/validation |
50.0% |
tracer/pkg |
96.6% |
Generated by Go PR Analysis workflow
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/adapters/postgres/limit_repository.go (1)
321-331:⚠️ Potential issue | 🟠 MajorPersist
reset_atwhen these fields change.
limit.Update(...)can recalculateResetAtfor custom-period edits, but thisUPDATEnever writes that field back. After a successful update, the in-memory entity and stored row can diverge, leaving the database with a stale reset boundary.💡 Suggested fix
query := sq.Update(r.tableName). Set("name", dbModel.Name). Set("description", dbModel.Description). Set("max_amount", dbModel.MaxAmount). Set("scopes", dbModel.Scopes). Set("status", dbModel.Status). + Set("reset_at", dbModel.ResetAt). Set("active_time_start", dbModel.ActiveTimeStart). Set("active_time_end", dbModel.ActiveTimeEnd). Set("custom_start_date", dbModel.CustomStartDate). Set("custom_end_date", dbModel.CustomEndDate). Set("updated_at", dbModel.UpdatedAt).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapters/postgres/limit_repository.go` around lines 321 - 331, The UPDATE builder for r.tableName is missing the reset_at column, so when limit.Update(...) recalculates dbModel.ResetAt for custom-period edits the database isn't updated; add Set("reset_at", dbModel.ResetAt) to the sq.Update(...) chain (where query is built using r.tableName and dbModel) so the recalculated ResetAt is persisted alongside name, description, max_amount, scopes, status, active_time_start/end, custom_start/end and updated_at.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/adapters/postgres/limit_postgresql_model.go`:
- Around line 98-146: After constructing ActiveTimeStart/ActiveTimeEnd and
CustomStartDate/CustomEndDate but before returning the &model.Limit, validate
the paired-field invariants: ensure ActiveTimeStart and ActiveTimeEnd are either
both non-nil or both nil, and likewise CustomStartDate and CustomEndDate; if one
side is set without its pair return a descriptive error. Also validate ordering
(ActiveTimeStart <= ActiveTimeEnd and CustomStartDate <= CustomEndDate) and any
domain span limits your domain requires (e.g., max date span or max time
window); if any check fails return an error instead of the model. Use the
existing variables ActiveTimeStart, ActiveTimeEnd, customStartDate,
customEndDate and the surrounding mapper function that builds and returns
*model.Limit to locate where to insert these checks.
In `@internal/services/command/create_limit_test.go`:
- Around line 495-620: Add a happy-path test that exercises non-nil
ActiveTimeStart/ActiveTimeEnd and CustomStartDate/CustomEndDate to verify
command-layer parsing and repository handoff: in a new subtest (alongside
TestCreateLimitCommand_Execute_PartialCustomPeriod and
TestCreateLimitCommand_Execute_PartialTimeWindow) create a CreateLimitInput with
both ActiveTimeStart and ActiveTimeEnd set and both CustomStartDate and
CustomEndDate set, construct NewCreateLimitCommand with NewMockLimitRepository
and NewMockAuditWriter, set expectations that the repository's create/save
method (the method on NewMockLimitRepository used by Execute) is called once
with a limit containing the supplied times/dates and that
auditWriter.RecordLimitEvent is called once, call
cmd.Execute(context.Background(), input) and assert no error and non-nil result;
ensure you reference CreateLimitInput, ActiveTimeStart, ActiveTimeEnd,
CustomStartDate, CustomEndDate and Execute to locate where to hook the
expectations.
In `@internal/testhelper/time_of_day.go`:
- Around line 5-21: Move this helper into the internal/testutil package and
replace the panic-only API with a non-panicking constructor plus a thin Must
wrapper: implement a function (e.g. TimeOfDayFromString or NewTimeOfDayForTest)
that calls model.NewTimeOfDay and returns (model.TimeOfDay, error), then
reimplement MustNewTimeOfDay to call that function and panic only on error;
update package name from testhelper to testutil and update call sites to use the
new package and the non-panicking variant where appropriate.
In `@pkg/model/limit_weekly_custom_test.go`:
- Around line 540-604: The subtests in TestLimit_GetAPIResponse only inspect the
in-memory Limit fields and do not verify the serialized API contract; update
each subtest to produce and assert the actual response payload by either (a)
calling the model's response mapper (e.g., a function like Limit.ToAPIResponse /
BuildLimitResponse if one exists) and asserting the returned structure contains
the expected keys/values for LimitType (LimitTypeWeekly, LimitTypeCustom),
CustomStartDate, CustomEndDate, ResetAt, ActiveTimeStart, ActiveTimeEnd, etc.,
or (b) json.Marshal the response object and assert the resulting JSON
string/object contains the expected serialized keys and formatted values (e.g.,
"limit_type", "custom_start_date", "custom_end_date", "active_time_start",
"active_time_end", "reset_at") instead of only checking the in-memory fields on
the Limit struct.
- Around line 375-382: Add a table test that verifies the inclusive 5-year
boundary: insert a case (e.g. name "accepts period equal to 5 years") using
LimitTypeCustom where endDate is startDate.AddDate(5,0,0) (use testutil.Ptr for
both dates), set now to a date within the range, and assert expectError: false;
keep the existing failing case that uses constant.ErrLimitCustomPeriodTooLong to
ensure one-over-limit still fails; reference the
MaxCustomPeriodYears/LimitTypeCustom logic and the ErrLimitCustomPeriodTooLong
constant when locating where to add the new row.
In `@pkg/model/limit.go`:
- Around line 574-583: The Update branch never clears an existing window because
it only runs when at least one of activeTimeStart/activeTimeEnd is non-nil; add
an explicit clear flag (e.g., clearActiveWindow) to the Limit.Update signature
and handle it before the existing branch: if clearActiveWindow is true set
l.ActiveTimeStart = nil and l.ActiveTimeEnd = nil and mark updated = true;
otherwise keep the current logic (call ValidateTimeWindow when one or both
bounds provided and assign l.ActiveTimeStart/l.ActiveTimeEnd). Ensure
ValidateTimeWindow is still used when assigning new bounds and update any
callers to pass the new clear flag where a caller intends to remove the time
window.
- Around line 173-179: The CUSTOM period handling is inconsistent:
IsWithinCustomPeriod uses the full timestamp while ValidateCustomPeriod and
CalculateCustomResetAt round to day; pick a single behavior—normalize
CustomEndDate to UTC midnight everywhere. Update ValidateCustomPeriod to
truncate CustomEndDate.UTC() to 24*time.Hour (zeroing time) when validating,
update IsWithinCustomPeriod to compare against that truncated date (so the
period is inclusive/exclusive consistently), and change CalculateCustomResetAt
to return truncatedCustomEndDate.AddDate(0,0,1) (midnight UTC of the next day).
Apply the same truncation logic to any other uses (e.g., the other functions
noted) and adjust tests accordingly.
---
Outside diff comments:
In `@internal/adapters/postgres/limit_repository.go`:
- Around line 321-331: The UPDATE builder for r.tableName is missing the
reset_at column, so when limit.Update(...) recalculates dbModel.ResetAt for
custom-period edits the database isn't updated; add Set("reset_at",
dbModel.ResetAt) to the sq.Update(...) chain (where query is built using
r.tableName and dbModel) so the recalculated ResetAt is persisted alongside
name, description, max_amount, scopes, status, active_time_start/end,
custom_start/end and updated_at.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 150ce703-d7cf-4c97-ad1d-8411c45429a2
📒 Files selected for processing (11)
internal/adapters/postgres/limit_postgresql_model.gointernal/adapters/postgres/limit_repository.gointernal/adapters/postgres/limit_repository_test.gointernal/services/command/create_limit.gointernal/services/command/create_limit_test.gointernal/services/command/update_limit.gointernal/services/command/update_limit_timewindow_test.gointernal/testhelper/time_of_day.gopkg/model/limit.gopkg/model/limit_test.gopkg/model/limit_weekly_custom_test.go
Comment on lines
+98
to
146
| // Convert time windows from database strings to TimeOfDay | ||
| var activeTimeStart, activeTimeEnd *model.TimeOfDay | ||
|
|
||
| if m.ActiveTimeStart.Valid { | ||
| parsed, err := model.NewTimeOfDay(m.ActiveTimeStart.String) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid active_time_start in database: %w", err) | ||
| } | ||
|
|
||
| activeTimeStart = &parsed | ||
| } | ||
|
|
||
| if m.ActiveTimeEnd.Valid { | ||
| parsed, err := model.NewTimeOfDay(m.ActiveTimeEnd.String) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid active_time_end in database: %w", err) | ||
| } | ||
|
|
||
| activeTimeEnd = &parsed | ||
| } | ||
|
|
||
| // Convert custom period dates | ||
| var customStartDate, customEndDate *time.Time | ||
| if m.CustomStartDate.Valid { | ||
| customStartDate = &m.CustomStartDate.Time | ||
| } | ||
|
|
||
| if m.CustomEndDate.Valid { | ||
| customEndDate = &m.CustomEndDate.Time | ||
| } | ||
|
|
||
| return &model.Limit{ | ||
| ID: id, | ||
| Name: m.Name, | ||
| Description: description, | ||
| LimitType: limitType, | ||
| MaxAmount: m.MaxAmount, | ||
| Currency: m.Currency, | ||
| Scopes: scopes, | ||
| Status: status, | ||
| ResetAt: resetAt, | ||
| CreatedAt: m.CreatedAt, | ||
| UpdatedAt: m.UpdatedAt, | ||
| DeletedAt: deletedAt, | ||
| ID: id, | ||
| Name: m.Name, | ||
| Description: description, | ||
| LimitType: limitType, | ||
| MaxAmount: m.MaxAmount, | ||
| Currency: m.Currency, | ||
| Scopes: scopes, | ||
| Status: status, | ||
| ResetAt: resetAt, | ||
| ActiveTimeStart: activeTimeStart, | ||
| ActiveTimeEnd: activeTimeEnd, | ||
| CustomStartDate: customStartDate, | ||
| CustomEndDate: customEndDate, | ||
| CreatedAt: m.CreatedAt, | ||
| UpdatedAt: m.UpdatedAt, | ||
| DeletedAt: deletedAt, | ||
| }, nil |
There was a problem hiding this comment.
Revalidate the new period/window invariants before returning the entity.
This mapper now rebuilds ActiveTime* and Custom*Date independently, but it never checks paired-field requirements, date ordering, or span limits. A row with only one side set will still deserialize into a model.Limit and leak invalid state past the adapter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/adapters/postgres/limit_postgresql_model.go` around lines 98 - 146,
After constructing ActiveTimeStart/ActiveTimeEnd and
CustomStartDate/CustomEndDate but before returning the &model.Limit, validate
the paired-field invariants: ensure ActiveTimeStart and ActiveTimeEnd are either
both non-nil or both nil, and likewise CustomStartDate and CustomEndDate; if one
side is set without its pair return a descriptive error. Also validate ordering
(ActiveTimeStart <= ActiveTimeEnd and CustomStartDate <= CustomEndDate) and any
domain span limits your domain requires (e.g., max date span or max time
window); if any check fails return an error instead of the model. Use the
existing variables ActiveTimeStart, ActiveTimeEnd, customStartDate,
customEndDate and the surrounding mapper function that builds and returns
*model.Limit to locate where to insert these checks.
Comment on lines
+495
to
+620
| func TestCreateLimitCommand_Execute_PartialCustomPeriod(t *testing.T) { | ||
| validScope := model.Scope{ | ||
| AccountID: testutil.UUIDPtr(testutil.MustDeterministicUUID(1)), | ||
| } | ||
|
|
||
| startDate := "2026-11-27T00:00:00Z" | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| limitType model.LimitType | ||
| customStartDate *string | ||
| customEndDate *string | ||
| errorIs error | ||
| }{ | ||
| { | ||
| name: "CUSTOM with only customStartDate", | ||
| limitType: model.LimitTypeCustom, | ||
| customStartDate: &startDate, | ||
| customEndDate: nil, | ||
| errorIs: constant.ErrLimitCustomDatesRequired, | ||
| }, | ||
| { | ||
| name: "CUSTOM with only customEndDate", | ||
| limitType: model.LimitTypeCustom, | ||
| customStartDate: nil, | ||
| customEndDate: &startDate, | ||
| errorIs: constant.ErrLimitCustomDatesRequired, | ||
| }, | ||
| { | ||
| name: "DAILY with only customStartDate rejected", | ||
| limitType: model.LimitTypeDaily, | ||
| customStartDate: &startDate, | ||
| customEndDate: nil, | ||
| errorIs: constant.ErrLimitCustomDatesRequired, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| ctrl := gomock.NewController(t) | ||
|
|
||
| mockRepo := NewMockLimitRepository(ctrl) | ||
| auditWriter := NewMockAuditWriter(ctrl) | ||
|
|
||
| auditWriter.EXPECT(). | ||
| RecordLimitEvent(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). | ||
| Times(0) | ||
|
|
||
| cmd, cmdErr := NewCreateLimitCommand(mockRepo, testutil.NewDefaultMockClock(), auditWriter) | ||
| require.NoError(t, cmdErr) | ||
|
|
||
| input := &CreateLimitInput{ | ||
| Name: "Test Partial Custom", | ||
| LimitType: tc.limitType, | ||
| MaxAmount: decimal.RequireFromString("1000"), | ||
| Currency: "USD", | ||
| Scopes: []model.Scope{validScope}, | ||
| CustomStartDate: tc.customStartDate, | ||
| CustomEndDate: tc.customEndDate, | ||
| } | ||
|
|
||
| result, err := cmd.Execute(context.Background(), input) | ||
|
|
||
| require.Error(t, err, "Partial custom period should be rejected") | ||
| assert.ErrorIs(t, err, tc.errorIs) | ||
| assert.Nil(t, result) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestCreateLimitCommand_Execute_PartialTimeWindow(t *testing.T) { | ||
| validScope := model.Scope{ | ||
| AccountID: testutil.UUIDPtr(testutil.MustDeterministicUUID(1)), | ||
| } | ||
|
|
||
| startTime := testhelper.MustNewTimeOfDay("09:00") | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| activeTimeStart *model.TimeOfDay | ||
| activeTimeEnd *model.TimeOfDay | ||
| }{ | ||
| { | ||
| name: "only activeTimeStart provided", | ||
| activeTimeStart: &startTime, | ||
| activeTimeEnd: nil, | ||
| }, | ||
| { | ||
| name: "only activeTimeEnd provided", | ||
| activeTimeStart: nil, | ||
| activeTimeEnd: &startTime, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| ctrl := gomock.NewController(t) | ||
|
|
||
| mockRepo := NewMockLimitRepository(ctrl) | ||
| auditWriter := NewMockAuditWriter(ctrl) | ||
|
|
||
| auditWriter.EXPECT(). | ||
| RecordLimitEvent(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). | ||
| Times(0) | ||
|
|
||
| cmd, cmdErr := NewCreateLimitCommand(mockRepo, testutil.NewDefaultMockClock(), auditWriter) | ||
| require.NoError(t, cmdErr) | ||
|
|
||
| input := &CreateLimitInput{ | ||
| Name: "Test Partial TimeWindow", | ||
| LimitType: model.LimitTypeDaily, | ||
| MaxAmount: decimal.RequireFromString("1000"), | ||
| Currency: "USD", | ||
| Scopes: []model.Scope{validScope}, | ||
| ActiveTimeStart: tc.activeTimeStart, | ||
| ActiveTimeEnd: tc.activeTimeEnd, | ||
| } | ||
|
|
||
| result, err := cmd.Execute(context.Background(), input) | ||
|
|
||
| require.Error(t, err, "Partial time window should be rejected") | ||
| assert.ErrorIs(t, err, constant.ErrLimitTimeWindowMismatch) | ||
| assert.Nil(t, result) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a happy-path create case for the new fields.
These new tests only exercise mismatch validation. The command-layer parsing and repository handoff for non-nil ActiveTime* / Custom*Date values is still unverified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/services/command/create_limit_test.go` around lines 495 - 620, Add a
happy-path test that exercises non-nil ActiveTimeStart/ActiveTimeEnd and
CustomStartDate/CustomEndDate to verify command-layer parsing and repository
handoff: in a new subtest (alongside
TestCreateLimitCommand_Execute_PartialCustomPeriod and
TestCreateLimitCommand_Execute_PartialTimeWindow) create a CreateLimitInput with
both ActiveTimeStart and ActiveTimeEnd set and both CustomStartDate and
CustomEndDate set, construct NewCreateLimitCommand with NewMockLimitRepository
and NewMockAuditWriter, set expectations that the repository's create/save
method (the method on NewMockLimitRepository used by Execute) is called once
with a limit containing the supplied times/dates and that
auditWriter.RecordLimitEvent is called once, call
cmd.Execute(context.Background(), input) and assert no error and non-nil result;
ensure you reference CreateLimitInput, ActiveTimeStart, ActiveTimeEnd,
CustomStartDate, CustomEndDate and Execute to locate where to hook the
expectations.
Comment on lines
+5
to
+21
| package testhelper | ||
|
|
||
| import ( | ||
| "fmt" | ||
|
|
||
| "tracer/pkg/model" | ||
| ) | ||
|
|
||
| // MustNewTimeOfDay creates a model.TimeOfDay from a string or panics. | ||
| // Use only in tests where invalid input indicates a bug. | ||
| func MustNewTimeOfDay(s string) model.TimeOfDay { | ||
| tod, err := model.NewTimeOfDay(s) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("MustNewTimeOfDay(%q): %v", s, err)) | ||
| } | ||
|
|
||
| return tod |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Move this helper into internal/testutil and add a non-panicking variant.
Shared test helpers here are centralized under internal/testutil, and helpers that can fail should expose (model.TimeOfDay, error) with Must* as a thin wrapper. The current shape forks the helper namespace and forces panic-only call sites.
Based on learnings: Place shared test helpers in internal/testutil package; test helper functions that can fail should return errors instead of panicking, with Must* wrapper for test setup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/testhelper/time_of_day.go` around lines 5 - 21, Move this helper
into the internal/testutil package and replace the panic-only API with a
non-panicking constructor plus a thin Must wrapper: implement a function (e.g.
TimeOfDayFromString or NewTimeOfDayForTest) that calls model.NewTimeOfDay and
returns (model.TimeOfDay, error), then reimplement MustNewTimeOfDay to call that
function and panic only on error; update package name from testhelper to
testutil and update call sites to use the new package and the non-panicking
variant where appropriate.
Comment on lines
+375
to
+382
| name: "rejects period exceeding 5 years", | ||
| limitType: LimitTypeCustom, | ||
| startDate: testutil.Ptr(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)), | ||
| endDate: testutil.Ptr(time.Date(2031, 1, 1, 0, 0, 0, 0, time.UTC)), // > 5 years | ||
| now: time.Date(2025, 2, 15, 0, 0, 0, 0, time.UTC), | ||
| expectError: true, | ||
| errorIs: constant.ErrLimitCustomPeriodTooLong, | ||
| }, |
There was a problem hiding this comment.
Add the exact 5-year pass case.
This table locks down the failing side of MaxCustomPeriodYears, but not the inclusive boundary. Please add a case where endDate == startDate.AddDate(5, 0, 0) so a future change cannot accidentally make the limit exclusive. Based on learnings: Test boundary conditions: exactly at limit (pass), one over limit (fail), zero/empty values, negative values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/model/limit_weekly_custom_test.go` around lines 375 - 382, Add a table
test that verifies the inclusive 5-year boundary: insert a case (e.g. name
"accepts period equal to 5 years") using LimitTypeCustom where endDate is
startDate.AddDate(5,0,0) (use testutil.Ptr for both dates), set now to a date
within the range, and assert expectError: false; keep the existing failing case
that uses constant.ErrLimitCustomPeriodTooLong to ensure one-over-limit still
fails; reference the MaxCustomPeriodYears/LimitTypeCustom logic and the
ErrLimitCustomPeriodTooLong constant when locating where to add the new row.
Comment on lines
+540
to
+604
| // TestLimit_GetAPIResponse tests that new fields are included in API response. | ||
| func TestLimit_GetAPIResponse(t *testing.T) { | ||
| validScope := Scope{AccountID: testutil.UUIDPtr(testutil.MustDeterministicUUID(120))} | ||
| fixedTime := testutil.FixedTime() | ||
|
|
||
| t.Run("includes WEEKLY type in response", func(t *testing.T) { | ||
| limit := &Limit{ | ||
| ID: testutil.MustDeterministicUUID(121), | ||
| Name: "Weekly Limit", | ||
| LimitType: LimitTypeWeekly, | ||
| MaxAmount: decimal.RequireFromString("5000"), | ||
| Currency: "USD", | ||
| Scopes: []Scope{validScope}, | ||
| Status: LimitStatusActive, | ||
| ResetAt: testutil.Ptr(fixedTime.AddDate(0, 0, 7)), | ||
| CreatedAt: fixedTime, | ||
| UpdatedAt: fixedTime, | ||
| } | ||
|
|
||
| assert.Equal(t, LimitTypeWeekly, limit.LimitType) | ||
| }) | ||
|
|
||
| t.Run("includes CUSTOM type with dates in response", func(t *testing.T) { | ||
| limit := &Limit{ | ||
| ID: testutil.MustDeterministicUUID(122), | ||
| Name: "Custom Limit", | ||
| LimitType: LimitTypeCustom, | ||
| MaxAmount: decimal.RequireFromString("100000"), | ||
| Currency: "USD", | ||
| Scopes: []Scope{validScope}, | ||
| Status: LimitStatusActive, | ||
| CustomStartDate: testutil.Ptr(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)), | ||
| CustomEndDate: testutil.Ptr(time.Date(2025, 3, 31, 0, 0, 0, 0, time.UTC)), | ||
| ResetAt: testutil.Ptr(time.Date(2025, 4, 1, 0, 0, 0, 0, time.UTC)), | ||
| CreatedAt: fixedTime, | ||
| UpdatedAt: fixedTime, | ||
| } | ||
|
|
||
| assert.Equal(t, LimitTypeCustom, limit.LimitType) | ||
| require.NotNil(t, limit.CustomStartDate) | ||
| require.NotNil(t, limit.CustomEndDate) | ||
| }) | ||
|
|
||
| t.Run("includes time window fields in response", func(t *testing.T) { | ||
| limit := &Limit{ | ||
| ID: testutil.MustDeterministicUUID(123), | ||
| Name: "Time Window Limit", | ||
| LimitType: LimitTypeDaily, | ||
| MaxAmount: decimal.RequireFromString("1000"), | ||
| Currency: "USD", | ||
| Scopes: []Scope{validScope}, | ||
| Status: LimitStatusActive, | ||
| ActiveTimeStart: testutil.Ptr(mustNewTimeOfDay("20:00")), | ||
| ActiveTimeEnd: testutil.Ptr(mustNewTimeOfDay("06:00")), | ||
| ResetAt: testutil.Ptr(fixedTime.AddDate(0, 0, 1)), | ||
| CreatedAt: fixedTime, | ||
| UpdatedAt: fixedTime, | ||
| } | ||
|
|
||
| require.NotNil(t, limit.ActiveTimeStart) | ||
| require.NotNil(t, limit.ActiveTimeEnd) | ||
| assert.Equal(t, "20:00", limit.ActiveTimeStart.String()) | ||
| assert.Equal(t, "06:00", limit.ActiveTimeEnd.String()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
This test never exercises the response contract.
These subtests only re-read fields from the in-memory Limit, so broken JSON tags or a broken response mapper would still pass. Marshal the payload or call the actual response builder and assert the serialized keys/values for the new fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/model/limit_weekly_custom_test.go` around lines 540 - 604, The subtests
in TestLimit_GetAPIResponse only inspect the in-memory Limit fields and do not
verify the serialized API contract; update each subtest to produce and assert
the actual response payload by either (a) calling the model's response mapper
(e.g., a function like Limit.ToAPIResponse / BuildLimitResponse if one exists)
and asserting the returned structure contains the expected keys/values for
LimitType (LimitTypeWeekly, LimitTypeCustom), CustomStartDate, CustomEndDate,
ResetAt, ActiveTimeStart, ActiveTimeEnd, etc., or (b) json.Marshal the response
object and assert the resulting JSON string/object contains the expected
serialized keys and formatted values (e.g., "limit_type", "custom_start_date",
"custom_end_date", "active_time_start", "active_time_end", "reset_at") instead
of only checking the in-memory fields on the Limit struct.
Comment on lines
+173
to
+179
| // CalculateCustomResetAt computes reset time for CUSTOM limits. | ||
| // Returns customEndDate + 1 day at midnight UTC. | ||
| func CalculateCustomResetAt(customEndDate time.Time) *time.Time { | ||
| resetAt := customEndDate.UTC().Truncate(24*time.Hour).AddDate(0, 0, 1) | ||
|
|
||
| return &resetAt | ||
| } |
There was a problem hiding this comment.
Make CUSTOM period handling consistent.
IsWithinCustomPeriod treats CustomEndDate as an exact exclusive instant, but ValidateCustomPeriod and CalculateCustomResetAt round it to the day. A limit ending at 2025-06-15T08:00:00Z still validates at noon and will not reset until midnight, even though it stopped matching at 08:00. Either normalize custom periods to date-only boundaries everywhere or compare/reset against the full timestamp everywhere.
Also applies to: 478-505, 703-715
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/model/limit.go` around lines 173 - 179, The CUSTOM period handling is
inconsistent: IsWithinCustomPeriod uses the full timestamp while
ValidateCustomPeriod and CalculateCustomResetAt round to day; pick a single
behavior—normalize CustomEndDate to UTC midnight everywhere. Update
ValidateCustomPeriod to truncate CustomEndDate.UTC() to 24*time.Hour (zeroing
time) when validating, update IsWithinCustomPeriod to compare against that
truncated date (so the period is inclusive/exclusive consistently), and change
CalculateCustomResetAt to return truncatedCustomEndDate.AddDate(0,0,1) (midnight
UTC of the next day). Apply the same truncation logic to any other uses (e.g.,
the other functions noted) and adjust tests accordingly.
Comment on lines
+574
to
+583
| // Update time window if both fields provided (must be together or both nil) | ||
| if activeTimeStart != nil || activeTimeEnd != nil { | ||
| if err := ValidateTimeWindow(activeTimeStart, activeTimeEnd); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| l.ActiveTimeStart = activeTimeStart | ||
| l.ActiveTimeEnd = activeTimeEnd | ||
| updated = true | ||
| } |
There was a problem hiding this comment.
Update can't remove an existing time window.
This branch only runs when at least one bound is non-nil, so passing both activeTimeStart and activeTimeEnd as nil leaves the old window in place. Once a limit has a window, Limit.Update has no way to return it to 24/7 behavior without a tri-state field or an explicit clear flag.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/model/limit.go` around lines 574 - 583, The Update branch never clears an
existing window because it only runs when at least one of
activeTimeStart/activeTimeEnd is non-nil; add an explicit clear flag (e.g.,
clearActiveWindow) to the Limit.Update signature and handle it before the
existing branch: if clearActiveWindow is true set l.ActiveTimeStart = nil and
l.ActiveTimeEnd = nil and mark updated = true; otherwise keep the current logic
(call ValidateTimeWindow when one or both bounds provided and assign
l.ActiveTimeStart/l.ActiveTimeEnd). Ensure ValidateTimeWindow is still used when
assigning new bounds and update any callers to pass the new clear flag where a
caller intends to remove the time window.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Overview
This pull request extends the
Limitmodel and its PostgreSQL repository to support new time window and custom period fields:ActiveTimeStart,ActiveTimeEnd,CustomStartDate, andCustomEndDate. These fields are now handled throughout the data access layer, including model conversion, repository CRUD operations, and related tests. The changes ensure that these new fields are properly persisted, queried, and tested.Model and Data Layer Enhancements:
ActiveTimeStart,ActiveTimeEnd,CustomStartDate,CustomEndDate) toLimitPostgreSQLModeland updated its conversion methods to map these fields between the database and the domain model. [1] [2] [3] [4]Create,Update,GetByID,List, and scan helpers) to include the new fields in SQL queries and mapping logic. [1] [2] [3] [4] [5] [6]Testing Improvements:
Service Layer Update:
CreateLimitInputstruct in the command service to accept the new fields, preparing for their use in business logic.Other Minor Changes:
Add domain constructors and validation for new limit types:
Note: CalculatePeriodKey for WEEKLY/CUSTOM will be added in PR 3 (validation pipeline) along with its tests.
Pull Request Checklist
Pull Request Type
Checklist
Please check each item after it's completed.
Additional Notes
Obs: Please, always remember to target your PR to develop branch instead of main.