Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds an email subsystem and StrategySummaryApp to compute and send daily saved-strategy summary emails via AWS SES, including API endpoint, templates, domain types, DB migrations, repository/service implementations, mocks, and dependency wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as API Handler
participant App as StrategySummaryApp
participant UserRepo as UserRepository
participant StrategyRepo as StrategyRepository
participant EmailSvc as EmailService
participant EmailRepo as EmailRepository
participant SES as AWS SES
Client->>API: POST /sendDailyStrategySummaries
API->>API: attach profile & logger
API->>App: SendDailyStrategySummaries(ctx)
App->>UserRepo: ListUsersWithEmail()
UserRepo-->>App: users
loop per user
App->>StrategyRepo: Load saved strategies
StrategyRepo-->>App: strategies
loop per strategy
App->>App: computeStrategySummary()
end
App->>EmailSvc: GenerateStrategySummaryEmail(user, summaries)
EmailSvc->>EmailSvc: render template (strategy_summary.html)
EmailSvc-->>App: subject, body
App->>EmailSvc: SendStrategySummaryEmail(user, summaries)
EmailSvc->>EmailRepo: SendEmail(to, subject, body)
EmailRepo->>SES: SendEmail API
SES-->>EmailRepo: MessageId / error
EmailRepo-->>EmailSvc: result
end
App-->>API: result
API-->>Client: 200 OK / 500 error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
internal/app/strategy_summary.app.go
Outdated
| lg.Info("starting daily strategy summaries") | ||
|
|
||
| // Get all users with email addresses | ||
| users, err := h.UserAccountRepository.ListUsersWithEmail() |
There was a problem hiding this comment.
It should really only include users that are opted into this feature.
internal/app/strategy_summary.app.go
Outdated
| } | ||
|
|
||
| // Use a reference portfolio value for calculations (e.g., $10,000) | ||
| referencePortfolioValue := decimal.NewFromInt(10000) |
There was a problem hiding this comment.
We shouldn't need this assumption.
internal/domain/strategy_summary.go
Outdated
| // StrategySummaryAsset represents a single asset that a strategy would buy | ||
| type StrategySummaryAsset struct { | ||
| Symbol string | ||
| Quantity decimal.Decimal |
There was a problem hiding this comment.
I don't think we need this.
![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
🤖 Fix all issues with AI agents
In `@api/api.go`:
- Line 121: The POST route for sendDailyStrategySummaries is currently
unprotected; update the registration and/or handler so only authenticated users
can call it: attach the existing getGoogleAuthMiddleware (or the same middleware
stack used by other protected routes) when registering the route instead of a
direct engine.POST("/sendDailyStrategySummaries", m.sendDailyStrategySummaries),
and inside sendDailyStrategySummaries verify the authenticated context by
checking for the userAccountID value placed by the middleware (reject the
request with an unauthorized response if missing); alternatively, enforce an
explicit authorization check (e.g., admin-only) before performing the bulk email
send to prevent unauthenticated or unauthorized invocation.
In `@api/send_daily_strategy_summaries.resolver.go`:
- Around line 33-38: The handler currently returns the full error text to the
client via c.JSON(500, SendDailyStrategySummariesResponse{Message:
fmt.Sprintf("Failed to process daily strategy summaries: %v", err)}) while also
logging it with lg.Errorf; change the response to a generic message (e.g.,
"Internal server error processing daily strategy summaries.") and keep the
detailed error only in the server log by retaining lg.Errorf("failed to send
daily strategy summaries: %v", err); ensure
SendDailyStrategySummariesResponse.Message no longer contains err or its
formatting.
In `@internal/app/strategy_summary.app.go`:
- Around line 73-80: The code calls PriceRepository.LatestTradingDay() and then
dereferences the returned *time.Time (e.g., in computeStrategySummary and later
around the 105–109 area) without checking for nil; add a nil-check immediately
after the call to LatestTradingDay() and return a clear error (or handle the
empty-data case) if the pointer is nil to avoid a panic. Locate the call sites
(PriceRepository.LatestTradingDay and any use of latestTradingDay in
computeStrategySummary) and guard usages like
latestTradingDay.UTC()/Year()/Month() with an if latestTradingDay == nil {
return fmt.Errorf("no latest trading day available") } (or appropriate handling)
before dereferencing. Ensure the same nil-check is applied to the second
occurrence referenced in the comment.
In `@templates/strategy_summary.html`:
- Line 39: The template currently formats .Weight as a percent without scaling,
showing "0.25%" for a 25% value; update the formatting to multiply .Weight by
100 before printing. Fix by changing the template expression around printf to
pass (theWeight*100) — either precompute a Percent field in the Go code and use
that in templates, or register a small template function (e.g., mulf) and use
{{printf "%.2f%%" (mulf .Weight 100)}} so that templates/strategy_summary.html
prints the correct percentage for .Weight.
🧹 Nitpick comments (10)
internal/repository/user_account.repository.go (1)
62-81: Consider filtering empty strings in SQL rather than in Go.The current implementation fetches all users with non-null emails, then filters out empty strings in memory. This can be optimized by adding the constraint to the query itself, avoiding unnecessary data transfer and memory allocation.
♻️ Proposed refactor
func (h userAccountRepositoryHandler) ListUsersWithEmail() ([]model.UserAccount, error) { t := table.UserAccount query := t.SELECT(t.AllColumns). - WHERE(t.Email.IS_NOT_NULL()) + WHERE( + t.Email.IS_NOT_NULL(). + AND(t.Email.NOT_EQ(postgres.String(""))), + ) result := []model.UserAccount{} err := query.Query(h.DB, &result) if err != nil { return nil, fmt.Errorf("failed to list users with email: %w", err) } - // Filter out users with empty email strings - filtered := []model.UserAccount{} - for _, user := range result { - if user.Email != nil && *user.Email != "" { - filtered = append(filtered, user) - } - } - - return filtered, nil + return result, nil }templates/strategy_summary.html (1)
21-47: Consider adding a fallback for empty strategies.If a user has no saved strategies or all strategies have no assets, the email would show empty content between the greeting and signature. Consider adding an
{{if}}block with a message for this edge case.♻️ Proposed enhancement
<p>Here's what your saved strategies would buy today:</p> + {{if not .Strategies}} + <p>You don't have any saved strategies yet.</p> + {{else}} {{range .Strategies}} <div class="strategy-section"> ... </div> {{end}} + {{end}}internal/service/email.service_test.go (2)
19-49: Code duplication: consider extracting shared test helper.The
initializeEmailServicehelper duplicates the secrets-parsing logic found ininternal/repository/ses_email.repository_test.go'sinitializeEmailHandler. Consider extracting a shared test utility for loading SES secrets to reduce duplication.Also applies to: 12-45
63-67: Consider using placeholder email for test data.The test contains a real email address. While acceptable in test files, consider using a placeholder like
"test@example.com"or environment variable for the preview data to avoid accidentally committing PII.internal/repository/ses_email.repository_test.go (2)
72-84: Simplify error handling -require.NoErrorwill already fail the test.The
require.NoError(t, err)on line 82 will fail the test, making the earlyreturnon line 83 unreachable. The logging is helpful, but consider restructuring:Proposed simplification
if err != nil { t.Logf("ERROR: Failed to send email: %v", err) t.Logf("") t.Logf("Common issues:") t.Logf("1. SES Sandbox Mode: If your SES account is in sandbox mode,") t.Logf(" you can only send to verified email addresses.") t.Logf(" Verify %s in SES Console or request production access.", testEmail) t.Logf("2. Check AWS credentials are configured correctly") t.Logf("3. Check spam folder") t.Logf("4. Verify the 'fromEmail' domain is verified in SES") - require.NoError(t, err) - return + t.FailNow() }
56-56: Consider using placeholder email.Same as the service test—consider using
"test@example.com"or an environment variable instead of a real email address.internal/service/email.service.go (1)
134-153: Silent error fromos.Getwd()may hide issues.Line 136 ignores the error from
os.Getwd(). While this is often acceptable since the absolute path is just one of several fallbacks, consider logging a warning when it fails to aid debugging in unusual environments.Proposed improvement
func findTemplatePath(templateName string) (string, error) { - wd, _ := os.Getwd() + wd, err := os.Getwd() + if err != nil { + wd = "" // Fall back to relative paths only + }internal/repository/ses_email.repository.go (2)
66-66: Consider accepting context parameter for cancellation support.
SendEmailusescontext.Background(), which prevents callers from cancelling long-running SES requests. Consider adding a context parameter to the interface for better control over timeouts and cancellation.Proposed interface change
type EmailRepository interface { - SendEmail(to string, subject string, body string) error + SendEmail(ctx context.Context, to string, subject string, body string) error }Then update the implementation:
-func (h *emailRepositoryHandler) SendEmail(to string, subject string, body string) error { +func (h *emailRepositoryHandler) SendEmail(ctx context.Context, to string, subject string, body string) error { // ... - result, err := h.sesClient.SendEmail(context.Background(), input) + result, err := h.sesClient.SendEmail(ctx, input)
71-75: Remove dead code or implement logging.The MessageId handling does nothing—the value is assigned to the blank identifier. Either remove this block or implement actual logging/tracing.
Option 1: Remove dead code
result, err := h.sesClient.SendEmail(context.Background(), input) if err != nil { return fmt.Errorf("failed to send email via SES: %w", err) } - - // Log the message ID for tracking (optional - can be removed if not needed) - if result.MessageId != nil { - // Message ID can be used to track email delivery in CloudWatch - _ = result.MessageId - } return nilOption 2: Implement logging
- // Log the message ID for tracking (optional - can be removed if not needed) - if result.MessageId != nil { - // Message ID can be used to track email delivery in CloudWatch - _ = result.MessageId - } + // Optionally log for tracking: log.Printf("Email sent, MessageId: %s", *result.MessageId)internal/app/strategy_summary.app.go (1)
201-223: Sort assets for deterministic email ordering.
Map iteration order is random; sorting avoids flaky output and stabilizes emails/tests.♻️ Proposed change
@@ -import ( - "context" - "factorbacktest/internal/calculator" - "factorbacktest/internal/data" - "factorbacktest/internal/db/models/postgres/public/model" - "factorbacktest/internal/domain" - "factorbacktest/internal/logger" - "factorbacktest/internal/repository" - "factorbacktest/internal/service" - "fmt" - "time" +import ( + "context" + "factorbacktest/internal/calculator" + "factorbacktest/internal/data" + "factorbacktest/internal/db/models/postgres/public/model" + "factorbacktest/internal/domain" + "factorbacktest/internal/logger" + "factorbacktest/internal/repository" + "factorbacktest/internal/service" + "fmt" + "sort" + "time" @@ for symbol, position := range computeTargetPortfolioResponse.TargetPortfolio.Positions { weight, ok := computeTargetPortfolioResponse.AssetWeights[symbol] if !ok { continue } @@ assets = append(assets, domain.StrategySummaryAsset{ Symbol: symbol, Quantity: position.ExactQuantity, Weight: weight, FactorScore: factorScore, Price: price, }) } + + sort.Slice(assets, func(i, j int) bool { + if assets[i].Weight == assets[j].Weight { + return assets[i].Symbol < assets[j].Symbol + } + return assets[i].Weight > assets[j].Weight + })
api/api.go
Outdated
| engine.GET("/publishedStrategies", m.getPublishedStrategies) | ||
|
|
||
| engine.POST("/rebalance", m.rebalance) | ||
| engine.POST("/sendDailyStrategySummaries", m.sendDailyStrategySummaries) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the handler implementation
rg -n -A 30 'func.*sendDailyStrategySummaries' --type=goRepository: sahilsk11/factorbacktest
Length of output: 2336
🏁 Script executed:
#!/bin/bash
# Check the getGoogleAuthMiddleware implementation
rg -n -B 5 -A 20 'getGoogleAuthMiddleware' api/api.goRepository: sahilsk11/factorbacktest
Length of output: 1662
🏁 Script executed:
#!/bin/bash
# Look for the full middleware chain and route registration
rg -n -B 2 -A 2 'sendDailyStrategySummaries' api/api.goRepository: sahilsk11/factorbacktest
Length of output: 218
🏁 Script executed:
#!/bin/bash
# Look for endpoints that explicitly check authentication or have comments about security
rg -n 'engine\.(GET|POST|PUT|DELETE)' api/api.go | head -20Repository: sahilsk11/factorbacktest
Length of output: 1084
🏁 Script executed:
#!/bin/bash
# Check if there are any endpoints with auth checks in their handlers
rg -n 'GetUserAccount\|c\.GetString' --type=go api/ | head -20Repository: sahilsk11/factorbacktest
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check a few handlers to see if they verify user authentication
rg -n -A 10 'func.*backtest|func.*rebalance' api/*.go | head -50Repository: sahilsk11/factorbacktest
Length of output: 1963
🏁 Script executed:
#!/bin/bash
# Look for GetUserAccount usage which might indicate auth checks
rg -n 'GetUserAccount|UserAccount' api/*.goRepository: sahilsk11/factorbacktest
Length of output: 2005
Endpoint lacks authentication protection.
The /sendDailyStrategySummaries endpoint can be called without authentication since getGoogleAuthMiddleware allows unauthenticated requests to proceed (lines 252-254), and the handler does not validate user context. This allows unauthorized users to trigger bulk email sends, potentially leading to abuse or resource exhaustion.
Other protected endpoints in the codebase check for userAccountID via context (set only after successful authentication). This endpoint should follow the same pattern or implement alternative protection such as authentication/authorization requirements or rate limiting.
🤖 Prompt for AI Agents
In `@api/api.go` at line 121, The POST route for sendDailyStrategySummaries is
currently unprotected; update the registration and/or handler so only
authenticated users can call it: attach the existing getGoogleAuthMiddleware (or
the same middleware stack used by other protected routes) when registering the
route instead of a direct engine.POST("/sendDailyStrategySummaries",
m.sendDailyStrategySummaries), and inside sendDailyStrategySummaries verify the
authenticated context by checking for the userAccountID value placed by the
middleware (reject the request with an unauthorized response if missing);
alternatively, enforce an explicit authorization check (e.g., admin-only) before
performing the bulk email send to prevent unauthenticated or unauthorized
invocation.
| if err != nil { | ||
| lg.Errorf("failed to send daily strategy summaries: %v", err) | ||
| c.JSON(500, SendDailyStrategySummariesResponse{ | ||
| Message: fmt.Sprintf("Failed to process daily strategy summaries: %v", err), | ||
| }) | ||
| return |
There was a problem hiding this comment.
Avoid exposing internal error details in API responses.
The error response includes the full error message (%v, err), which may leak internal implementation details to clients. Return a generic message to clients and log the detailed error server-side only.
Proposed fix
if err != nil {
lg.Errorf("failed to send daily strategy summaries: %v", err)
c.JSON(500, SendDailyStrategySummariesResponse{
- Message: fmt.Sprintf("Failed to process daily strategy summaries: %v", err),
+ Message: "Failed to process daily strategy summaries",
})
return
}📝 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.
| if err != nil { | |
| lg.Errorf("failed to send daily strategy summaries: %v", err) | |
| c.JSON(500, SendDailyStrategySummariesResponse{ | |
| Message: fmt.Sprintf("Failed to process daily strategy summaries: %v", err), | |
| }) | |
| return | |
| if err != nil { | |
| lg.Errorf("failed to send daily strategy summaries: %v", err) | |
| c.JSON(500, SendDailyStrategySummariesResponse{ | |
| Message: "Failed to process daily strategy summaries", | |
| }) | |
| return | |
| } |
🤖 Prompt for AI Agents
In `@api/send_daily_strategy_summaries.resolver.go` around lines 33 - 38, The
handler currently returns the full error text to the client via c.JSON(500,
SendDailyStrategySummariesResponse{Message: fmt.Sprintf("Failed to process daily
strategy summaries: %v", err)}) while also logging it with lg.Errorf; change the
response to a generic message (e.g., "Internal server error processing daily
strategy summaries.") and keep the detailed error only in the server log by
retaining lg.Errorf("failed to send daily strategy summaries: %v", err); ensure
SendDailyStrategySummariesResponse.Message no longer contains err or its
formatting.
internal/app/strategy_summary.app.go
Outdated
| // Get latest trading day (assumes prices are already updated for today) | ||
| latestTradingDay, err := h.PriceRepository.LatestTradingDay() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get latest trading day: %w", err) | ||
| } | ||
|
|
||
| // Use a reference portfolio value for calculations (e.g., $10,000) | ||
| referencePortfolioValue := decimal.NewFromInt(10000) |
There was a problem hiding this comment.
Guard against nil latestTradingDay before dereference.
LatestTradingDay() returns *time.Time; if it returns nil without error (e.g., empty data set), the dereference in computeStrategySummary will panic.
🛠️ Proposed fix
latestTradingDay, err := h.PriceRepository.LatestTradingDay()
if err != nil {
return fmt.Errorf("failed to get latest trading day: %w", err)
}
+ if latestTradingDay == nil {
+ return fmt.Errorf("no trading day available")
+ }Also applies to: 105-109
🤖 Prompt for AI Agents
In `@internal/app/strategy_summary.app.go` around lines 73 - 80, The code calls
PriceRepository.LatestTradingDay() and then dereferences the returned *time.Time
(e.g., in computeStrategySummary and later around the 105–109 area) without
checking for nil; add a nil-check immediately after the call to
LatestTradingDay() and return a clear error (or handle the empty-data case) if
the pointer is nil to avoid a panic. Locate the call sites
(PriceRepository.LatestTradingDay and any use of latestTradingDay in
computeStrategySummary) and guard usages like
latestTradingDay.UTC()/Year()/Month() with an if latestTradingDay == nil {
return fmt.Errorf("no latest trading day available") } (or appropriate handling)
before dereferencing. Ensure the same nil-check is applied to the second
occurrence referenced in the comment.
templates/strategy_summary.html
Outdated
| <tr> | ||
| <td>{{.Symbol}}</td> | ||
| <td>{{.Quantity}}</td> | ||
| <td>{{printf "%.2f%%" .Weight}}</td> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for StrategySummaryAsset definition and how Weight is used
rg -n -B 2 -A 10 'type StrategySummaryAsset' --type=goRepository: sahilsk11/factorbacktest
Length of output: 683
Fix percentage display for Weight field: multiply by 100 before formatting.
The Weight field is stored as a decimal fraction between 0 and 1 (e.g., 0.25 for 25%). The current template {{printf "%.2f%%" .Weight}} will display "0.25%" instead of "25.00%". Multiply by 100 before formatting, either by pre-processing in the Go code or using a template function: {{printf "%.2f%%" (mulf .Weight 100)}}.
🤖 Prompt for AI Agents
In `@templates/strategy_summary.html` at line 39, The template currently formats
.Weight as a percent without scaling, showing "0.25%" for a 25% value; update
the formatting to multiply .Weight by 100 before printing. Fix by changing the
template expression around printf to pass (theWeight*100) — either precompute a
Percent field in the Go code and use that in templates, or register a small
template function (e.g., mulf) and use {{printf "%.2f%%" (mulf .Weight 100)}} so
that templates/strategy_summary.html prints the correct percentage for .Weight.
|
|
||
| // Test_emailServiceHandler_GenerateStrategySummaryEmail_Preview renders a template | ||
| // with sample data and saves it to a file for preview | ||
| func Test_emailServiceHandler_GenerateStrategySummaryEmail_Preview(t *testing.T) { |
There was a problem hiding this comment.
These should be updated so that we use the mock.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@cmd/util.go`:
- Around line 127-133: The code unconditionally initializes the email
repo/service using secrets.SES which is missing in test/dev and causes AWS SDK
failures; update cmd/util.go to check for SES config presence (e.g., ensure
secrets.SES.Region and secrets.SES.FromEmail are non-empty) before calling
repository.NewEmailRepository and service.NewEmailService, and skip or return a
noop emailService when values are absent. Locate the email initialization around
the emailRepository/emailService variables (NewEmailRepository, NewEmailService)
and gate that logic with a simple conditional or factory that returns a
disabled/noop implementation to avoid calling config.LoadDefaultConfig when SES
is not configured.
In `@internal/app/strategy_summary.app.go`:
- Around line 162-176: In the conversion loop that builds assets for the
StrategySummaryResult (the block iterating
computeTargetPortfolioResponse.TargetPortfolio.Positions and populating assets
:= []domain.StrategySummaryAsset{}), stop silently skipping entries and instead
return an explicit error when required data is missing: if a symbol lacks a
weight in computeTargetPortfolioResponse.AssetWeights, a factorScore in
computeTargetPortfolioResponse.FactorScores, or a price in priceMap, return a
descriptive error (including the symbol and which field is missing) so callers
can detect data inconsistencies rather than producing incomplete summaries.
In `@internal/repository/user_account.repository.go`:
- Around line 33-41: The GetByID implementation in userAccountRepositoryHandler
treats a "no rows" result as an error; update the error handling after
query.Query(h.DB, &out) to detect a not-found case (use errors.Is(err,
sql.ErrNoRows) or the project's equivalent) and return nil, nil for not-found,
while preserving the fmt.Errorf("failed to get user account: %w", err) behavior
for other errors; ensure you add the necessary imports (errors and database/sql)
if missing and keep function signature (GetByID, model.UserAccount, query.Query)
unchanged.
In `@internal/service/email.service.go`:
- Around line 72-80: The public handler method SendStrategySummaryEmail
currently dereferences user.Email without checking whether user is nil, which
can panic; add an initial nil guard in SendStrategySummaryEmail (if user == nil
{ return fmt.Errorf("user is nil") }) before any dereferences, and do the same
for the other public handler method(s) that dereference user (the second public
method around lines 93-100). Ensure you return a clear error instead of
proceeding, and keep calls to GenerateStrategySummaryEmail only after the nil
check.
♻️ Duplicate comments (1)
internal/app/strategy_summary.app.go (1)
78-90: Guard against nillatestTradingDaybefore dereference.
LatestTradingDay()returns a*time.Time; if it returns nil without error, dereferencing will panic. Add a nil-check right after the call.🛠️ Proposed fix
latestTradingDay, err := h.PriceRepository.LatestTradingDay() if err != nil { return fmt.Errorf("failed to get latest trading day: %w", err) } +if latestTradingDay == nil { + return fmt.Errorf("no latest trading day available") +}
🧹 Nitpick comments (2)
internal/repository/email_preference.repository.go (1)
93-115: FilterOFFrows in SQL to avoid extra reads.
Right now you fetch all rows for the email type and filter in Go. Pushing the filter into the query reduces IO and memory.♻️ Proposed refactor
- query := t.SELECT(t.AllColumns). - // email_type is a Postgres enum; use NewEnumValue to avoid `operator does not exist: email_type = text` - WHERE(t.EmailType.EQ(postgres.NewEnumValue(emailType.String()))) + query := t.SELECT(t.AllColumns). + // email_type is a Postgres enum; use NewEnumValue to avoid `operator does not exist: email_type = text` + WHERE( + postgres.AND( + t.EmailType.EQ(postgres.NewEnumValue(emailType.String())), + t.Frequency.NEQ(postgres.NewEnumValue(string(model.EmailFrequency_Off))), + ), + ) @@ - out := []model.EmailPreference{} - for _, r := range rows { - if r.Frequency != model.EmailFrequency_Off { - out = append(out, r) - } - } - - return out, nil + return rows, nilinternal/app/strategy_summary.app.go (1)
87-97: Surface per-user failures (log + optional aggregated error).Right now failures are only counted, which can hide issues from callers/alerts. Consider logging per-user errors and returning an error if any failures occurred.
🔧 Possible refinement
for _, optInPreference := range optedInPrefs { err := h.processSavedStrategyEmail(ctx, optInPreference, *latestTradingDay) if err != nil { emailsFailed++ + lg.Warnw("failed to process strategy summary email", + "userAccountID", optInPreference.UserAccountID, + "err", err, + ) continue } emailsSent++ } lg.Infof("daily strategy summaries completed: %d emails sent, %d failed", emailsSent, emailsFailed) +if emailsFailed > 0 { + return fmt.Errorf("daily strategy summaries completed with %d failures", emailsFailed) +} return nil
| // Initialize email repository and service | ||
| emailRepository, err := repository.NewEmailRepository(secrets.SES.Region, secrets.SES.FromEmail) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create email repository: %w", err) | ||
| } | ||
| emailService := service.NewEmailService(emailRepository) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine cmd/util.go
find . -name "cmd" -type dRepository: sahilsk11/factorbacktest
Length of output: 73
🏁 Script executed:
# List files in cmd directory
ls -la cmd/ 2>/dev/null || echo "cmd directory not found in root"Repository: sahilsk11/factorbacktest
Length of output: 466
🏁 Script executed:
# Search for util.go
fd -name "util.go" -type fRepository: sahilsk11/factorbacktest
Length of output: 298
🏁 Script executed:
# Search for SES configuration and usage
rg -i "ses" -A 3 -B 3 --type-listRepository: sahilsk11/factorbacktest
Length of output: 457
🏁 Script executed:
# Look for secrets structure and initialization
rg "secrets" -A 2 -B 2 -t go | head -100Repository: sahilsk11/factorbacktest
Length of output: 5655
🏁 Script executed:
# List the structure first
git ls-files | head -30Repository: sahilsk11/factorbacktest
Length of output: 812
🏁 Script executed:
# Look for the cmd directory and util.go
git ls-files "cmd/*"Repository: sahilsk11/factorbacktest
Length of output: 146
🏁 Script executed:
# Search for SES configuration patterns
rg "SES" -t go -A 2 -B 2Repository: sahilsk11/factorbacktest
Length of output: 6352
🏁 Script executed:
# Search for secrets initialization
rg "secrets" -t go -A 3 -B 1 | head -150Repository: sahilsk11/factorbacktest
Length of output: 7335
🏁 Script executed:
# Read cmd/util.go completely
cat cmd/util.goRepository: sahilsk11/factorbacktest
Length of output: 5940
🏁 Script executed:
# Check the Secrets struct definition and SES field
cat internal/util/util.goRepository: sahilsk11/factorbacktest
Length of output: 3038
🏁 Script executed:
# Check what happens in NewEmailRepository with empty config
rg -A 20 "func NewEmailRepository" internal/repository/Repository: sahilsk11/factorbacktest
Length of output: 1636
🏁 Script executed:
# Check if there's any validation in NewEmailRepository for empty values
cat internal/repository/ses_email.repository.go | head -100Repository: sahilsk11/factorbacktest
Length of output: 2251
🏁 Script executed:
# Look for secrets files
find . -name "secrets*.json" -o -name "*.secrets*" 2>/dev/null | head -20Repository: sahilsk11/factorbacktest
Length of output: 87
🏁 Script executed:
# Check git for any reference to secrets files
git ls-files | grep -i secretRepository: sahilsk11/factorbacktest
Length of output: 85
🏁 Script executed:
# Check what's in secrets-test.json
cat secrets-test.jsonRepository: sahilsk11/factorbacktest
Length of output: 278
Add SES configuration to test/dev secrets or make email initialization conditional.
The test environment will fail at startup because secrets-test.json lacks the ses field entirely. When unmarshaled, secrets.SES.Region and secrets.SES.FromEmail become empty strings, causing config.LoadDefaultConfig() to fail. Either add SES configuration to all environment secret files, or implement conditional initialization that skips email setup when SES config is unavailable.
🤖 Prompt for AI Agents
In `@cmd/util.go` around lines 127 - 133, The code unconditionally initializes the
email repo/service using secrets.SES which is missing in test/dev and causes AWS
SDK failures; update cmd/util.go to check for SES config presence (e.g., ensure
secrets.SES.Region and secrets.SES.FromEmail are non-empty) before calling
repository.NewEmailRepository and service.NewEmailService, and skip or return a
noop emailService when values are absent. Locate the email initialization around
the emailRepository/emailService variables (NewEmailRepository, NewEmailService)
and gate that logic with a simple conditional or factory that returns a
disabled/noop implementation to avoid calling config.LoadDefaultConfig when SES
is not configured.
| // 5. Convert to StrategySummaryResult domain object | ||
| assets := []domain.StrategySummaryAsset{} | ||
| for symbol := range computeTargetPortfolioResponse.TargetPortfolio.Positions { | ||
| weight, ok := computeTargetPortfolioResponse.AssetWeights[symbol] | ||
| if !ok { | ||
| continue | ||
| } | ||
| factorScore, ok := computeTargetPortfolioResponse.FactorScores[symbol] | ||
| if !ok { | ||
| continue | ||
| } | ||
| price, ok := priceMap[symbol] | ||
| if !ok { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Avoid silently skipping missing asset data.
Skipping missing weights/scores/prices can produce incomplete summaries without signaling an error. Prefer failing fast to surface data inconsistencies.
💡 Safer handling
for symbol := range computeTargetPortfolioResponse.TargetPortfolio.Positions {
weight, ok := computeTargetPortfolioResponse.AssetWeights[symbol]
if !ok {
- continue
+ return nil, fmt.Errorf("missing weight for symbol %s", symbol)
}
factorScore, ok := computeTargetPortfolioResponse.FactorScores[symbol]
if !ok {
- continue
+ return nil, fmt.Errorf("missing factor score for symbol %s", symbol)
}
price, ok := priceMap[symbol]
if !ok {
- continue
+ return nil, fmt.Errorf("missing price for symbol %s", symbol)
}📝 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.
| // 5. Convert to StrategySummaryResult domain object | |
| assets := []domain.StrategySummaryAsset{} | |
| for symbol := range computeTargetPortfolioResponse.TargetPortfolio.Positions { | |
| weight, ok := computeTargetPortfolioResponse.AssetWeights[symbol] | |
| if !ok { | |
| continue | |
| } | |
| factorScore, ok := computeTargetPortfolioResponse.FactorScores[symbol] | |
| if !ok { | |
| continue | |
| } | |
| price, ok := priceMap[symbol] | |
| if !ok { | |
| continue | |
| } | |
| // 5. Convert to StrategySummaryResult domain object | |
| assets := []domain.StrategySummaryAsset{} | |
| for symbol := range computeTargetPortfolioResponse.TargetPortfolio.Positions { | |
| weight, ok := computeTargetPortfolioResponse.AssetWeights[symbol] | |
| if !ok { | |
| return nil, fmt.Errorf("missing weight for symbol %s", symbol) | |
| } | |
| factorScore, ok := computeTargetPortfolioResponse.FactorScores[symbol] | |
| if !ok { | |
| return nil, fmt.Errorf("missing factor score for symbol %s", symbol) | |
| } | |
| price, ok := priceMap[symbol] | |
| if !ok { | |
| return nil, fmt.Errorf("missing price for symbol %s", symbol) | |
| } |
🤖 Prompt for AI Agents
In `@internal/app/strategy_summary.app.go` around lines 162 - 176, In the
conversion loop that builds assets for the StrategySummaryResult (the block
iterating computeTargetPortfolioResponse.TargetPortfolio.Positions and
populating assets := []domain.StrategySummaryAsset{}), stop silently skipping
entries and instead return an explicit error when required data is missing: if a
symbol lacks a weight in computeTargetPortfolioResponse.AssetWeights, a
factorScore in computeTargetPortfolioResponse.FactorScores, or a price in
priceMap, return a descriptive error (including the symbol and which field is
missing) so callers can detect data inconsistencies rather than producing
incomplete summaries.
| func (h userAccountRepositoryHandler) GetByID(userAccountID uuid.UUID) (*model.UserAccount, error) { | ||
| t := table.UserAccount | ||
| query := t.SELECT(t.AllColumns). | ||
| WHERE(t.UserAccountID.EQ(postgres.UUID(userAccountID))) | ||
| out := model.UserAccount{} | ||
| err := query.Query(h.DB, &out) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get user account: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find all repository files and examine error handling patterns
find . -type f -name "*.repository.go" | head -20Repository: sahilsk11/factorbacktest
Length of output: 1119
🏁 Script executed:
# Search for ErrNoRows handling in repository files
rg "ErrNoRows" --type go -A 3 -B 3Repository: sahilsk11/factorbacktest
Length of output: 4021
🏁 Script executed:
# Look at the user_account.repository.go file for context
cat -n internal/repository/user_account.repository.go | head -60Repository: sahilsk11/factorbacktest
Length of output: 2147
🏁 Script executed:
# Check what error handling pattern is used in other GetByID methods
rg "func.*GetByID" --type go -A 8 | head -100Repository: sahilsk11/factorbacktest
Length of output: 851
Handle not-found separately in GetByID.
ErrNoRows currently becomes an error, which may bubble as a failure instead of "not found." Other repository methods (e.g., email_preference.repository.go, strategy.repository.go) handle this consistently by returning nil, nil for not-found cases.
Suggested fix
out := model.UserAccount{}
err := query.Query(h.DB, &out)
+ if errors.Is(err, qrm.ErrNoRows) {
+ return nil, nil
+ }
if err != nil {
return nil, fmt.Errorf("failed to get user account: %w", err)
}
return &out, nil📝 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.
| func (h userAccountRepositoryHandler) GetByID(userAccountID uuid.UUID) (*model.UserAccount, error) { | |
| t := table.UserAccount | |
| query := t.SELECT(t.AllColumns). | |
| WHERE(t.UserAccountID.EQ(postgres.UUID(userAccountID))) | |
| out := model.UserAccount{} | |
| err := query.Query(h.DB, &out) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get user account: %w", err) | |
| } | |
| func (h userAccountRepositoryHandler) GetByID(userAccountID uuid.UUID) (*model.UserAccount, error) { | |
| t := table.UserAccount | |
| query := t.SELECT(t.AllColumns). | |
| WHERE(t.UserAccountID.EQ(postgres.UUID(userAccountID))) | |
| out := model.UserAccount{} | |
| err := query.Query(h.DB, &out) | |
| if errors.Is(err, qrm.ErrNoRows) { | |
| return nil, nil | |
| } | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get user account: %w", err) | |
| } | |
| return &out, nil | |
| } |
🤖 Prompt for AI Agents
In `@internal/repository/user_account.repository.go` around lines 33 - 41, The
GetByID implementation in userAccountRepositoryHandler treats a "no rows" result
as an error; update the error handling after query.Query(h.DB, &out) to detect a
not-found case (use errors.Is(err, sql.ErrNoRows) or the project's equivalent)
and return nil, nil for not-found, while preserving the fmt.Errorf("failed to
get user account: %w", err) behavior for other errors; ensure you add the
necessary imports (errors and database/sql) if missing and keep function
signature (GetByID, model.UserAccount, query.Query) unchanged.
| func (h *emailServiceHandler) SendStrategySummaryEmail( | ||
| user *model.UserAccount, | ||
| strategyResults []domain.StrategySummaryResult, | ||
| ) error { | ||
| if user.Email == nil || *user.Email == "" { | ||
| return fmt.Errorf("user has no email address") | ||
| } | ||
|
|
||
| subject, htmlBody, err := h.GenerateStrategySummaryEmail(user, strategyResults) |
There was a problem hiding this comment.
Guard against nil user to avoid panics.
Both public methods dereference user without a nil check; a nil caller would crash the process.
🐛 Suggested fix
func (h *emailServiceHandler) SendStrategySummaryEmail(
user *model.UserAccount,
strategyResults []domain.StrategySummaryResult,
) error {
+ if user == nil {
+ return fmt.Errorf("user is nil")
+ }
if user.Email == nil || *user.Email == "" {
return fmt.Errorf("user has no email address")
}
@@
func (h *emailServiceHandler) GenerateStrategySummaryEmail(
user *model.UserAccount,
strategyResults []domain.StrategySummaryResult,
) (string, string, error) {
+ if user == nil {
+ return "", "", fmt.Errorf("user is nil")
+ }
if len(strategyResults) == 0 {
return "", "", fmt.Errorf("no strategy results provided")
}Also applies to: 93-100
🤖 Prompt for AI Agents
In `@internal/service/email.service.go` around lines 72 - 80, The public handler
method SendStrategySummaryEmail currently dereferences user.Email without
checking whether user is nil, which can panic; add an initial nil guard in
SendStrategySummaryEmail (if user == nil { return fmt.Errorf("user is nil") })
before any dereferences, and do the same for the other public handler method(s)
that dereference user (the second public method around lines 93-100). Ensure you
return a clear error instead of proceeding, and keep calls to
GenerateStrategySummaryEmail only after the nil check.
1 participant
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.