Fixes #38116 - Handle version removal for multi-CV hosts #11282

pavanshekar · 2025-01-20T19:02:26Z

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

Promote a CV Version to multiple environments
Using Hammer CLI, update the activation key to use mutli CVE
Example command: hammer activation-key update --organization="Default Organization" --id=1 --content-view-environments="ABC/cv1,XYZ/cv1"
Confirm that the activation key uses multi CVEs

Assign and register multiple hosts using the multi-CV activation key

Remove the version from the environment

Go to the CV associated with the activation key -> Versions
Click on the 3 dots of the respective CV Version and select the option "Remove from environments"
Follow the steps to remove the CV Version from the environment
A task will be created which should successfully remove the version from the environment when the CVE is in use by a multi-CV host

jeremylenz

Looks pretty good! some initial comments

jeremylenz · 2025-01-21T21:33:12Z

webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVReassignHostsForm.js

+  const multiCVWarning = hostResponse?.results?.some?.(host =>
+    host.content_facet_attributes?.multi_content_view_environment);
+
+  const multiCVRemovalInfo = __('This environment is used in one or more hosts. The environment will simply be removed from the multi-environment hosts. The content view and lifecycle environment you select here will only apply to single-environment host. See hammer activation-key --help for more details.');


Suggested change

const multiCVRemovalInfo = __('This environment is used in one or more hosts. The environment will simply be removed from the multi-environment hosts. The content view and lifecycle environment you select here will only apply to single-environment host. See hammer activation-key --help for more details.');

const multiCVRemovalInfo = __('This content view version is used in one or more multi-environment hosts. The version will simply be removed from the multi-environment hosts. The content view and lifecycle environment you select here will only apply to single-environment hosts. See hammer activation-key --help for more details.');

jeremylenz · 2025-01-21T21:35:07Z

webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js

+              <FlexItem><ExclamationTriangleIcon /></FlexItem>
+              <FlexItem>
+                <p>
+                  {__(`Content view environment will be removed from ${pluralize(multiCVHostsCount, 'multi-environment host')}.`)}


Can we also include the content view environment's label? "Content view environment envX/cvX will be removed.."

I have included and pushed the updated code

jeremylenz · 2025-01-21T21:35:56Z

app/lib/actions/katello/content_view_environment/reassign_objects.rb

-              plan_action(Host::Reassign, host, options[:system_content_view_id], options[:system_environment_id])
+              content_facet_attributes = host.content_facet
+              if content_facet_attributes.multi_content_view_environment?
+                content_facet_attributes.content_view_environments -= [content_view_environment]


note to self for testing: Need to make sure this calls #content_view_environments= so that the priority is properly updated.

jeremylenz · 2025-01-22T17:06:21Z

webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js

