Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFeature flags now use module-backed list/toggle handlers, store updates, and route registration. The old v2 feature endpoints were removed, and tests now cover persistence, HTTP behavior, and module wiring. ChangesFeature Flags API
Sequence Diagram(s)sequenceDiagram
participant Router
participant Handlers
participant Service
participant Store
Router->>Handlers: GET /api/v2/features
Handlers->>Service: GetAllFlags(ctx)
Service->>Store: GetAllFlags(ctx)
Store-->>Service: []FeatureFlag
Service-->>Handlers: []FeatureFlag
Handlers-->>Router: JSON response
Router->>Handlers: PUT /api/v2/features/{feature_id}/toggle
Handlers->>Service: ToggleFlag(ctx, id, requestedBy)
Service->>Store: GetFlagByID(ctx, id)
Store-->>Service: FeatureFlag
Service->>Store: SetFlag(ctx, featureFlag)
Store-->>Service: updated FeatureFlag
Service-->>Handlers: updated FeatureFlag
Handlers-->>Router: JSON response / HTTP status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
coderabbitai
Bot
added
api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
server/featureflags/internal/services/services.go (1)
110-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
requestedByis accepted but never used.
ToggleFlagtakesrequestedByyet ignores it — the audit actor is resolved from the context insideSetFlag. The handler computesuser.ID.String()and passes it here expecting it to be recorded, but it is silently discarded, so the value the caller supplies and the actor actually persisted can diverge. Either drop the parameter or thread it through to the audit write.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/featureflags/internal/services/services.go` around lines 110 - 127, ToggleFlag currently accepts requestedBy but never uses it, so the caller-supplied audit actor is discarded. Update Service.ToggleFlag and its SetFlag call path to either remove requestedBy entirely or pass it through so the audit write records that exact value instead of resolving a different actor from context; use ToggleFlag and SetFlag as the key symbols when making the change.server/featureflags/internal/appdb/appdb.go (1)
138-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
pgx.ErrNoRowsbranch afterQuery.
querier.Querydoesn't surfacepgx.ErrNoRowsfor an empty result set; that case is correctly handled by theCollectOneRowbranch below (Line 147). This first check at Line 139-141 is unreachable for the no-rows case and can be dropped, leaving only the generic error check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/featureflags/internal/appdb/appdb.go` around lines 138 - 144, Remove the unreachable pgx.ErrNoRows handling after querier.Query in appdb.go, since Query does not return that error for empty results and the no-rows case is already handled later by CollectOneRow. Keep the generic err check in the Query path and leave the FeatureFlag lookup logic unchanged otherwise.server/featureflags/internal/routes/routes.go (1)
24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParameter name shadows the imported
handlerspackage.
handlers *handlers.Handlersshadows thehandlersimport within the function body. It works here but is a readability/footgun risk if the package is later referenced insideRegister. ConsiderhandlerSet *handlers.Handlers(consistent with the rest of the cohort).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/featureflags/internal/routes/routes.go` at line 24, The Register function’s parameter name shadows the imported handlers package, which can become confusing or break future references inside the function body. Rename the parameter in Register from handlers to a clearer name like handlerSet, and update any uses within the function to match while keeping the *handlers.Handlers type unchanged.server/featureflags/featureflags.go (1)
45-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated store/service/handler wiring.
NewFeatureFlagRequestAdapterandRegisterbuild the identicalstore → svc → handlerSetchain. Consider extracting a small private constructor (e.g.newHandlers(pool)) so both entry points share one source of truth.♻️ Proposed refactor
+func newHandlers(pool *pgxpool.Pool) *handlers.Handlers { + var ( + store = appdb.NewStore(pool) + svc = services.NewService(store) + ) + return handlers.NewHandlersContainer(svc) +} + func NewFeatureFlagRequestAdapter(pool *pgxpool.Pool) FeatureFlagRequestAdapter { - var ( - store = appdb.NewStore(pool) - svc = services.NewService(store) - handlerSet = handlers.NewHandlersContainer(svc) - ) - - return handlerSet + return newHandlers(pool) } func Register(routerInst *router.Router, pool *pgxpool.Pool) { - var ( - store = appdb.NewStore(pool) - svc = services.NewService(store) - handlerSet = handlers.NewHandlersContainer(svc) - ) - - routes.Register(routerInst, handlerSet) + routes.Register(routerInst, newHandlers(pool)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/featureflags/featureflags.go` around lines 45 - 62, Both NewFeatureFlagRequestAdapter and Register duplicate the same appdb.NewStore -> services.NewService -> handlers.NewHandlersContainer wiring; extract that shared setup into a small private helper (for example, a newHandlers function) and have both entry points call it so the featureflags package has a single source of truth for constructing the handler set.server/featureflags/internal/handlers/handlers_test.go (1)
213-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the unauthenticated case on the real handler contract.
Line 216 builds a request with no
bhctx.Context, butHandlers.ToggleFlagreadsbhctx.FromRequest(request).AuthCtx. Attaching an emptybhctx.Contexthere would exercise the intended “no user on auth context” branch instead of depending on missing-context fallback behavior.Suggested test setup
} else { req, err := http.NewRequest(http.MethodPut, "/api/v2/features/"+tt.featureID+"/toggle", nil) require.NoError(t, err) - request = req + request = bhctx.SetRequestContext(req, &bhctx.Context{}) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/featureflags/internal/handlers/handlers_test.go` around lines 213 - 220, The unauthenticated test setup in handlers_test.go should use the real handler contract by providing an empty bhctx.Context instead of a request with no bhctx.Context at all. Update the test case around the request creation for ToggleFlag so that it still calls Handlers.ToggleFlag through bhctx.FromRequest(request).AuthCtx, but exercises the “no user on auth context” path explicitly rather than relying on fallback behavior from a missing context.server/featureflags/featureflags_e2e_test.go (1)
247-257: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the toggle was persisted, not just returned.
This test only checks the response body. Because
Service.ToggleFlagflips the struct before callingSetFlag, it would still pass if the write path regressed after the in-memory update. Add a follow-up read againstfeature_flagsto verify the committedenabledstate.Suggested assertion
var envelope featureFlagEnvelope require.NoError(t, json.NewDecoder(resp.Body).Decode(&envelope)) assert.Equal(t, featureID, envelope.Data.ID) assert.True(t, envelope.Data.Enabled, "Enabled should have flipped from false to true") + + var persistedEnabled bool + err = db.Pool().QueryRow(ctx, `SELECT enabled FROM feature_flags WHERE id = $1`, featureID).Scan(&persistedEnabled) + require.NoError(t, err) + assert.True(t, persistedEnabled, "toggled value should be persisted") })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/featureflags/featureflags_e2e_test.go` around lines 247 - 257, The toggle test currently only validates the HTTP response from newToggleRequest and featureFlagEnvelope, so it can miss a broken persistence write after Service.ToggleFlag updates the in-memory struct. After asserting the response, add a follow-up read against feature_flags to verify the committed enabled state for featureID, and assert that the stored value matches the toggled result rather than relying only on the returned envelope.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/featureflags/featureflags_e2e_test.go`:
- Around line 104-106: The parsing in the feature flag E2E setup is incomplete
because `strings.FieldsSeq` is iterated but the split key/value pairs are not
being stored; update the loop in the `environmentMap` construction so the
`strings.SplitN` result (`parts`) is assigned into the map when the pair is
valid. Use the `cfg.Database.Connection` parsing block in the test helper to
ensure each extracted entry is mapped from `parts[0]` to `parts[1]`.
In `@server/featureflags/internal/appdb/appdb.go`:
- Around line 266-273: The UPDATE path in appdb.UpdateFeatureFlag is checking
pgx.ErrNoRows after tx.Exec, but Exec won’t return that for zero-row updates.
After calling tx.Exec and building the query in updateBuilder, inspect the
command tag’s RowsAffected value and return services.ErrNotFound when it is 0;
keep returning any real exec error unchanged. Remove the dead pgx.ErrNoRows
branch and base the not-found behavior on the Exec result instead.
In `@server/featureflags/internal/handlers/views.go`:
- Around line 26-49: The FeatureFlagView response includes a leaky DeletedAt
field that serializes incorrectly because sql.NullTime is not JSON-friendly and
BuildFeatureFlagView never populates it. Remove DeletedAt from FeatureFlagView
and from any related JSON contract, then update BuildFeatureFlagView to return
only the populated fields from services.FeatureFlag; if a deleted timestamp is
truly needed later, switch to a properly marshaled time type such as *time.Time
with omitempty. Also drop the now-unused database/sql import after removing the
field.
---
Nitpick comments:
In `@server/featureflags/featureflags_e2e_test.go`:
- Around line 247-257: The toggle test currently only validates the HTTP
response from newToggleRequest and featureFlagEnvelope, so it can miss a broken
persistence write after Service.ToggleFlag updates the in-memory struct. After
asserting the response, add a follow-up read against feature_flags to verify the
committed enabled state for featureID, and assert that the stored value matches
the toggled result rather than relying only on the returned envelope.
In `@server/featureflags/featureflags.go`:
- Around line 45-62: Both NewFeatureFlagRequestAdapter and Register duplicate
the same appdb.NewStore -> services.NewService -> handlers.NewHandlersContainer
wiring; extract that shared setup into a small private helper (for example, a
newHandlers function) and have both entry points call it so the featureflags
package has a single source of truth for constructing the handler set.
In `@server/featureflags/internal/appdb/appdb.go`:
- Around line 138-144: Remove the unreachable pgx.ErrNoRows handling after
querier.Query in appdb.go, since Query does not return that error for empty
results and the no-rows case is already handled later by CollectOneRow. Keep the
generic err check in the Query path and leave the FeatureFlag lookup logic
unchanged otherwise.
In `@server/featureflags/internal/handlers/handlers_test.go`:
- Around line 213-220: The unauthenticated test setup in handlers_test.go should
use the real handler contract by providing an empty bhctx.Context instead of a
request with no bhctx.Context at all. Update the test case around the request
creation for ToggleFlag so that it still calls Handlers.ToggleFlag through
bhctx.FromRequest(request).AuthCtx, but exercises the “no user on auth context”
path explicitly rather than relying on fallback behavior from a missing context.
In `@server/featureflags/internal/routes/routes.go`:
- Line 24: The Register function’s parameter name shadows the imported handlers
package, which can become confusing or break future references inside the
function body. Rename the parameter in Register from handlers to a clearer name
like handlerSet, and update any uses within the function to match while keeping
the *handlers.Handlers type unchanged.
In `@server/featureflags/internal/services/services.go`:
- Around line 110-127: ToggleFlag currently accepts requestedBy but never uses
it, so the caller-supplied audit actor is discarded. Update Service.ToggleFlag
and its SetFlag call path to either remove requestedBy entirely or pass it
through so the audit write records that exact value instead of resolving a
different actor from context; use ToggleFlag and SetFlag as the key symbols when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7d4eb8e4-f741-400d-b7f4-357685694f9f
📒 Files selected for processing (21)
cmd/api/src/api/registration/v2.gocmd/api/src/api/v2/flag.gocmd/api/src/api/v2/flag_test.goserver/featureflags/featureflags.goserver/featureflags/featureflags_e2e_test.goserver/featureflags/featureflags_test.goserver/featureflags/internal/appdb/appdb.goserver/featureflags/internal/appdb/appdb_integration_test.goserver/featureflags/internal/appdb/appdb_test.goserver/featureflags/internal/handlers/handlers.goserver/featureflags/internal/handlers/handlers_test.goserver/featureflags/internal/handlers/mocks/featureflag.goserver/featureflags/internal/handlers/views.goserver/featureflags/internal/routes/routes.goserver/featureflags/internal/routes/routes_test.goserver/featureflags/internal/services/mocks/database.goserver/featureflags/internal/services/services.goserver/featureflags/internal/services/services_test.goserver/featureflags/mocks/featureflagrequestadapter.goserver/modules/modules.goserver/modules/modules_test.go
💤 Files with no reviewable changes (3)
- cmd/api/src/api/registration/v2.go
- cmd/api/src/api/v2/flag_test.go
- cmd/api/src/api/v2/flag.go
| sqlQuery, args := updateBuilder.Build() | ||
| _, err = tx.Exec(ctx, sqlQuery, args...) | ||
| if errors.Is(err, pgx.ErrNoRows) { | ||
| return services.ErrNotFound | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🌐 Web query:
Does pgx v5 Conn/Tx Exec return pgx.ErrNoRows when an UPDATE statement affects zero rows, or does it return a CommandTag with RowsAffected of 0?
💡 Result:
In pgx v5, the Exec method does not return pgx.ErrNoRows when an UPDATE statement affects zero rows [1]. Instead, it returns a pgconn.CommandTag with a RowsAffected count of 0, which is considered a successful execution [1]. The pgx.ErrNoRows error is specific to QueryRow operations (and its variants like CollectOneRow) when they are expected to return exactly one row but find none [2][3]. Since an UPDATE statement is a command rather than a query that returns a result set, it does not trigger the "no rows found" error condition [3]. To check how many rows were updated, you should inspect the CommandTag returned by Exec: result, err:= conn.Exec(ctx, "UPDATE my_table SET val = $1 WHERE id = $2", val, id) if err!= nil { // handle error } rowsAffected:= result.RowsAffected if rowsAffected == 0 { // handle the case where no rows were updated }
Citations:
- 1: https://go.pact.im/doc/pkg/github.com/jackc/pgx/v5.html
- 2: https://pkg.go.dev/github.com/jackc/pgx/v5
- 3: Query or QueryRows does not return ErrNoRows error when no record is found jackc/pgx#2251
Replace pgx.ErrNoRows check with RowsAffected check for UPDATE operations
pgx.ErrNoRows is only returned by QueryRow operations when zero rows are found, not by Exec. An UPDATE affecting zero rows returns nil error with RowsAffected() == 0. The current check is dead code; inspect the command tag to return services.ErrNotFound when no rows are updated.
Proposed fix
- sqlQuery, args := updateBuilder.Build()
- _, err = tx.Exec(ctx, sqlQuery, args...)
- if errors.Is(err, pgx.ErrNoRows) {
- return services.ErrNotFound
- }
- if err != nil {
- return err
- }
+ sqlQuery, args := updateBuilder.Build()
+ commandTag, err := tx.Exec(ctx, sqlQuery, args...)
+ if err != nil {
+ return err
+ }
+ if commandTag.RowsAffected() == 0 {
+ return services.ErrNotFound
+ }📝 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.
| sqlQuery, args := updateBuilder.Build() | |
| _, err = tx.Exec(ctx, sqlQuery, args...) | |
| if errors.Is(err, pgx.ErrNoRows) { | |
| return services.ErrNotFound | |
| } | |
| if err != nil { | |
| return err | |
| } | |
| sqlQuery, args := updateBuilder.Build() | |
| commandTag, err := tx.Exec(ctx, sqlQuery, args...) | |
| if err != nil { | |
| return err | |
| } | |
| if commandTag.RowsAffected() == 0 { | |
| return services.ErrNotFound | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/featureflags/internal/appdb/appdb.go` around lines 266 - 273, The
UPDATE path in appdb.UpdateFeatureFlag is checking pgx.ErrNoRows after tx.Exec,
but Exec won’t return that for zero-row updates. After calling tx.Exec and
building the query in updateBuilder, inspect the command tag’s RowsAffected
value and return services.ErrNotFound when it is 0; keep returning any real exec
error unchanged. Remove the dead pgx.ErrNoRows branch and base the not-found
behavior on the Exec result instead.
stephanieslamb
left a comment
stephanieslamb
left a comment
There was a problem hiding this comment.
Looks good! I still want to do a quick peek through test cases. But I think with these changes, it should be good.
| // feature_id path parameter. The handler delegates to the service layer, which | ||
| // loads the existing flag, validates that it is user-updatable, flips its | ||
| // Enabled value, persists the result, and conditionally triggers an analysis | ||
| // request when the ADCS flag is being disabled. |
There was a problem hiding this comment.
should probably update this comment since we removed the ADCS check and this no longer triggers analysis
| type Service interface { | ||
| type FeatureFlagRequestAdapter interface { | ||
| IsEnabled(ctx context.Context, key string) (bool, error) | ||
| ToggleFlag(response http.ResponseWriter, request *http.Request) |
There was a problem hiding this comment.
I'm not sure this is needed in this interface? Is ToggleFlag called cross slice?
There was a problem hiding this comment.
i went ahead and just removed it. on the off chance that it is being used elsewhere, we can always add it back when/if another slice uses it.
| IsEnabled(ctx context.Context, key string) (bool, error) | ||
| } | ||
|
|
||
| // Handlers is a dependency injection container for app_config handlers |
There was a problem hiding this comment.
Handlers is a dependency injection container for featureflags handlers
| // handleAppConfigError maps service-layer errors to HTTP responses, translating | ||
| // known sentinel errors to their corresponding status codes and falling back to | ||
| // a logged 500 for anything unexpected. | ||
| func handleAppConfigError(request *http.Request, response http.ResponseWriter, err error) { |
There was a problem hiding this comment.
handleAppConfigError -> handleFeatureFlagError
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/modules/modules_test.go`:
- Around line 40-48: The panic tests in modules_test.go only verify that
modules.Register panics, so they can pass on unrelated failures; update
TestRegister_PanicsOnNilRouter and the other nil-guard tests to assert the exact
panic value with assert.PanicsWithValue. Use the concrete nil-check messages
from modules.Register as the expected values, so the contract is pinned to the
intended guard rather than any panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d66a1c87-0b32-4a22-b0f2-e2e2f9736e4f
📒 Files selected for processing (3)
cmd/api/src/api/registration/v2.goserver/modules/modules.goserver/modules/modules_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/modules/modules.go
- cmd/api/src/api/registration/v2.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/modules/modules_test.go`:
- Around line 40-48: The panic tests in modules_test.go only verify that
modules.Register panics, so they can pass on unrelated failures; update
TestRegister_PanicsOnNilRouter and the other nil-guard tests to assert the exact
panic value with assert.PanicsWithValue. Use the concrete nil-check messages
from modules.Register as the expected values, so the contract is pinned to the
intended guard rather than any panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d66a1c87-0b32-4a22-b0f2-e2e2f9736e4f
📒 Files selected for processing (3)
cmd/api/src/api/registration/v2.goserver/modules/modules.goserver/modules/modules_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/modules/modules.go
- cmd/api/src/api/registration/v2.go
🛑 Comments failed to post (1)
server/modules/modules_test.go (1)
40-48: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the expected panic value.
These tests currently pass on any panic, so a downstream failure in module registration would still look like the intended nil-guard fired. Using
assert.PanicsWithValuehere makes the test pin the contract inmodules.Register.Proposed fix
- assert.Panics(t, func() { + assert.PanicsWithValue(t, "modules: Register requires a non-nil Router", func() { modules.Register(modules.Deps{ Router: nil, Pool: new(pgxpool.Pool), Graph: &graph.DatabaseSwitch{}, RateLimitMiddleware: noopRateLimit, }) })- assert.Panics(t, func() { + assert.PanicsWithValue(t, "modules: Register requires a non-nil Pool", func() { modules.Register(modules.Deps{ Router: &routerInst, Pool: nil, Graph: &graph.DatabaseSwitch{}, RateLimitMiddleware: noopRateLimit, }) })- assert.Panics(t, func() { + assert.PanicsWithValue(t, "modules: Register requires a non-nil Graph", func() { modules.Register(modules.Deps{ Router: &routerInst, Pool: new(pgxpool.Pool), Graph: nil, RateLimitMiddleware: noopRateLimit, }) })- assert.Panics(t, func() { + assert.PanicsWithValue(t, "modules: Register requires a non-nil RateLimitMiddleware", func() { modules.Register(modules.Deps{ Router: &routerInst, Pool: new(pgxpool.Pool), Graph: &graph.DatabaseSwitch{}, RateLimitMiddleware: nil, }) })Also applies to: 58-65, 75-82, 92-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/modules/modules_test.go` around lines 40 - 48, The panic tests in modules_test.go only verify that modules.Register panics, so they can pass on unrelated failures; update TestRegister_PanicsOnNilRouter and the other nil-guard tests to assert the exact panic value with assert.PanicsWithValue. Use the concrete nil-check messages from modules.Register as the expected values, so the contract is pinned to the intended guard rather than any panic.
| } | ||
|
|
||
| // ToggleFlag enables/disables the feature flag by the feature flag id | ||
| func (s *Service) ToggleFlag(ctx context.Context, id int32, requestedBy string) (FeatureFlag, error) { |
There was a problem hiding this comment.
I don't think requestedBy is being used. I think this is currently captured in the insertAuditLog function.
| ID int32 `json:"id"` | ||
| CreatedAt time.Time `json:"created_at"` | ||
| UpdatedAt time.Time `json:"updated_at"` | ||
| DeletedAt time.Time `json:"deleted_at"` |
There was a problem hiding this comment.
DeletedAt can be removed
coderabbitai
Bot
removed
api
2 participants
Description
We want to move ToggleFlag to the new API architecture. The handler should be moved out of v2 into the server folder following the new pattern.
Motivation and Context
Resolves BED-8586
Why is this change required? What problem does it solve?
We are migrating endpoints to use the new architecture.
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
i used bruno to test the endpoints and ran all of the tests that were added/modified.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
GET /api/v2/configandPUT /api/v2/config.GET /api/v2/featuresandPUT /api/v2/features/{feature_id}/toggle.400, missing flags return404, and non-user-updatable flags return403.