test(api): added v3 product catalog e2e tests#4201
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:
📝 WalkthroughWalkthroughAdds comprehensive v3 end-to-end tests and a lightweight v3 test client, documents e2e practices, defers plan currency validation from handler to domain validation, and adds post-merge compatibility validation when updating plan‑addon assignments. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as Test (v3Client)
participant API as API v3 Server
participant Service as ProductCatalog Service
participant DB as Database
Tester->>API: POST /api/v3/... (create plan/addon/attach)
API->>Service: convert & validate request
Service->>DB: persist draft resource
DB-->>Service: OK
Service-->>API: 201 Draft response
API-->>Tester: created (Draft)
Tester->>API: POST /api/v3/.../publish
API->>Service: publish -> run domain validations (including merged plan-addon checks)
Service->>DB: update status / set EffectiveFrom or EffectiveTo
DB-->>Service: OK
Service-->>API: 200/204 (Published/Updated)
API-->>Tester: publish response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
rolosp
force-pushed
the
chore/productcatalog-v3-e2e
branch
from
fb0cf2f to
d81a4ae
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (8)
e2e/planaddons_v3_test.go (3)
216-300: Great matrix — one small coverage gap worth noting.The
multiple + max_quantity=0 → schema minimumrow confirms the TypeSpec binder catches<1before the domain validator runs, which is exactly the right shape. Nice.One gap: negative values (e.g.
max_quantity=-1). If the schema minimum is1, negatives should hit the sameminimumrule, but there's a non-zero chance they decode/validate differently (signed int parsing edge, or the binder interpreting the sign). A single extra row would close that door:♻️ Optional extra row
{ // Caught by the TypeSpec binder (minimum: 1), not the domain // validator — the schema layer fires first for max_quantity<1. name: "multiple + max_quantity=0 → 400 schema minimum", instanceType: apiv3.AddonInstanceTypeMultiple, maxQuantity: lo.ToPtr(0), expectedStatus: http.StatusBadRequest, expectedRule: "minimum", }, + { + name: "multiple + max_quantity=-1 → 400 schema minimum", + instanceType: apiv3.AddonInstanceTypeMultiple, + maxQuantity: lo.ToPtr(-1), + expectedStatus: http.StatusBadRequest, + expectedRule: "minimum", + },Non-blocking; ship it either way.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/planaddons_v3_test.go` around lines 216 - 300, Add one more test case to TestV3PlanAddonInstanceTypeMaxQuantityMatrix’s cases table: a row for instanceType apiv3.AddonInstanceTypeMultiple with maxQuantity set to lo.ToPtr(-1) and expectedStatus http.StatusBadRequest with expectedRule "minimum". Place it near the existing "multiple + max_quantity=0 → 400 schema minimum" case in the cases slice so the binder/schema-layer behavior for negative values is covered by the same assertion paths (assertInvalidParameterRule / expectedRule).
368-386: Nice test — assertion could be a touch stricter.The test confirms
publish plan against archived addon → 400, which is the important bit. Since we're here, consider also asserting on the specific problem code or detail so a future refactor that accidentally returns 400 for the wrong reason (e.g., transient 400 from a different validator) still fails meaningfully:♻️ Optional: pin the rejection reason
status, _, problem = c.PublishPlan(planID) assert.Equal(t, http.StatusBadRequest, status, "publish against archived addon should reject: %+v", problem) require.NotNil(t, problem) + // Pin the reason so a future 400-for-a-different-reason doesn't slip by. + // assertValidationCode(t, problem, "plan_addon_incompatible_status") + // or assertProblemDetail(t, problem, "<expected substring>")Per the skill doc's own "write it, run it, inspect the problem" guidance — once you've seen the response shape on a real run, uncommenting one of these would tighten this up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/planaddons_v3_test.go` around lines 368 - 386, In TestV3PlanAddonPublishAgainstArchivedAddon, tighten the failure check by asserting the specific rejection reason from the returned problem object (rather than only status 400) — inspect the real test run to see the problem structure and add an assertion against the appropriate field on problem (for example/assert-style: require.Equal(t, "<expected_problem_code>", problem.Code) or require.Contains(t, problem.Detail, "archived") or assert.Contains(t, problem.Message, "archived")), keeping the existing status and non-nil checks; update the assertions around the PublishPlan call in this test to include that extra check.
327-345: Pinning observed behavior is fine, but worth a breadcrumb for future-you.The "delete from deleted plan returns 400 with
plan is deleteddetail" is a behavior choice the server could reasonably flip to 404 later. The comment on line 328-329 acknowledges that. As a belt-and-suspenders move, consider accepting both shapes so this test doesn't become a tripwire the day someone tidies the handler:♻️ Optional: accept either pinned behavior
- status, problem = c.DetachAddon(planID, planAddonID) - assert.Equal(t, http.StatusBadRequest, status, "detach from deleted: %+v", problem) - assertProblemDetail(t, problem, "plan is deleted") + status, problem = c.DetachAddon(planID, planAddonID) + assert.Contains(t, []int{http.StatusBadRequest, http.StatusNotFound}, status, + "detach from deleted: %+v", problem) + if status == http.StatusBadRequest { + assertProblemDetail(t, problem, "plan is deleted") + }Totally optional — sometimes pinning is the point of the test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/planaddons_v3_test.go` around lines 327 - 345, The test currently strictly expects a 400 with detail "plan is deleted" when calling c.DetachAddon in the t.Run "detach from a deleted plan → 400 'plan is deleted'"; relax it to accept either the pinned behavior or a 404 by checking the returned status from c.DetachAddon and asserting: if status == http.StatusBadRequest then call assertProblemDetail(t, problem, "plan is deleted"), otherwise assert status == http.StatusNotFound (and optionally assert problem is nil or has a not-found detail); keep the existing setupAttachedPlanAddon, PublishPlan/ArchivePlan/DeletePlan sequence and the test name intact.e2e/plans_v3_test.go (1)
386-403: Table readability nit — the inlineisLastcomments are a head-scratcher.The cases are correct (first phase of a 2-phase slice is the "non-last", the last is the "last"), but the inline comments describe what
isLast=true/falsemeans tovalidPlanPhase, not what the test is asserting. SononLast := validPlanPhase("non_last", true /* isLast — no duration */)reads as "this IS last, has no duration" while the variable name + test name say it's a non-last phase. It clicks once you trace through, but it slowed me down.A small rename of the local vars or flipping the comment to describe the resulting defect would help:
♻️ One shape for clearer reading
{ name: "non-last phase missing duration → plan_has_non_last_phase_with_no_duration", mutate: func(r *apiv3.CreatePlanRequest) { - nonLast := validPlanPhase("non_last", true /* isLast — no duration */) - last := validPlanPhase("last", true /* isLast */) - r.Phases = []apiv3.BillingPlanPhase{nonLast, last} + // Both phases built with isLast=true → neither has a duration. + // The first one is positionally non-last, so it's the defect. + firstNoDuration := validPlanPhase("non_last", true) + last := validPlanPhase("last", true) + r.Phases = []apiv3.BillingPlanPhase{firstNoDuration, last} }, code: "plan_has_non_last_phase_with_no_duration", }, { name: "last phase with a duration → plan_has_last_phase_with_duration", mutate: func(r *apiv3.CreatePlanRequest) { - first := validPlanPhase("first", false /* isLast */) - last := validPlanPhase("last", false /* bounded — should be open */) + // Both phases built with isLast=false → both carry a duration. + // The second is positionally last, so its duration is the defect. + first := validPlanPhase("first", false) + lastWithDuration := validPlanPhase("last", false) - r.Phases = []apiv3.BillingPlanPhase{first, last} + r.Phases = []apiv3.BillingPlanPhase{first, lastWithDuration} }, code: "plan_has_last_phase_with_duration", },Totally optional — just a kindness to the next reader.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/plans_v3_test.go` around lines 386 - 403, The inline comments next to calls to validPlanPhase in the test cases are describing the meaning of the boolean parameter rather than the intended test scenario, which is confusing (e.g., nonLast := validPlanPhase("non_last", true /* isLast — no duration */) reads as “is last” while the test intends a non-last phase). Fix by renaming the local variables or updating the inline comments so they describe the resulting defect being tested (e.g., mark the boolean comment to indicate that passing true produces a phase with no duration, or rename nonLast→phaseWithNoDuration and last→openLastPhase), keeping references to validPlanPhase and the test case names (plan_has_non_last_phase_with_no_duration, plan_has_last_phase_with_duration) so intent is clear.e2e/addons_v3_test.go (1)
306-318: Future-proofing nit on the matrix loop.
require.NotNil(t, addon)runs unconditionally, which is fine today because every row expects201. But the table shape invites future rows withexpectedStatus=http.StatusBadRequest(there's even a comment above about dropped rate-card compatibility cases that should be re-enabled), and any of those will panic here instead of failing cleanly on the status assertion.Small safety net:
♻️ Gate the nil check on the expected status
status, addon, problem := c.CreateAddon(body) assert.Equal(t, tc.expectedStatus, status, "problem: %+v", problem) - require.NotNil(t, addon) + if tc.expectedStatus == http.StatusCreated { + require.NotNil(t, addon) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/addons_v3_test.go` around lines 306 - 318, The test unconditionally calls require.NotNil(t, addon) after c.CreateAddon(body), which will panic for cases expecting a non-201 status; change the assertion to only require.NotNil when the expected status is http.StatusCreated (201). Locate the table-driven loop that calls newV3Client(), validAddonRequest("instance_type_price"), sets body.InstanceType and body.RateCards, then calls c.CreateAddon(body); after asserting status equals tc.expectedStatus, guard the require.NotNil(t, addon) behind a conditional like if tc.expectedStatus == http.StatusCreated (or 201) so tests expecting errors fail cleanly on the status assertion instead of panicking.e2e/v3helpers_test.go (2)
341-422: Fixture builders panic onFromBillingPrice*errors — consider a shared helper.The three rate-card builders (
validFlatRateCard,validUnitRateCard,validGraduatedRateCard) each hand-roll theBillingPrice{}+FromBillingPriceXxx+panic(err)dance. For hardcoded inputs it's fine (they'll never realistically fail), but a tiny helper would tighten things and reduce the repeated boilerplate if you add more shapes later:♻️ Optional: centralize the "build-or-panic" price construction
func mustPrice(fn func(*apiv3.BillingPrice) error) apiv3.BillingPrice { p := apiv3.BillingPrice{} if err := fn(&p); err != nil { panic(fmt.Errorf("build fixture price: %w", err)) } return p } // usage: price := mustPrice(func(p *apiv3.BillingPrice) error { return p.FromBillingPriceFlat(apiv3.BillingPriceFlat{ Type: apiv3.BillingPriceFlatTypeFlat, Amount: "10", }) })Very much a nice-to-have — no correctness concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/v3helpers_test.go` around lines 341 - 422, The three fixture builders (validFlatRateCard, validUnitRateCard, validGraduatedRateCard) duplicate the BillingPrice + FromBillingPrice* + panic(err) pattern; add a small helper like mustPrice(fn func(*apiv3.BillingPrice) error) apiv3.BillingPrice that constructs a BillingPrice, calls the provided FromBillingPrice* function (FromBillingPriceFlat, FromBillingPriceUnit, FromBillingPriceGraduated) and panics with a wrapped error (fmt.Errorf("build fixture price: %w", err)) on failure, then replace the inline construction in each builder with a call to mustPrice to remove the boilerplate.
95-110: Minor: swallowed unmarshal error onvalidationErrors.If
extensions.validationErrorsis present but malformed (e.g., a server bug returns it as a single object instead of an array), this silently returnsniland the subsequentassertValidationCodewill fail with the less-informative "no validation errors in problem response" instead of telling you "validationErrors existed but didn't decode as[]v3ValidationError". Low-probability but free to surface:♻️ Surface decode errors via t.Helper for easier debugging
- b, err := json.Marshal(raw) - if err != nil { - return nil - } - var out []v3ValidationError - _ = json.Unmarshal(b, &out) - return out + b, err := json.Marshal(raw) + if err != nil { + return nil + } + var out []v3ValidationError + if err := json.Unmarshal(b, &out); err != nil { + return nil + } + return outTotally optional — the current behavior won't hide real bugs, just makes the failure message less precise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/v3helpers_test.go` around lines 95 - 110, The current v3Problem.ValidationErrors silently swallows json.Marshal/json.Unmarshal errors; update it to surface decode failures in tests by changing the method to accept t *testing.T (e.g., func (p *v3Problem) ValidationErrors(t *testing.T) []v3ValidationError), call t.Helper(), and call t.Fatalf with a clear message if Marshal/Unmarshal returns an error so malformed extensions.validationErrors produce an actionable test failure; update all test callers to pass their *testing.T accordingly..agents/skills/e2e/SKILL.md (1)
88-98: Tiny heads-up on the illustrative snippet.The
api.UpsertSubjectJSONRequestBody{ api.SubjectUpsert{Key: "customer-1"} }literal reads oddly — oapi-codegen usually emitsUpsertSubjectJSONRequestBodyas a type alias forSubjectUpsert, so either the outer wrapper is redundant or (if it's a struct) the inner field needs a name. Since this block is the "copy me as a template" example in the skill doc, a compiling snippet would save the next reader a detour throughapi.gen.go.Not blocking — just a paper-cut for future-you.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/e2e/SKILL.md around lines 88 - 98, The example TestIngest snippet uses a nested composite literal that won't compile; replace the inner/outer mismatch when calling UpsertSubjectWithResponse. Locate TestIngest and change the request arg from api.UpsertSubjectJSONRequestBody{ api.SubjectUpsert{Key: "customer-1"} } to the correct literal form (either api.UpsertSubjectJSONRequestBody{Key: "customer-1"} if UpsertSubjectJSONRequestBody is a struct, or simply api.SubjectUpsert{Key: "customer-1"} if UpsertSubjectJSONRequestBody is a type alias) so the call to UpsertSubjectWithResponse compiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.agents/skills/e2e/SKILL.md:
- Around line 88-98: The example TestIngest snippet uses a nested composite
literal that won't compile; replace the inner/outer mismatch when calling
UpsertSubjectWithResponse. Locate TestIngest and change the request arg from
api.UpsertSubjectJSONRequestBody{ api.SubjectUpsert{Key: "customer-1"} } to the
correct literal form (either api.UpsertSubjectJSONRequestBody{Key: "customer-1"}
if UpsertSubjectJSONRequestBody is a struct, or simply api.SubjectUpsert{Key:
"customer-1"} if UpsertSubjectJSONRequestBody is a type alias) so the call to
UpsertSubjectWithResponse compiles.
In `@e2e/addons_v3_test.go`:
- Around line 306-318: The test unconditionally calls require.NotNil(t, addon)
after c.CreateAddon(body), which will panic for cases expecting a non-201
status; change the assertion to only require.NotNil when the expected status is
http.StatusCreated (201). Locate the table-driven loop that calls newV3Client(),
validAddonRequest("instance_type_price"), sets body.InstanceType and
body.RateCards, then calls c.CreateAddon(body); after asserting status equals
tc.expectedStatus, guard the require.NotNil(t, addon) behind a conditional like
if tc.expectedStatus == http.StatusCreated (or 201) so tests expecting errors
fail cleanly on the status assertion instead of panicking.
In `@e2e/planaddons_v3_test.go`:
- Around line 216-300: Add one more test case to
TestV3PlanAddonInstanceTypeMaxQuantityMatrix’s cases table: a row for
instanceType apiv3.AddonInstanceTypeMultiple with maxQuantity set to
lo.ToPtr(-1) and expectedStatus http.StatusBadRequest with expectedRule
"minimum". Place it near the existing "multiple + max_quantity=0 → 400 schema
minimum" case in the cases slice so the binder/schema-layer behavior for
negative values is covered by the same assertion paths
(assertInvalidParameterRule / expectedRule).
- Around line 368-386: In TestV3PlanAddonPublishAgainstArchivedAddon, tighten
the failure check by asserting the specific rejection reason from the returned
problem object (rather than only status 400) — inspect the real test run to see
the problem structure and add an assertion against the appropriate field on
problem (for example/assert-style: require.Equal(t, "<expected_problem_code>",
problem.Code) or require.Contains(t, problem.Detail, "archived") or
assert.Contains(t, problem.Message, "archived")), keeping the existing status
and non-nil checks; update the assertions around the PublishPlan call in this
test to include that extra check.
- Around line 327-345: The test currently strictly expects a 400 with detail
"plan is deleted" when calling c.DetachAddon in the t.Run "detach from a deleted
plan → 400 'plan is deleted'"; relax it to accept either the pinned behavior or
a 404 by checking the returned status from c.DetachAddon and asserting: if
status == http.StatusBadRequest then call assertProblemDetail(t, problem, "plan
is deleted"), otherwise assert status == http.StatusNotFound (and optionally
assert problem is nil or has a not-found detail); keep the existing
setupAttachedPlanAddon, PublishPlan/ArchivePlan/DeletePlan sequence and the test
name intact.
In `@e2e/plans_v3_test.go`:
- Around line 386-403: The inline comments next to calls to validPlanPhase in
the test cases are describing the meaning of the boolean parameter rather than
the intended test scenario, which is confusing (e.g., nonLast :=
validPlanPhase("non_last", true /* isLast — no duration */) reads as “is last”
while the test intends a non-last phase). Fix by renaming the local variables or
updating the inline comments so they describe the resulting defect being tested
(e.g., mark the boolean comment to indicate that passing true produces a phase
with no duration, or rename nonLast→phaseWithNoDuration and last→openLastPhase),
keeping references to validPlanPhase and the test case names
(plan_has_non_last_phase_with_no_duration, plan_has_last_phase_with_duration) so
intent is clear.
In `@e2e/v3helpers_test.go`:
- Around line 341-422: The three fixture builders (validFlatRateCard,
validUnitRateCard, validGraduatedRateCard) duplicate the BillingPrice +
FromBillingPrice* + panic(err) pattern; add a small helper like mustPrice(fn
func(*apiv3.BillingPrice) error) apiv3.BillingPrice that constructs a
BillingPrice, calls the provided FromBillingPrice* function
(FromBillingPriceFlat, FromBillingPriceUnit, FromBillingPriceGraduated) and
panics with a wrapped error (fmt.Errorf("build fixture price: %w", err)) on
failure, then replace the inline construction in each builder with a call to
mustPrice to remove the boilerplate.
- Around line 95-110: The current v3Problem.ValidationErrors silently swallows
json.Marshal/json.Unmarshal errors; update it to surface decode failures in
tests by changing the method to accept t *testing.T (e.g., func (p *v3Problem)
ValidationErrors(t *testing.T) []v3ValidationError), call t.Helper(), and call
t.Fatalf with a clear message if Marshal/Unmarshal returns an error so malformed
extensions.validationErrors produce an actionable test failure; update all test
callers to pass their *testing.T accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a0f5f711-a1c2-4660-9f2a-33dbdce87292
📒 Files selected for processing (9)
.agents/skills/e2e/SKILL.mdapi/v3/handlers/plans/convert.goapi/v3/handlers/plans/convert_test.goe2e/addons_v3_test.goe2e/planaddons_v3_test.goe2e/plans_v3_test.goe2e/v3helpers_test.goopenmeter/productcatalog/planaddon.goopenmeter/productcatalog/planaddon/service/planaddon.go
💤 Files with no reviewable changes (1)
- api/v3/handlers/plans/convert_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/planaddons_v3_test.go (1)
366-376: Consider pinning the active-plan detach failure reason.The
detach from an active plan → 400case only checks the status code. Since400can come from many paths, a quickassertProblemDetail(orassertValidationCode) tying this to the actual "plan is active/not draft" reason would make regressions easier to spot — similar to how the deleted-plan case is pinned at line 395. Totally optional, just a nice-to-have for symmetry.💡 Suggested tweak
status, problem = c.DetachAddon(planID, planAddonID) assert.Equal(t, http.StatusBadRequest, status, "detach from active should reject: %+v", problem) + require.NotNil(t, problem) + assert.NotEmpty(t, problem.Detail, "expected a reason in problem.Detail")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/planaddons_v3_test.go` around lines 366 - 376, The test "detach from an active plan → 400" currently only asserts the HTTP 400 status; update it to also assert the specific failure reason by calling assertProblemDetail or assertValidationCode after c.DetachAddon to verify the error maps to "plan is active" (or the project's validation code for non-draft plans), using the same planID/planAddonID from setupAttachedPlanAddon and the response/problem returned by c.DetachAddon so the test pins the rejection to the active-plan-not-draft condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/planaddons_v3_test.go`:
- Around line 366-376: The test "detach from an active plan → 400" currently
only asserts the HTTP 400 status; update it to also assert the specific failure
reason by calling assertProblemDetail or assertValidationCode after
c.DetachAddon to verify the error maps to "plan is active" (or the project's
validation code for non-draft plans), using the same planID/planAddonID from
setupAttachedPlanAddon and the response/problem returned by c.DetachAddon so the
test pins the rejection to the active-plan-not-draft condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ee3a620-7072-4e28-97ad-fbf619321a2a
📒 Files selected for processing (2)
e2e/planaddons_v3_test.goopenmeter/productcatalog/planaddon/service/planaddon.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/productcatalog/planaddon/service/planaddon.go
There was a problem hiding this comment.
🧹 Nitpick comments (5)
openmeter/productcatalog/planaddon/service/planaddon.go (2)
376-376: Stale log message.
logger.Debug("validating plan add-on assignment")sits right after the validate block, just before the adapter write. Reads a bit confusing — probably wants to be"persisting plan add-on assignment"(or moved above line 325 next to the real validation step).🪵 Tiny tweak
- logger.Debug("validating plan add-on assignment") + logger.Debug("persisting plan add-on assignment")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/planaddon/service/planaddon.go` at line 376, The log message logger.Debug("validating plan add-on assignment") is stale because it appears after the validation block and just before the adapter write; update it to accurately reflect the action (e.g., logger.Debug("persisting plan add-on assignment")) or move the existing debug call up next to the actual validation step in the function that performs validation and adapter write (refer to the validate block and the adapter write call in planaddon.go around the function handling plan add-on assignment) so logs match the real operation.
325-370: Nice catch pulling validation in here — one mild hazard to flag.The patch-merge mirrors the adapter's
SetOrClearMaxQuantity/ nil-skip semantics, and the heads-up comment calls out the coupling nicely. Two small things worth thinking about:
- This is now a silent contract between this block and
planaddon/adapter/planaddon.goUpdatePlanAddon. If someone later changes the adapter to (say) stop clearingMaxQuantitywhenparams.MaxQuantityis nil, or treatsMetadata == nildifferently, validation and persistence will disagree and we could either reject valid updates or admit invalid ones. Consider extracting the merge into a small helper shared with (or invoked from) the adapter — or at minimum add a table-test that locks in the current semantics so the drift gets caught in CI.- On failure you return
NewGenericValidationErrorwhile the siblingCreatePlanAddonpath returns the same shape, which is consistent — 👍. Just double-check the underlyingpa.Validate()error path already producesProductCatalogValidationErrorextensions when appropriate; wrapping it again here shouldn't flatten those structured details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/planaddon/service/planaddon.go` around lines 325 - 370, The validation merges incoming patch semantics locally (using params, planAddon and building pa then calling pa.Validate()) but that duplicates the patch-merge rules in planaddon/adapter/planaddon.go::UpdatePlanAddon and risks drift; refactor the merge into a single helper (e.g., MergePlanAddonPatch or ApplyPlanAddonPatch) that encapsulates the SetOrClearMaxQuantity / nil-skip behavior and is called from both UpdatePlanAddon and this validation path (use symbols planAddon, params, pa.Validate()), and add a table-driven unit test that asserts the current merge outcomes for combinations of nil/non-nil MaxQuantity, FromPlanPhase, Metadata and Annotations to lock the semantics in CI. Ensure the helper preserves existing semantics (MaxQuantity uses params.MaxQuantity as-is, FromPlanPhase/Metadata/Annotations are only overwritten when non-nil).e2e/planaddons_v3_test.go (3)
313-331: Good call pinning observed behavior — leave a breadcrumb for the spec.The comment already notes the doc-vs-server mismatch (404 vs 400 "plan is deleted"). Since this test will now enforce the 400 contract, consider opening a follow-up issue (or TODO with a link) to reconcile the OpenAPI doc with reality — otherwise the next person hitting the docs will assume 404 and be confused by this test.
Happy to draft the issue if you want.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/planaddons_v3_test.go` around lines 313 - 331, Test enforces a 400 "plan is deleted" contract that contradicts the OpenAPI doc's 404; add a short TODO comment in the test "detach from a deleted plan → 400 'plan is deleted'" (near setupAttachedPlanAddon / PublishPlan / ArchivePlan / DeletePlan / DetachAddon) that creates an issue or links an existing issue to reconcile the OpenAPI spec with the observed server behavior so future readers know this mismatch is intentional and tracked.
354-372: Minor: assertion order nit.Line 371's
require.NotNil(t, problem)runs afterassert.Equalhas already readproblemin its format arg on line 370. Ifproblemis nil, the formatter handles it fine, but the NotNil check is then redundant for the preceding line and doesn't guard anything. Either moverequire.NotNil(t, problem)before the status assert, or drop it — same pattern also shows up inTestV3PlanAddonDuplicateAttachmentRejected(line 393).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/planaddons_v3_test.go` around lines 354 - 372, The assertion order is unsafe because assert.Equal in TestV3PlanAddonPublishAgainstArchivedAddon reads the `problem` variable in its format string before the subsequent require.NotNil(t, problem) can guard it; move the require.NotNil(t, problem) call to immediately before the status assertion (i.e., place it right after the ArchiveAddon/PublishPlan call that sets `problem`) or remove the redundant NotNil, and apply the same change to TestV3PlanAddonDuplicateAttachmentRejected so `problem` is checked before any formatting that references it.
152-186: Brittle substring match on server error strings.
expectedDetailInvalues like"invalid active status, allowed statuses: [draft scheduled]"couple the e2e suite to the exact phrasing and slice-formatting of domain error messages. A harmless rewording (or switching fromfmt.Sprintf("%v", slice)to a comma-joined list) will turn these into red flakes even though behavior is correct. If the API exposes a stable error code ortype/extensionsfield for these, asserting on that would be a lot more durable; otherwise maybe match on just"allowed statuses"+ the offending status keyword.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/planaddons_v3_test.go` around lines 152 - 186, The test uses brittle full-string substring checks via expectedDetailIn in the table-driven cases; update the assertions in the test loop that reads expectedDetailIn to instead validate a stable error identifier or structured field (e.g., response JSON "code", "type", or "extensions") when available, and if not available assert on minimal, robust fragments such as the phrase "allowed statuses" plus the specific offending status (use the planStatus/addonStatus values from each case) rather than the full formatted slice text; locate the assertion logic that consumes expectedDetailIn in the test function and replace it with these more durable checks referencing expectedDetailIn (or replace expectedDetailIn entries with the small fragments) so rewording or slice formatting changes won't break the tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/planaddons_v3_test.go`:
- Around line 313-331: Test enforces a 400 "plan is deleted" contract that
contradicts the OpenAPI doc's 404; add a short TODO comment in the test "detach
from a deleted plan → 400 'plan is deleted'" (near setupAttachedPlanAddon /
PublishPlan / ArchivePlan / DeletePlan / DetachAddon) that creates an issue or
links an existing issue to reconcile the OpenAPI spec with the observed server
behavior so future readers know this mismatch is intentional and tracked.
- Around line 354-372: The assertion order is unsafe because assert.Equal in
TestV3PlanAddonPublishAgainstArchivedAddon reads the `problem` variable in its
format string before the subsequent require.NotNil(t, problem) can guard it;
move the require.NotNil(t, problem) call to immediately before the status
assertion (i.e., place it right after the ArchiveAddon/PublishPlan call that
sets `problem`) or remove the redundant NotNil, and apply the same change to
TestV3PlanAddonDuplicateAttachmentRejected so `problem` is checked before any
formatting that references it.
- Around line 152-186: The test uses brittle full-string substring checks via
expectedDetailIn in the table-driven cases; update the assertions in the test
loop that reads expectedDetailIn to instead validate a stable error identifier
or structured field (e.g., response JSON "code", "type", or "extensions") when
available, and if not available assert on minimal, robust fragments such as the
phrase "allowed statuses" plus the specific offending status (use the
planStatus/addonStatus values from each case) rather than the full formatted
slice text; locate the assertion logic that consumes expectedDetailIn in the
test function and replace it with these more durable checks referencing
expectedDetailIn (or replace expectedDetailIn entries with the small fragments)
so rewording or slice formatting changes won't break the tests.
In `@openmeter/productcatalog/planaddon/service/planaddon.go`:
- Line 376: The log message logger.Debug("validating plan add-on assignment") is
stale because it appears after the validation block and just before the adapter
write; update it to accurately reflect the action (e.g.,
logger.Debug("persisting plan add-on assignment")) or move the existing debug
call up next to the actual validation step in the function that performs
validation and adapter write (refer to the validate block and the adapter write
call in planaddon.go around the function handling plan add-on assignment) so
logs match the real operation.
- Around line 325-370: The validation merges incoming patch semantics locally
(using params, planAddon and building pa then calling pa.Validate()) but that
duplicates the patch-merge rules in
planaddon/adapter/planaddon.go::UpdatePlanAddon and risks drift; refactor the
merge into a single helper (e.g., MergePlanAddonPatch or ApplyPlanAddonPatch)
that encapsulates the SetOrClearMaxQuantity / nil-skip behavior and is called
from both UpdatePlanAddon and this validation path (use symbols planAddon,
params, pa.Validate()), and add a table-driven unit test that asserts the
current merge outcomes for combinations of nil/non-nil MaxQuantity,
FromPlanPhase, Metadata and Annotations to lock the semantics in CI. Ensure the
helper preserves existing semantics (MaxQuantity uses params.MaxQuantity as-is,
FromPlanPhase/Metadata/Annotations are only overwritten when non-nil).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13d45d76-a7dd-42d1-ab16-f7b6926a8564
📒 Files selected for processing (2)
e2e/planaddons_v3_test.goopenmeter/productcatalog/planaddon/service/planaddon.go
rolosp
force-pushed
the
chore/productcatalog-v3-e2e
branch
from
d99cbe8 to
f4a39a7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
e2e/addons_v3_test.go (2)
248-253: Heads-up: substring assertion is coupled to a server-side error string.
"feature with ID <id> not found"will break silently if anyone tweaks that error message upstream. Since the comment already notes the PC coderate_card_feature_not_foundisn't yet surfaced inextensions.validationErrors, consider leaving a TODO to switch toassertValidationCodeonce it is — that way the intent is visible and the brittle string match has a clear exit plan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/addons_v3_test.go` around lines 248 - 253, The test currently uses a brittle substring assertion via assertProblemDetail(t, problem, "feature with ID "+fakeFeatureID+" not found"); replace this with a TODO and a more robust check: keep the existing assertProblemDetail for now but add a clear TODO comment referencing rate_card_feature_not_found and assertValidationCode so future changes can swap to assertValidationCode when the server surfaces the PC code; specifically update the test around CreateAddon to (1) retain or slightly relax the existing assertion to avoid strict dependency on exact wording, (2) add a TODO mentioning rate_card_feature_not_found and assertValidationCode, and (3) annotate use of fakeFeatureID and the assertProblemDetail/ assertValidationCode functions so the intent and migration path are obvious.
285-318: Tiny nit:expectedStatusis effectively constant here.Both rows expect
http.StatusCreated, so the field isn't pulling weight yet. Totally fine to keep as scaffolding for the "re-enable once validator is wired" rows called out in the comment above — just flagging in case you'd rather drop it until those rows land. Also,require.NotNil(t, addon)on line 316 will only be safe while every case is a 2xx; worth revisiting when you add non-201 cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/addons_v3_test.go` around lines 285 - 318, The test TestV3AddonInstanceTypePriceCompatibility defines an expectedStatus field that is identical (http.StatusCreated) for all cases and uses require.NotNil(t, addon) which only holds for 2xx responses; simplify by removing the expectedStatus field from the cases and assert status == http.StatusCreated directly (or replace the per-case field with a single constant variable if you prefer), and update the require.NotNil check to run only when status is a 2xx (e.g., if status >= 200 && status < 300 { require.NotNil(t, addon) } ) so the assertion remains safe when non-201 cases are later added; target symbols: TestV3AddonInstanceTypePriceCompatibility, expectedStatus, require.NotNil.e2e/planaddons_v3_test.go (1)
97-110: Consider plugging two small coverage gaps tied to this PR's new behavior.This PR specifically introduces post-merge
PlanAddon.Validate()onUpdatePlanAddon, but the lifecycle update sub-test (Lines 97-110) only walks the happy path. A failing-update case (e.g., patchingFromPlanPhaseto a phase that doesn't exist on the plan, or otherwise driving the post-merge state to failValidate()) would directly lock in the new validation behavior called out in the PR description.Additionally, the
TestV3PlanAddonInstanceTypeMaxQuantityMatrixtable (Lines 213-286) coversmultiple + {nil, 5, 0}andsingle + nil, but missessingle + positive max_quantity, which perPlanAddon.Validate()inopenmeter/productcatalog/planaddon.goshould be rejected withErrPlanAddonMaxQuantityMustNotBeSet. That's the one branch in the matrix exercised purely by the domain validator (not the schema binder), so it's worth pinning.Happy to draft both cases if useful — otherwise totally fine to defer. Flagging as recommended since the rest of the matrix is thorough.
As per coding guidelines: "Make sure the tests are comprehensive and cover the changes."
Also applies to: 213-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/planaddons_v3_test.go` around lines 97 - 110, Add two failing-path tests to cover the new post-merge validation: 1) in the lifecycle update sub-test around UpdatePlanAddon, add a case that sets FromPlanPhase to a non-existent phase (or otherwise drives post-merge state to fail PlanAddon.Validate()) and assert the response is a non-200 status and the returned problem/error indicates validation failed (use UpdatePlanAddon and PlanAddon.Validate as anchors). 2) In TestV3PlanAddonInstanceTypeMaxQuantityMatrix, add the missing table row for instance_type == "single" with a positive max_quantity and assert the request is rejected with the domain validation error ErrPlanAddonMaxQuantityMustNotBeSet (reference TestV3PlanAddonInstanceTypeMaxQuantityMatrix and ErrPlanAddonMaxQuantityMustNotBeSet). Ensure assertions check HTTP status/problem or returned error matches the expected validation failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/addons_v3_test.go`:
- Around line 248-253: The test currently uses a brittle substring assertion via
assertProblemDetail(t, problem, "feature with ID "+fakeFeatureID+" not found");
replace this with a TODO and a more robust check: keep the existing
assertProblemDetail for now but add a clear TODO comment referencing
rate_card_feature_not_found and assertValidationCode so future changes can swap
to assertValidationCode when the server surfaces the PC code; specifically
update the test around CreateAddon to (1) retain or slightly relax the existing
assertion to avoid strict dependency on exact wording, (2) add a TODO mentioning
rate_card_feature_not_found and assertValidationCode, and (3) annotate use of
fakeFeatureID and the assertProblemDetail/ assertValidationCode functions so the
intent and migration path are obvious.
- Around line 285-318: The test TestV3AddonInstanceTypePriceCompatibility
defines an expectedStatus field that is identical (http.StatusCreated) for all
cases and uses require.NotNil(t, addon) which only holds for 2xx responses;
simplify by removing the expectedStatus field from the cases and assert status
== http.StatusCreated directly (or replace the per-case field with a single
constant variable if you prefer), and update the require.NotNil check to run
only when status is a 2xx (e.g., if status >= 200 && status < 300 {
require.NotNil(t, addon) } ) so the assertion remains safe when non-201 cases
are later added; target symbols: TestV3AddonInstanceTypePriceCompatibility,
expectedStatus, require.NotNil.
In `@e2e/planaddons_v3_test.go`:
- Around line 97-110: Add two failing-path tests to cover the new post-merge
validation: 1) in the lifecycle update sub-test around UpdatePlanAddon, add a
case that sets FromPlanPhase to a non-existent phase (or otherwise drives
post-merge state to fail PlanAddon.Validate()) and assert the response is a
non-200 status and the returned problem/error indicates validation failed (use
UpdatePlanAddon and PlanAddon.Validate as anchors). 2) In
TestV3PlanAddonInstanceTypeMaxQuantityMatrix, add the missing table row for
instance_type == "single" with a positive max_quantity and assert the request is
rejected with the domain validation error ErrPlanAddonMaxQuantityMustNotBeSet
(reference TestV3PlanAddonInstanceTypeMaxQuantityMatrix and
ErrPlanAddonMaxQuantityMustNotBeSet). Ensure assertions check HTTP
status/problem or returned error matches the expected validation failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: baa46d24-b69d-465f-aa3f-047998f9f119
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (12)
.agents/skills/e2e/SKILL.mdapi/spec/packages/aip/src/productcatalog/planaddon.tspapi/v3/api.gen.goapi/v3/handlers/plans/convert.goapi/v3/handlers/plans/convert_test.goe2e/addons_v3_test.goe2e/planaddons_v3_test.goe2e/plans_v3_test.goe2e/v3helpers_test.goopenmeter/productcatalog/errors.goopenmeter/productcatalog/planaddon.goopenmeter/productcatalog/planaddon/service/planaddon.go
💤 Files with no reviewable changes (1)
- api/v3/handlers/plans/convert_test.go
✅ Files skipped from review due to trivial changes (6)
- openmeter/productcatalog/errors.go
- openmeter/productcatalog/planaddon.go
- api/spec/packages/aip/src/productcatalog/planaddon.tsp
- .agents/skills/e2e/SKILL.md
- e2e/v3helpers_test.go
- e2e/plans_v3_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- api/v3/handlers/plans/convert.go
- openmeter/productcatalog/planaddon/service/planaddon.go
rolosp
force-pushed
the
chore/productcatalog-v3-e2e
branch
from
f4a39a7 to
4641a9c
Compare
rolosp
force-pushed
the
chore/productcatalog-v3-e2e
branch
from
9271c4a to
d16e7a2
Compare
rolosp
force-pushed
the
chore/productcatalog-v3-e2e
branch
from
d16e7a2 to
36deedc
Compare
2 participants
Overview
FromAPICreatePlanRequest; the service layer's PlanMeta.Validate() already produces
a structured ValidationIssue with the correct HTTP status and validationErrors
extensions — behaviour from the client's perspective is unchanged (still 400), but
the error shape is now consistent with all other validation errors
validates the post-merge state before persisting — builds a PlanAddon reflecting
the patched fields (applying nil = keep-existing semantics for
FromPlanPhase/Metadata/Annotations, nil = clear for MaxQuantity) and calls
PlanAddon.Validate(), returning a structured GenericValidationError on failure
e2e tests (e2e/)
fixture builders (validAddonRequest, validFlatRateCard, validUnitRateCard,
validGraduatedRateCard, etc.), and a withPageSize option
→ archive → delete → soft-delete) plus mixed rate-card round-trip and
versioning/auto-archive tests
lifecycle tests
Skill
Added .agents/skills/e2e/SKILL.md documenting the v3 e2e test patterns for future
agents.
Summary by CodeRabbit
Documentation
Tests
Bug Fixes