-
Notifications
You must be signed in to change notification settings - Fork 297
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
Fixes #38116 - Handle version removal for multi-CV hosts #11282
Conversation
3f31f26
to
30729ca
Compare
a5ddfe2
to
fef992a
Compare
fcced92
to
fb6ccbf
Compare
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.
Looks pretty good! some initial comments
webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVReassignHostsForm.js
Outdated
Show resolved
Hide resolved
webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js
Outdated
Show resolved
Hide resolved
fb6ccbf
to
bf66961
Compare
1a5884f
to
7b52bbd
Compare
webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js
Outdated
Show resolved
Hide resolved
webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js
Outdated
Show resolved
Hide resolved
webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js
Outdated
Show resolved
Hide resolved
webpack/scenes/ContentViews/Details/Versions/Delete/affectedHosts.js
Outdated
Show resolved
Hide resolved
c9c6b5a
to
a019aca
Compare
webpack/scenes/ContentViews/Details/Versions/Delete/__tests__/cvVersionRemove.test.js
Show resolved
Hide resolved
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.
Tested and seems to work as expected, except for one comment below.
webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js
Outdated
Show resolved
Hide resolved
webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js
Outdated
Show resolved
Hide resolved
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.
Another edge case I found:
The correct content view environment label for "Library/Default Organization View" should just be "Library" (without the "Default Organization View").
I'm thinking it may be best to alter the rabl somehow to provide the correct content view environment labels, and then we wouldn't have to assemble them on the frontend..
webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js
Outdated
Show resolved
Hide resolved
a019aca
to
2c5fdfd
Compare
webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js
Outdated
Show resolved
Hide resolved
webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js
Outdated
Show resolved
Hide resolved
webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js
Outdated
Show resolved
Hide resolved
2c5fdfd
to
b2b0a88
Compare
webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js
Show resolved
Hide resolved
b2b0a88
to
74bb1c8
Compare
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.
Works well and I feel much better about the logic now. Nice use of flatMap
to find the correct content view environment :)
One more translation nit below, but feel free to merge :) ACK 👍
webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js
Outdated
Show resolved
Hide resolved
74bb1c8
to
7db32f6
Compare
What are the changes introduced in this pull request?
Added checks for multi-CV host associations and helper methods to handle version removal from the environment, ensuring that relevant content facets and environment links are cleaned up. This ensures that only the targeted version is removed without affecting unrelated associations or environments.
Considerations taken when implementing this change?
Ensured selective removal of associations for multi-CV hosts to avoid reassigning hosts linked to single CVEs or impacting unrelated environments, while preserving existing associations and removing only the targeted version from the hosts' content facets. Existing methods were reused for consistency and to minimize duplication, and checks were added to handle edge cases, such as multiple hosts linked to the same environment, to prevent unintended side effects.
What are the testing steps for this pull request?
Create a multi-CV activation key
Example command: hammer activation-key update --organization="Default Organization" --id=1 --content-view-environments="ABC/cv1,XYZ/cv1"
Assign and register multiple hosts using the multi-CV activation key
Remove the version from the environment