The current approach uses adds an extra parameter to the components and
passes it through from the parent components. It's never a lot of
levels, so it feels alright, but it's feels like a bit of a code smell.
I wonder if it would make sense to use a context for each change
request? 🤔
Supersedes: https://github.com/Unleash/unleash/pull/6181
This PR updates the way we show deleted strategies in the CR UI. Instead
of showing just the strategy name and a diff on hover, we show the same
strategy config as we do for new and updated strategies.
This makes it easier to see what you have deleted.
In doing so, it also fixes two issues:
1. inconsistent border radius for segment changes listed. Due to an
override in `frontend/src/themes/theme.ts`, these would get a border
radius of `theme.shape.borderRadiusLarge` instead of
`theme.shape.borderRadiusMedium`. It does this by adding a class and
making the selector more specific.
2. The background was unset for the strategy rollout box and constraint
item boxes.
It looks like this:
<img width="728" alt="image"
src="https://github.com/Unleash/unleash/assets/17786332/7cba28ac-0454-444d-8cfa-f46543ccf2dc">
<img width="728" alt="image"
src="https://github.com/Unleash/unleash/assets/17786332/832be653-3def-4afc-b72f-36fcd76ad83d">
Or with more kinds of strategies:
<img width="454" alt="image"
src="https://github.com/Unleash/unleash/assets/17786332/f18e5482-7d2e-4cbd-8177-9de6dfb10307">
Note: I'm happy to isolate the color changes to a separate PR if that's
preferable.
This PR adds showing of env variant conflicts in change requests.
This is a simple solution that only compares the total state of
variants. We *could* potentially do a modified version where we show
each and every variant as its own property. Because variants have to be
unique by name and because their names can't be changed after their
creation, we could create a map of variant name to their data.
<img width="1105" alt="image"
src="https://github.com/Unleash/unleash/assets/17786332/0c67f958-6c4e-453a-9791-0e132fb1f23e">
Use React's context to track how many CRs are moved into their next
state with conflicts present.
This PR wraps environment change requests and change request overviews
in a change request plausible context that contains a
`willOverwriteStrategyChanges` property. This property is updated by the
diff calculation if there are any conflicts and then read by the
`changeState` function in the `useChangeRequestApi` hook.
As long as at least one of the strategies in the CR contain conflicts,
it will be marked as overwriting changes.
We had to make some updates to let the compiler know about the types and
fix an issue with nested objects not being compared as objects (instead
as strings), but this saves us a few lines and is hopefully more
readable.
This PR fixes a bug in the displayed value of the conflict list so that
it shows the value it would update to instead of the snapshot value.
In doing so, it updates the logic of the algorithm to:
1. if the snapshot value and the current value are the same, it's not a
conflict (it's an intended change)
2. If the snapshot value differs from the current value, it is a
conflict if and only if the value in the change differs from the current
value. Otherwise, it's not a conflict.
The new test cases are:
- it shows a diff for a property if the snapshot and live version differ
for that property and the changed value is different from the live
version
- it does not show a diff for a property if the live version and the
change have the same value, even if the snapshot differs from the live
version
- it does not show a diff for a property if the snapshot and the live
version are the same
A strategy title can be either an empty string or undefined on the
type we use in the frontend. In the snapshot it can be an empty
string, null (presumably), and undefined.
This change updates the diffing logic to handle the various title diff
cases correctly. It also updates the type used for the snapshot to
reflect this.
This change adds an algorithm with tests for detecting what changes
would be overwritten by applying a CR.
Test cases:
- It compares strategies regardless of order of keys in the objects.
This ensures that two strategies with the same content but different
order of keys are compared correctly.
- It treats `undefined` or missing segments in old config as equal to
`[]` in change
- It treats `undefined` or missing strategy variants in old config and
change as equal to `[]`
- It lists changes in a sorted list with the correct values
- It ignores object order on nested objects. Similar to the first
point, this does order-insensitive comparison for nested objects (such
as params and constraints).
This PR adds uuids as ids using a symbol in order to make sure we only
use this to keep internal order in the viritual DOM. This makes us able
to have predictable mutable lists on the frontend, and makes it easy to
not pass this property along to the backend.
This changes the two interfaces IChangeRequest and
IChangeRequestSchedule to be union types instead of interfaces. It also
extracts the constituents of those union types into proper types
themselves (so that they can be used in function type signatures etc).
It also updates the type names.
This turned out to be more work than I had imagined, but I think the end
result pays off, giving us more type safety and control.
I wanted to use just `ChangeRequest` for the IChangeRequest type, but
that caused issues due to naming collisions with the `ChangeRequest`
component that we have, causing tests to fail. I've named it
`ChangeRequestType` as a potential solution, but suggestions are
welcome.
The relevant changes are in
`frontend/src/component/changeRequest/changeRequest.types.ts`.
Everything else is updated references and some necessary refactoring to
respect the new types.
This PR makes a change to how variants work in the new setup. Variants
will now:
* Be removed if you change tab or unmount the component and it has no
name
* Moved StrategyVariants into a separate component to isolate this
change
* Add error handling around onSubmit and only trigger feedback if it's
successful
This PR refactores the StrategyVariants component to be passed in from
the outside to the new form component. This allows us to pass in the
StrategyVariants with an "editable" property in the create form which we
use to determine the editable state of the name input field. If the
editable field is not passed in we keep the old behavior.
Notable changes:
* StrategyVariants is now passed in from the outside, allowing us to
define different props at call time
* Added tests for the new behavior, and for keeping the old behavior
(such as in edit strategy)
* Added tracking
This change addresses two things that were done in
https://github.com/Unleash/unleash/pull/4004 and that I believe to be
bugs.
1. It shows the previous strategy name also if there was no previous
title. So if there was no previous title, it'll show the strategy name
with a strikethrough and then the new title (see the discussion
section).
2. It changes a `span` component to a [`del`
component](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/del).
I believe the
span was erroneously changed from a `s` component (strikethrough
component) in the linked PR (based on a comment on the PR). This
caused the strikethrough to not be there anymore. However, the `del`
component is semantically more correct and reintroduces the
strikethrough, so it is a better change.
3. It uses [`ins`
elements](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/ins)
for names that have changed.
Finally, it removes a redundant pair of curly braces.
How it looks now:
## Discussion
Regarding point 1: It might be that we don't want to show a
strikethrough through the name of the strategy if there was no previous
title. In that case, the changes related to the first point should be
removed. If we do that, it looks like this:
It makes it harder (impossible, actually) to see when a custom title was
added, but that might be what we want.
But maybe the solution is to also use `ins` elements for new data. That
way the difference is visible (and semantically correct):
- make the blocks that hold the constraints full width
- remove the margin-top and margin-bottom from the parent div + increase
the gap: 20px on this parent div
- remove the margin-bottom from the constraints containers
- add the strategies in a div and add a margin-bottom: 8px
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
<!-- Thanks for creating a PR! To make it easier for reviewers and
everyone else to understand what your changes relate to, please add some
relevant content to the headings below. Feel free to ignore or delete
sections that you don't think are relevant. Thank you! ❤️ -->
Change request UI for reordering strategies with variants
## About the changes
<!-- Describe the changes introduced. What are they and why are they
being introduced? Feel free to also add screenshots or steps to view the
changes if they're visual. -->
<!-- Does it close an issue? Multiple? -->
Closes #
<!-- (For internal contributors): Does it relate to an issue on public
roadmap? -->
<!--
Relates to [roadmap](https://github.com/orgs/Unleash/projects/10) item:
#
-->
### Important files
<!-- PRs can contain a lot of changes, but not all changes are equally
important. Where should a reviewer start looking to get an overview of
the changes? Are any files particularly important? -->
## Discussion points
<!-- Anything about the PR you'd like to discuss before it gets merged?
Got any questions or doubts? -->
---------
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