+                <p>
+                  {__(`Content view environment 
+                    ${selectedEnv
+                    ?.map(({ name }) => `${name}/${multiCVHosts[0]?.content_facet_attributes?.content_view?.name}`)


@sjha4 Is there a less awkward way this CVE label could be assembled?

jeremylenz · 2025-01-22T17:10:09Z

webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js

+                  {__(`Content view environment 
+                    ${selectedEnv
+                    ?.map(({ name }) => `${name}/${multiCVHosts[0]?.content_facet_attributes?.content_view?.name}`)
+                    .join(', ')} 
+                    will be removed from ${pluralize(multiCVHostsCount, 'multi-environment host')}.`)}


When using ${...}, the __() won't properly translate the string. We should use something similar to this:

katello/webpack/scenes/Hosts/ChangeContentSource/components/ContentSourceForm.js

Lines 248 to 264 in 527df0a

<FormattedMessage

defaultMessage={__('After configuring Foreman, configuration must also be updated on {hosts}. Choose one of the following options to update {hosts}:')}

values={{

hosts: (

<FormattedMessage

defaultMessage="{count, plural, one {{singular}} other {# {plural}}}"

values={{

count: hostCount,

singular: __('the host'),

plural: __('hosts'),

}}

id="ccs-options-i18n"

/>

),

}}

id="ccs-options-description-i18n"

/>

jeremylenz · 2025-01-22T17:10:44Z

webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js

+          {singleCVActivationKeysCount > 0 && (
+            <Flex>
+              <FlexItem><ExclamationTriangleIcon /></FlexItem>
+              <FlexItem><p>{__(`${pluralize(singleCVActivationKeysCount, 'activation key')} will be moved to content view ${selectedCVNameForAK} in `)}</p></FlexItem>


Same here; please use FormattedMessage

jeremylenz · 2025-01-22T17:11:14Z

webpack/scenes/ContentViews/Details/Versions/Delete/affectedHosts.js

        }) => (
          <Tr ouiaId={id} key={id}>
            <Td>
              <a rel="noreferrer" target="_blank" href={urlBuilder(`new/hosts/${id}`, '')}>{name}</a>
            </Td>
            <Td><EnvironmentLabels environments={environment} /></Td>
+            <Td>{ multiContentViewEnvironment ? 'Yes' : 'No' }</Td>


Suggested change

<Td>{ multiContentViewEnvironment ? 'Yes' : 'No' }</Td>

<Td>{ multiContentViewEnvironment ? __('Yes') : __('No') }</Td>

I have made the suggested changes and pushed the code

pavanshekar · 2025-01-23T14:31:24Z

webpack/scenes/ContentViews/Details/Versions/Delete/__tests__/cvVersionRemove.test.js

@@ -192,7 +193,7 @@ test('Can open Remove wizard and remove version from environment with hosts', as
  fireEvent.click(getByText('Next'));
  await patientlyWaitFor(() => {
    expect(getByText('Review details')).toBeInTheDocument();
-    expect(getByText('1 host will be moved to content view cv2 in')).toBeInTheDocument();
+    expect(getByTestId('single-cv-hosts-remove')).toBeInTheDocument();


Updated test case to use getTestById for reliable assertion of FormattedMessage, as its text spans multiple elements.

jeremylenz

Tested and seems to work as expected, except for one comment below.

jeremylenz · 2025-01-24T17:03:34Z

webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js

+              <FlexItem data-testid="single-cv-hosts-remove">
+                <FormattedMessage
+                  id="single-cv-hosts-remove"
+                  defaultMessage="{count, plural, one {{singular}} other {# {plural}}} will be moved to content view {cvName} in {envName}."


This is omitting the number:

jeremylenz · 2025-01-24T19:00:16Z

webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js

+              <FlexItem data-testid="single-cv-activation-keys-remove">
+                <FormattedMessage
+                  id="single-cv-activation-keys-remove"
+                  defaultMessage="{count, plural, one {{singular}} other {# {plural}}} will be moved to content view {cvName} in {envName}."


Suggested change

defaultMessage="{count, plural, one {{singular}} other {# {plural}}} will be moved to content view {cvName} in {envName}."

defaultMessage="{count, plural, one {# {singular}} other {# {plural}}} will be moved to content view {cvName} in {envName}."

jeremylenz

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..

jeremylenz · 2025-01-24T19:22:39Z

webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js

+                  defaultMessage="Content view {envSingularOrPlural} {envCV} will be removed from {count, plural, one {# {keySingular}} other {# {keyPlural}}}."
+                  values={{
+                    envSingularOrPlural: selectedEnv?.length === 1 ? __('environment') : __('environments'),


These plurals will not be properly translated, since plural rules can be different in different languages.

I have resolved the edge case and the plurals translation issue and pushed the updated code. The changes have been implemented in the frontend because other pages are using rabl for rendering. Please let me know if this approach is okay.

jeremylenz · 2025-01-28T15:12:50Z

webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js

+                      <FormattedMessage
+                        id="environment.plural"
+                        defaultMessage="{count, plural, one {environment} other {environments}}"
+                        values={{ count: selectedEnv?.length }}


The values object must contain values for every placeholder in the defaultMessage string. Looks like this one is missing environment and environments

jeremylenz · 2025-01-28T15:14:43Z

webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js

+              <FlexItem>
+                <FormattedMessage
+                  id="multi-cv-hosts-remove"
+                  defaultMessage="Content view {envSingularOrPlural} {envCV} will be removed from {count, plural, one {# {hostSingular}} other {# {hostPlural}}}."


plural isn't defined. I think maybe it needs this?

Suggested change

defaultMessage="Content view {envSingularOrPlural} {envCV} will be removed from {count, plural, one {# {hostSingular}} other {# {hostPlural}}}."

defaultMessage="Content view {envSingularOrPlural} {envCV} will be removed from {count, hostPlural, one {# {hostSingular}} other {# {hostPlural}}}."

I have updated the formatted message for correct translations. The plural keyword is used to handle singular and plural forms based on the hostCount value.

jeremylenz · 2025-01-28T15:15:22Z

webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js

-                </p>
+                <FormattedMessage
+                  id="multi-cv-activation-keys-remove"
+                  defaultMessage="Content view {envSingularOrPlural} {envCV} will be removed from {count, plural, one {# {keySingular}} other {# {keyPlural}}}."


Same, missing definition for plural

pavanshekar · 2025-01-29T22:57:39Z

webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js

+                    ),
+                    envCV: selectedCVE
+                      ?.map(cve => cve.label)
+                      .join(', '),


I have updated and optimized the logic, making it more concise.

pr-processor bot added the Not yet reviewed label Jan 20, 2025

pavanshekar marked this pull request as ready for review January 20, 2025 19:47

pr-processor bot added Waiting on contributor and removed Not yet reviewed labels Jan 20, 2025

pavanshekar force-pushed the issue-38116 branch from 3f31f26 to 30729ca Compare January 20, 2025 19:57

pr-processor bot added Needs re-review Waiting on contributor and removed Waiting on contributor labels Jan 20, 2025

pavanshekar force-pushed the issue-38116 branch from a5ddfe2 to fef992a Compare January 20, 2025 20:04

pr-processor bot added Waiting on contributor and removed Waiting on contributor labels Jan 20, 2025

pavanshekar force-pushed the issue-38116 branch from fcced92 to fb6ccbf Compare January 20, 2025 20:14

pr-processor bot removed the Waiting on contributor label Jan 20, 2025

jeremylenz reviewed Jan 21, 2025

View reviewed changes

pr-processor bot removed the Needs re-review label Jan 21, 2025

pavanshekar force-pushed the issue-38116 branch from fb6ccbf to bf66961 Compare January 22, 2025 16:37

pr-processor bot added the Waiting on contributor label Jan 22, 2025

pavanshekar force-pushed the issue-38116 branch from 1a5884f to 7b52bbd Compare January 22, 2025 17:04

pr-processor bot added Needs re-review and removed Waiting on contributor labels Jan 22, 2025

jeremylenz reviewed Jan 22, 2025

View reviewed changes

pr-processor bot removed the Needs re-review label Jan 22, 2025

pavanshekar force-pushed the issue-38116 branch 3 times, most recently from c9c6b5a to a019aca Compare January 23, 2025 14:29

pavanshekar commented Jan 23, 2025

View reviewed changes

jeremylenz reviewed Jan 24, 2025

View reviewed changes

pavanshekar force-pushed the issue-38116 branch from a019aca to 2c5fdfd Compare January 28, 2025 14:36

jeremylenz reviewed Jan 28, 2025

View reviewed changes

pavanshekar force-pushed the issue-38116 branch from 2c5fdfd to b2b0a88 Compare January 29, 2025 22:47

pavanshekar commented Jan 29, 2025

View reviewed changes

Fixes #38116 - Handle version removal for multi-CV hosts

74bb1c8

pavanshekar force-pushed the issue-38116 branch from b2b0a88 to 74bb1c8 Compare January 30, 2025 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #38116 - Handle version removal for multi-CV hosts #11282

Fixes #38116 - Handle version removal for multi-CV hosts #11282

pavanshekar commented Jan 20, 2025

jeremylenz left a comment

jeremylenz Jan 21, 2025

pavanshekar Jan 22, 2025

jeremylenz Jan 21, 2025

pavanshekar Jan 22, 2025

jeremylenz Jan 21, 2025

jeremylenz Jan 22, 2025

jeremylenz Jan 22, 2025

pavanshekar Jan 22, 2025

jeremylenz Jan 22, 2025

pavanshekar Jan 22, 2025

jeremylenz Jan 22, 2025

pavanshekar Jan 22, 2025

pavanshekar Jan 23, 2025

jeremylenz left a comment

jeremylenz Jan 24, 2025

jeremylenz Jan 24, 2025

pavanshekar Jan 27, 2025

jeremylenz left a comment •

edited

Loading

jeremylenz Jan 24, 2025

pavanshekar Jan 28, 2025

jeremylenz Jan 28, 2025

jeremylenz Jan 28, 2025

pavanshekar Jan 29, 2025

jeremylenz Jan 28, 2025

pavanshekar Jan 29, 2025

	const multiCVRemovalInfo = __('This environment is used in one or more hosts. The environment will simply be removed from the multi-environment hosts. The content view and lifecycle environment you select here will only apply to single-environment host. See hammer activation-key --help for more details.');
	const multiCVRemovalInfo = __('This content view version is used in one or more multi-environment hosts. The version will simply be removed from the multi-environment hosts. The content view and lifecycle environment you select here will only apply to single-environment hosts. See hammer activation-key --help for more details.');

	<FormattedMessage
	defaultMessage={__('After configuring Foreman, configuration must also be updated on {hosts}. Choose one of the following options to update {hosts}:')}
	values={{
	hosts: (
	<FormattedMessage
	defaultMessage="{count, plural, one {{singular}} other {# {plural}}}"
	values={{
	count: hostCount,
	singular: __('the host'),
	plural: __('hosts'),
	}}
	id="ccs-options-i18n"
	/>
	),
	}}
	id="ccs-options-description-i18n"
	/>

	<Td>{ multiContentViewEnvironment ? 'Yes' : 'No' }</Td>
	<Td>{ multiContentViewEnvironment ? __('Yes') : __('No') }</Td>

	defaultMessage="{count, plural, one {{singular}} other {# {plural}}} will be moved to content view {cvName} in {envName}."
	defaultMessage="{count, plural, one {# {singular}} other {# {plural}}} will be moved to content view {cvName} in {envName}."

	defaultMessage="Content view {envSingularOrPlural} {envCV} will be removed from {count, plural, one {# {hostSingular}} other {# {hostPlural}}}."
	defaultMessage="Content view {envSingularOrPlural} {envCV} will be removed from {count, hostPlural, one {# {hostSingular}} other {# {hostPlural}}}."

Fixes #38116 - Handle version removal for multi-CV hosts #11282

Are you sure you want to change the base?

Fixes #38116 - Handle version removal for multi-CV hosts #11282

Conversation

pavanshekar commented Jan 20, 2025

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

jeremylenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremylenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremylenz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremylenz left a comment •

edited

Loading