fix: allow editing content delivery config on projects with branching disabled - #3622
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR refines the BRANCHING feature validation in content delivery configuration updates. Rather than checking feature enablement upfront based on the Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Service as ContentDeliveryConfigService
participant Guard as ProjectFeatureGuard
participant BranchRepo as Branch Repository
User->>Service: update(config, dto)
Service->>BranchRepo: fetch active-or-default branch
BranchRepo-->>Service: newBranch
rect rgba(100, 150, 200, 0.5)
Note over Service: Conditional Feature Check
alt newBranch is non-default OR config.branch is non-default
Service->>Guard: checkEnabled(BRANCHING)
Guard-->>Service: validation result
else both branches are default
Note over Service: Skip BRANCHING check
end
end
Service->>Service: config.branch = newBranch
Service->>Service: proceed with standard update
Service-->>User: success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/cypress/e2e/branching/contentDeliveryBranching.cy.ts (1)
111-128: Nit: keep thegcycall style consistent within the suite.Lines 122–123 use
cy.gcy(...)while the rest of this file (including lines 118 and 120 of the same test) uses the baregcy(...)helper imported from../../common/shared. Functionally equivalent, but worth aligning for consistency.Proposed tweak
- cy.gcy('content-delivery-form-name').find('input').clear().type('Renamed'); - cy.gcy('content-delivery-form-save').click(); + gcy('content-delivery-form-name').find('input').clear().type('Renamed'); + gcy('content-delivery-form-save').click();Otherwise the regression coverage is solid: seeding with
BRANCHINGon, flipping it off, reloading, and asserting a successful update accurately reproduces the bug this PR fixes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cypress/e2e/branching/contentDeliveryBranching.cy.ts` around lines 111 - 128, The test uses the helper gcy imported from ../../common/shared but two assertions use cy.gcy(...) which is inconsistent; replace cy.gcy('content-delivery-form-name') and cy.gcy('content-delivery-form-save') with the bare gcy(...) calls so the suite consistently uses the gcy helper (same style as openEditDialog and the other gcy usages) and run the test to ensure behavior is unchanged.
🤖 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/cypress/e2e/branching/contentDeliveryBranching.cy.ts`:
- Around line 111-128: The test uses the helper gcy imported from
../../common/shared but two assertions use cy.gcy(...) which is inconsistent;
replace cy.gcy('content-delivery-form-name') and
cy.gcy('content-delivery-form-save') with the bare gcy(...) calls so the suite
consistently uses the gcy helper (same style as openEditDialog and the other gcy
usages) and run the test to ensure behavior is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7be98009-87d3-46df-9de6-ee90cf8f1551
📒 Files selected for processing (2)
e2e/cypress/e2e/branching/contentDeliveryBranching.cy.tswebapp/src/views/projects/developer/contentDelivery/useCdActions.tsx
dkrizan
marked this pull request as draft
April 24, 2026 20:23
… disabled The backend feature guard on the CDN update endpoint used to fire whenever filterBranch was non-null in the request body. Because this endpoint uses replace semantics, the frontend always echoes the stored branchName back on save, so editing any CDN config that referenced a branch would 400 with FEATURE_NOT_ENABLED_FOR_PROJECT once branching got disabled on the project (feature flag or project.useBranching). Refine the guard so it fires only when the config is — or would end up — pointing at a non-default branch. Echoing the default branch back on an already-default config is now a no-op.
dkrizan
force-pushed
the
dkrizan/fix-cdn-edit-feature-not-enabled
branch
from
April 24, 2026 21:16
dbc71d7 to
f29748f
Compare
dkrizan
changed the title
dkrizan
marked this pull request as ready for review
April 27, 2026 08:33
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/cypress/e2e/branching/contentDeliveryBranching.cy.ts (1)
118-123: Style nit: mix ofgcy()andcy.gcy()within the same test.Lines 118 and 120 use
gcy(...)while lines 122–123 usecy.gcy(...). The rest of this file consistently uses the importedgcyhelper; aligning here keeps the test readable.Proposed tweak
- cy.gcy('content-delivery-form-name').find('input').clear().type('Renamed'); - cy.gcy('content-delivery-form-save').click(); + gcy('content-delivery-form-name').find('input').clear().type('Renamed'); + gcy('content-delivery-form-save').click();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cypress/e2e/branching/contentDeliveryBranching.cy.ts` around lines 118 - 123, The test mixes two helper usages—call sites using gcy(...) and cy.gcy(...); update the occurrences of cy.gcy (used for 'content-delivery-form-name' and 'content-delivery-form-save') to use the imported gcy helper instead so the test consistently uses gcy across openEditDialog('Main CDN'), the branch selector assertion, and the input/save steps; this keeps style consistent and matches the rest of the file.backend/data/src/main/kotlin/io/tolgee/service/contentDelivery/ContentDeliveryConfigService.kt (1)
147-151: Guard logic is correct; the asymmetry betweencreate()andupdate()is real and worth documenting.The conditional correctly fires when either the new branch or existing config branch is on a non-default branch, matching the test matrix. When both are default (e.g., echoing the default branch on a default-branch config), the guard does not fire—a no-op as intended.
However,
create()at line 49 usescheckIfUsed(Feature.BRANCHING, dto.filterBranch), which fires wheneverfilterBranchis any non-empty string—including when it equals the default branch name (e.g., "main"). A client that always sendsfilterBranchfor default-branch creations would fail unnecessarily, unlike the update path which is now lenient. Consider whether the UI omitsfilterBranchon create for default-branch scenarios, or aligncreate()to matchupdate()'s logic by checking if the resolved branch is actually non-default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/data/src/main/kotlin/io/tolgee/service/contentDelivery/ContentDeliveryConfigService.kt` around lines 147 - 151, The create() path should mirror update() by resolving the branch before enforcing the branching guard: instead of calling checkIfUsed(Feature.BRANCHING, dto.filterBranch) directly, call branchService.getActiveOrDefault(projectId, dto.filterBranch) to get the resolved branch and only invoke projectFeatureGuard.checkEnabled(Feature.BRANCHING) (or checkIfUsed) when that resolved branch is non-default (resolvedBranch?.isDefault == false); update the create() implementation to use the same isDefault-based check as update() to avoid rejecting creations where dto.filterBranch equals the project's default branch name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@backend/data/src/main/kotlin/io/tolgee/service/contentDelivery/ContentDeliveryConfigService.kt`:
- Around line 147-151: The create() path should mirror update() by resolving the
branch before enforcing the branching guard: instead of calling
checkIfUsed(Feature.BRANCHING, dto.filterBranch) directly, call
branchService.getActiveOrDefault(projectId, dto.filterBranch) to get the
resolved branch and only invoke
projectFeatureGuard.checkEnabled(Feature.BRANCHING) (or checkIfUsed) when that
resolved branch is non-default (resolvedBranch?.isDefault == false); update the
create() implementation to use the same isDefault-based check as update() to
avoid rejecting creations where dto.filterBranch equals the project's default
branch name.
In `@e2e/cypress/e2e/branching/contentDeliveryBranching.cy.ts`:
- Around line 118-123: The test mixes two helper usages—call sites using
gcy(...) and cy.gcy(...); update the occurrences of cy.gcy (used for
'content-delivery-form-name' and 'content-delivery-form-save') to use the
imported gcy helper instead so the test consistently uses gcy across
openEditDialog('Main CDN'), the branch selector assertion, and the input/save
steps; this keeps style consistent and matches the rest of the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 503250bb-bde9-4290-b2b6-06d357235ac9
📒 Files selected for processing (3)
backend/data/src/main/kotlin/io/tolgee/service/contentDelivery/ContentDeliveryConfigService.kte2e/cypress/e2e/branching/contentDeliveryBranching.cy.tsee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/ContentDeliveryConfigBranchingTest.kt
bdshadow
approved these changes
Apr 27, 2026
TolgeeMachine
added a commit
that referenced
this pull request
Apr 27, 2026
## [3.181.3](v3.181.2...v3.181.3) (2026-04-27) ### Bug Fixes * allow editing content delivery config on projects with branching disabled ([#3622](#3622)) ([1da8d67](1da8d67))
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
filterBranchwas non-null in the request body. Because this endpoint uses PUT replace semantics (every field in the DTO is unconditionally reassigned), the frontend always echoes the storedbranchNameback on save. As a result, editing any CDN config that referenced a branch would 400 withFEATURE_NOT_ENABLED_FOR_PROJECTonce branching got disabled on the project (either the org'sBRANCHINGfeature flag orproject.useBranching).ContentDeliveryConfigService.update()so it only fires when the config is — or would end up — pointing at a non-default branch. Echoing the default branch back on an already-default config is now a no-op.Guard semantics
dto.filterBranchmain)nullmain)"main"main)"feature"feature)"feature"feature)"main"feature)nullTest plan
ContentDeliveryConfigBranchingTeststill pass, includingupdate CDN config with branch fails when branching not enabledandcreate CDN config with branch fails when branching not enabled.update default-branch CDN succeeds when branching not enabled and default is echoedupdate default-branch CDN succeeds when branching not enabled and filterBranch is nullupdate non-default-branch CDN fails when branching not enabled even if branch is unchangededits branched content delivery when branching gets disabledincontentDeliveryBranching.cy.ts— reproduces the original 400 via the UI.Summary by CodeRabbit
Release Notes
Tests
Bug Fixes