fix(employee): use latest payout for check-in#805
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in office check-in eligibility determination by changing from using the earliest (first) payout date to using the most recent (latest) payout date. This ensures that contractors are eligible for office check-ins based on recent payment activity within the last 45 days, rather than just having received a payment at any point in the past.
Changes:
- Renamed
GetFirstPayoutDateByDiscordtoGetLatestPayoutDateByDiscordand updated the query to sort dates in descending order - Added logic to scan all returned payout records and select the maximum date
- Updated test suite to verify the function correctly identifies the latest date even when results are returned out of order
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/service/notion/contractor_payouts.go | Modified query to sort by date descending and added loop to find latest payout date |
| pkg/service/notion/contractor_payouts_test.go | Renamed test functions, added second payout entry to test data, and updated assertions |
| pkg/handler/employee/office_checkin_eligibility.go | Renamed eligibility check function from isEligibleByFirstPayout to isEligibleByLatestPayout |
| pkg/handler/employee/office_checkin_eligibility_test.go | Updated test function names and calls to match renamed function |
| pkg/handler/employee/employee.go | Updated variable names and function calls to use latest payout instead of first payout |
Comments suppressed due to low confidence (1)
pkg/service/notion/contractor_payouts_test.go:100
- The test data order doesn't match the expected API behavior. Since the query sorts by Date in descending order (line 71), the API should return the latest date first (page-1), not the older date. The test should either: (1) reorder the results to have latestDt in page-1 and olderDt in page-2, matching the DESC sort, or (2) if testing defensive behavior for API inconsistencies, add a comment explaining why the test data intentionally contradicts the sort order.
resp := nt.DatabaseQueryResponse{
Results: []nt.Page{
{
ID: "page-1",
Parent: nt.Parent{
Type: nt.ParentTypeDatabase,
DatabaseID: dbID,
},
Properties: nt.DatabasePageProperties{
"Date": nt.DatabasePageProperty{
Date: &nt.Date{Start: olderDt},
},
},
},
{
ID: "page-2",
Parent: nt.Parent{
Type: nt.ParentTypeDatabase,
DatabaseID: dbID,
},
Properties: nt.DatabasePageProperties{
"Date": nt.DatabasePageProperty{
Date: &nt.Date{Start: latestDt},
},
},
},
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var latest *time.Time | ||
| for _, page := range resp.Results { | ||
| props, ok := page.Properties.(nt.DatabasePageProperties) | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| if date := s.extractDate(props, "Date"); date != nil { | ||
| s.logger.Debug(fmt.Sprintf("[DEBUG] contractor_payouts: first payout date found discord=%s date=%s", discord, date.Format("2006-01-02"))) | ||
| return date, nil | ||
| if latest == nil || date.After(*latest) { | ||
| latest = date | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The loop to find the maximum date is redundant since the query already sorts by Date in descending order (line 264). The first result with a non-nil date should already be the latest. This pattern is inconsistent with similar code in the codebase (e.g., pkg/service/notion/notion.go:503 which directly uses cls.Results[0] after sorting descending). Consider simplifying this to return the first valid date found, which would be more efficient and consistent with established patterns.
| require.NoError(t, err) | ||
| require.NotNil(t, got) | ||
| require.True(t, got.Equal(wantDate)) | ||
| require.NotEqual(t, oldestDate, *got) |
There was a problem hiding this comment.
This assertion is redundant. If the previous assertion (line 128) passes and confirms that got equals wantDate (2026-01-10), then this assertion will always pass since wantDate and oldestDate (2025-09-30) are different values. Consider removing this assertion as it doesn't add meaningful test coverage.
| require.NotEqual(t, oldestDate, *got) |
Summary
Testing