Fixes #38191: Skip reassignment for multiCV in remove #11349
Conversation
There was a problem hiding this comment.
Thanks for taking a look @pavanshekar! One concern below about the approach.
| dispatch(getHosts({ search: cvQuery })); | ||
|
|
||
| let akQuery = `content_view_id=${cvId}`; | ||
| const selectedEnvStringsForAK = selectedEnv.map(env => `lifecycle_environment_id=${env.id}`).join(' OR '); | ||
| if (selectedEnvStringsForAK.length) akQuery += ` AND (${selectedEnvStringsForAK})`; | ||
| dispatch(getActivationKeys({ search: akQuery })); |
There was a problem hiding this comment.
I feel uneasy about introducing two new API requests here when I don't think it should be necessary. Using scoped search may work okay, but I don't think this additional overhead is worth it.
The backend already knows every content view environment associated with the hosts and AKs, and we probably already have that info somewhere in the API response. Can we find a way to use the existing info instead?
There was a problem hiding this comment.
Implemented a solution by adding backend properties to avoid additional API requests while still skipping reassignment when all hosts/AKs are multi-environment.
pavanshekar
force-pushed
the
issue-38191
branch
from
5a67667 to
fec4e06
Compare
|
I've implemented the fix so that the hosts and activation keys counts now appear correctly in the Review step as well. |
| @@ -73,9 +73,21 @@ child :sorted_organization_readable_environments => :environments do | |||
| ::Host.authorized('view_hosts').in_content_view_environment(:content_view => version.content_view, :lifecycle_environment => env).count | |||
| end | |||
|
|
|||
| node :multi_env_host_count do |env| | |||
| hosts = ::Host.authorized('view_hosts') | |||
| .in_content_view_environment(content_view: version.content_view, lifecycle_environment: env) | |||
There was a problem hiding this comment.
Should use the non-deprecated method.
| .in_content_view_environment(content_view: version.content_view, lifecycle_environment: env) | |
| .in_content_view_environments(content_views: [version.content_view], lifecycle_environments: [env]) |
There was a problem hiding this comment.
I have pushed the suggested change
|
When there's only multi-environment hosts, I still want to have access to the "Show hosts" / "Show activation keys" expansion. Otherwise I can't see what hosts/AKs are affected. Can we duplicate "Show hosts" / "Show activation keys" on the Review step, maybe? |
|
I've added the "Show hosts" / "Show activation keys" sections to the Review step. |
What are the changes introduced in this pull request?
Updated the remove wizard to skip the reassignment selection step in remove version for multiCV. The change streamlines the process by automatically bypassing the unnecessary reassignment of cv/lce, ensuring that the user isn’t prompted to choose a replacement that will ultimately be ignored.
Considerations taken when implementing this change?
Ensured that the skipping logic remains intact while improving the user experience by eliminating steps which would be ignored. Special attention was given to maintaining backward compatibility for non-multiCV flows and updating tests to confirm that the wizard behaves as expected.
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