Conversation
gusfcarvalho
changed the title
There was a problem hiding this comment.
Pull request overview
This PR introduces a dedicated RiskConfig/risk.yaml configuration file and wires up a set of new risk lifecycle periodic jobs (scanner workers and notification workers). It also adds a new riskNotificationsSubscribed user preference with opt-out semantics for risk emails.
Changes:
- Add
RiskConfigwithLoadRiskConfig/Validate, loaded separately from workflow config, with support for env vars and arisk.yamlfile - Add four scanner workers (
RiskReviewDeadlineReminderScanner,RiskReviewOverdueEscalationScanner,RiskStaleRiskScanner,RiskEvidenceReconciliationScanner) and their corresponding notification/action workers - Add
RiskNotificationsSubscribedboolean field (defaulttrue) to theUsermodel and propagate it through the API handler and subscription endpoint
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
risk.yaml |
Default sample config file for risk job scheduling, all jobs disabled by default |
internal/config/risk.go |
New RiskConfig type with LoadRiskConfig, DefaultRiskConfig, and Validate |
internal/config/config.go |
Loads RiskConfig from risk.yaml and attaches to Config.Risk |
internal/config/risk_test.go |
Tests for default values, env override, file loading, and validation |
internal/service/worker/risk_workers.go |
All new scanner and notification worker implementations |
internal/service/worker/risk_job_types.go |
River job arg types, Kind()/Timeout() implementations, and insert option helpers |
internal/service/worker/risk_workers_test.go |
Tests for scanner enqueue behavior, duplicate closing, reopen logic, and subscription gating |
internal/service/worker/service.go |
Registers scanner workers and periodic jobs driven by RiskConfig |
internal/service/worker/service_test.go |
Test that risk periodic jobs are created from dedicated RiskConfig |
internal/service/worker/jobs.go |
Registers new notification/reopen workers, adds RiskNotificationsSubscribed to NotificationUser |
internal/service/worker/user_repository.go |
Maps RiskNotificationsSubscribed from DB user to NotificationUser |
internal/service/relational/ccf_internal.go |
Adds RiskNotificationsSubscribed bool with gorm:"default:true" to User |
internal/api/handler/users.go |
Adds RiskNotificationsSubscribed to subscription get/update endpoints |
internal/api/handler/users_integration_test.go |
Integration tests for the new subscription field |
internal/service/email/templates/templates/*.html/txt |
Three new email templates for risk notifications |
internal/service/email/templates/service_test.go |
Tests that all three new templates render without errors |
docs/swagger.json, docs/swagger.yaml, docs/docs.go |
Updated API docs for the new subscription field |
.env.example |
Documents the CCF_RISK_CONFIG env var |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| riskIDs := make([]uuid.UUID, 0, len(risks)) | ||
| for i := range risks { | ||
| risk := &risks[i] | ||
| if risk == nil || risk.ID == nil { |
There was a problem hiding this comment.
The risk == nil guard on line 549 is dead code. The function receives risks []riskrel.Risk (a slice of value types, not pointer types), so elements retrieved via &risks[i] can never be nil. While this doesn't cause a bug, it is misleading since it implies a condition that can never occur.
| if risk == nil || risk.ID == nil { | |
| if risk.ID == nil { |
| &riskrel.RiskSubjectLink{}, | ||
| &riskrel.RiskEvent{}, | ||
| &riskrel.RiskReview{}, |
There was a problem hiding this comment.
Lines 64–66 use extra indentation (\t\t\t) compared to the preceding lines 53–63 (\t\t), making the argument list of db.AutoMigrate(...) inconsistently formatted. All arguments should be at the same indentation level.
| &riskrel.RiskSubjectLink{}, | |
| &riskrel.RiskEvent{}, | |
| &riskrel.RiskReview{}, | |
| &riskrel.RiskSubjectLink{}, | |
| &riskrel.RiskEvent{}, | |
| &riskrel.RiskReview{}, |
| continue | ||
| } | ||
| ownerIDs := ownersByRiskID[*risk.ID] | ||
| overdueWindow := now.Format("2006-01-02") |
There was a problem hiding this comment.
The overdueWindow variable is declared inside the for i := range risks loop on line 141, but it always evaluates to the same value — now.Format("2006-01-02") — regardless of which risk is being processed. Since now is defined at the start of the outer Work function and never changes, this can be computed once before the loop (or even before the batch callback) to avoid the redundant formatting call on every iteration.
Summary
RiskConfig/risk.yamland load it separately so risk scheduling stays outside workflow configriskNotificationsSubscribedpreference) outlined in BCH-1183Testing
make reviewablepasses successfully