-
Notifications
You must be signed in to change notification settings - Fork 466
feat: edit versioned change request #6368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: chore/create-flag-migration
Are you sure you want to change the base?
feat: edit versioned change request #6368
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Docker builds report
|
|
My understanding of the requirements, written in test scenarios. @kyle-ssg is this your understanding of the behaviour? Anything else that we should be testing? Non-scheduled CRs
Scheduled CRs
|
@matthewelwell The exception to that at the moment is anyone can edit a change request if they have the ability to create one. I can see a use case for this tbh, if someone is away we should let someone with appropriate permissions to do this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| ...v, | ||
| originalPriority: i, | ||
| priority: i, | ||
| })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Segment override priorities from changeset are discarded
When editing a versioned change request, segment override priorities from the changeset are set at lines 119 and 166 (priority: changedFeatureState.feature_segment?.priority || 0), but then immediately overwritten with array indices at lines 185-189. Since the save function at create-feature/index.js:604 uses the top-level priority property for feature_segment.priority, any priority reordering specified in the original change request will be lost when the edit is saved. The segments also remain in the current live order rather than being sorted by the changeset's intended priorities.
Additional Locations (1)
| changeRequest.feature_states[0].feature_state_value, | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Legacy change requests ignored in versioned environments
The condition logic creates a gap for legacy change requests. The first branch requires isVersioned && changeRequest.change_sets, and the second requires !isVersioned && changeRequest.feature_states?.[0]. If an environment has versioning enabled but a change request was created before versioning (using feature_states instead of change_sets), neither branch executes. The changedEnvironmentFlag would retain the current live values rather than the change request's intended values, causing the edit modal to display incorrect data.
My concern with that is that we don't have any indication when a change request has been edited. If we're not going to add that (which is probably the correct approach for now), I would rather restrict it to the author, or environment admins for now. At the moment, it's only the author that can delete a change request for example - I think we should add environment admins to this list, but I don't think anyone should be able to edit or delete a CR. |
|
I've aligned the BE with my expectations in a PR here. |
Ok yep, I've updated the FE to do this too. Testing scenarios look valid |
| ? changeRequest.feature_states?.[0] | ||
| ?.multivariate_feature_state_values | ||
| : undefined | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Versioned CR edit loses multivariate allocation values
When editing a versioned change request, multivariate_options is explicitly passed as undefined. During save, this causes a fallback to projectFlag.multivariate_options (project defaults) instead of preserving the changeset's multivariate allocation values. If the original change request modified multivariate percentage allocations, those modifications would be lost and replaced with default values when the edited change request is saved.
Additional Locations (1)
| }) | ||
| const mergedStates = mergeChangeSets( | ||
| changeRequest.change_sets, | ||
| currentFeatureStatesResponse.data.results, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing null check causes crash when API call fails
The editChangeRequest function calls getFeatureStates and immediately accesses currentFeatureStatesResponse.data.results without checking if data exists. When an RTK Query endpoint fails (network error, API error, etc.), the response object has data: undefined and an error property instead. This will cause a TypeError crash when attempting to access .results on undefined. The codebase uses optional chaining (data?.results) elsewhere for similar patterns, suggesting this check was inadvertently omitted here.
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature!Changes
Allows editing a versioned change request
How did you test
Edit.Change.Requests.mov
this code?
Edit a change request in a versioned environment