Skip to content
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 #37861 - Update web UI for multi-CV activation key display #11161

Merged
merged 7 commits into from
Oct 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion app/controllers/katello/api/v2/activation_keys_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -300,11 +300,23 @@ def find_cve_for_single
@content_view_environments = [cve]
end

def params_likely_not_from_angularjs?
# AngularJS sends back the activation key's existing API response values.
# A side effect of this is that when it sends params[:content_view_environments] or params[:content_view_environment_ids],
# it incorrectly sends the nested objects from the rabl response, instead of the required single comma-separated string of Candlepin names.
# This would cause fetch_content_view_environments to fail.
# Therefore, we need a way to (a) detect if the request is from AngularJS, and (b) avoid trying to handle the nested objects as if they were strings.
# So we look at params[:multi_content_view_environment]. This is a computed value, not meant to be submitted as part of an update request.
# If it's true or false, it's likely AngularJS.
# And if the key is omitted, it's likely from Hammer or API, so it's safe to proceed.
!params.key?(:multi_content_view_environment)
end

def find_content_view_environments
@content_view_environments = []
if params[:environment_id] || params[:environment]
find_cve_for_single
elsif params[:content_view_environments] || params[:content_view_environment_ids]
elsif params_likely_not_from_angularjs? && (params[:content_view_environments] || params[:content_view_environment_ids])
@content_view_environments = ::Katello::ContentViewEnvironment.fetch_content_view_environments(
labels: params[:content_view_environments],
ids: params[:content_view_environment_ids],
Expand Down
31 changes: 24 additions & 7 deletions app/models/katello/content_view_environment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,20 +69,37 @@ def priority(content_object)
end

def self.fetch_content_view_environments(labels: [], ids: [], organization:)
# Must do maps here to ensure CVEs remain in the same order.
# Must ensure CVEs remain in the same order.
# Using ActiveRecord .where will return them in a different order.
id_errors = []
label_errors = []
cves = []
if ids.present?
cves = ids.map do |id|
::Katello::ContentViewEnvironment.find_by(id: id)
ids.each do |id|
cve = ::Katello::ContentViewEnvironment.find_by(id: id)
if cve.blank?
id_errors << id
else
cves << cve
end
end
cves.compact
elsif labels.present?
environment_names = labels.map(&:strip)
environment_names.map! do |name|
with_candlepin_name(name, organization: organization)
environment_names.each do |name|
cve = with_candlepin_name(name, organization: organization)
if cve.blank?
label_errors << name
else
cves << cve
end
end
environment_names.compact
end
if labels.present? && labels.length != cves.length
fail HttpErrors::UnprocessableEntity, _("No content view environments found with names: %{names}") % {names: label_errors.join(', ')} if label_errors.present?
elsif ids.present? && ids.length != cves.length
fail HttpErrors::UnprocessableEntity, _("No content view environments found with ids: %{ids}") % {ids: id_errors.join(', ')} if id_errors.present?
end
cves
end

private
Expand Down
24 changes: 24 additions & 0 deletions app/views/katello/api/v2/activation_keys/base.json.rabl
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,30 @@ node :multi_content_view_environment do |ak|
ak.multi_content_view_environment?
end

child :content_view_environments => :content_view_environments do
node :content_view do |cve|
{
id: cve.content_view&.id,
name: cve.content_view&.name,
composite: cve.content_view&.composite,
content_view_version: cve.content_view_version&.version,
content_view_version_id: cve.content_view_version&.id,
content_view_version_latest: cve.content_view_version&.latest?,
content_view_default: cve.content_view&.default?
}
end
node :lifecycle_environment do |cve|
{
id: cve.lifecycle_environment&.id,
name: cve.lifecycle_environment&.name,
lifecycle_environment_library: cve.lifecycle_environment&.library?
}
end
node :candlepin_name do |cve|
cve.candlepin_name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better name we can provide in the JSON for this? cve_label maybe? candlepin_name seems a little off if we are using the name for things we are doing in katello and hammer..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also called just label..

I've always been fond of "candlepin_name" 😢 but I guess I can change it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Time to make candlepin_name katello_name since we are using it everywhere including asking users to use it in hammer..
I'll leave it to your better judgement..I know we have a story to make these candlepin_names more easily available to users..

end
end

# single cv/lce for backward compatibility
node :content_view_id do |ak|
ak.single_content_view&.id
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

<div bst-alert="warning" ng-if="activationKey.multi_content_view_environment">
<span translate>
This activation key has multiple content view environments, which are not yet displayed in the web UI. Changing the content view or lifecycle environment here will overwrite the activation key's multiple environments. To avoid this, update the activation key via Hammer.
This activation key has multiple content view environments, which cannot yet be edited in the web UI. To change this key's content view environments, update the activation key via Hammer.
</span>
</div>

Expand Down Expand Up @@ -57,7 +57,6 @@ <h4 translate>Basic Information</h4>
</div>
</dd>


<div class="divider"></div>
<h4 translate>System Purpose</h4>

Expand Down Expand Up @@ -123,17 +122,18 @@ <h4 translate>Activation Key Content</h4>
on-delete="clearReleaseVersion()"
on-save="save(activationKey)">
</dd>

</dl>
<dl class="dl-horizontal dl-horizontal-left" ng-if="!activationKey.multi_content_view_environment">
<dt bst-feature-flag="lifecycle_environments">
<span translate>Environment</span>
</dt>
<dd bst-feature-flag="lifecycle_environments">
<div path-selector="environments"
ng-model="activationKey.environment"
mode="singleSelect"
selection-required="selectionRequired"
disabled="denied('edit_activation_keys', activationKey)"
disable-trigger="disableEnvironmentSelection">
ng-model="activationKey.environment"
mode="singleSelect"
selection-required="selectionRequired"
disabled="denied('edit_activation_keys', activationKey)"
disable-trigger="disableEnvironmentSelection">
</div>
</dd>

Expand All @@ -142,14 +142,14 @@ <h4 translate>Activation Key Content</h4>
</dt>
<dd bst-feature-flag="lifecycle_environments">
<div bst-edit-select="activationKey.content_view.name"
readonly="denied('edit_activation_keys', activationKey) || activationKey.environment === undefined || activationKey.environment === null"
selector="activationKey.content_view.id"
options="contentViews()"
on-cancel="cancelContentViewUpdate()"
on-save="saveContentView(activationKey)"
deletable="activationKey.content_view"
on-delete="resetEnvironment(activationKey)"
edit-trigger="editContentView">
readonly="denied('edit_activation_keys', activationKey) || activationKey.environment === undefined || activationKey.environment === null"
selector="activationKey.content_view.id"
options="contentViews()"
on-cancel="cancelContentViewUpdate()"
on-save="saveContentView(activationKey)"
deletable="activationKey.content_view"
on-delete="resetEnvironment(activationKey)"
edit-trigger="editContentView">
</div>

<div bst-alert="info" ng-show="editEnvironment">
Expand All @@ -160,6 +160,14 @@ <h4 translate>Activation Key Content</h4>
</div>
</dd>
</dl>
<dl class="dl-horizontal dl-horizontal-left">
<span id="ak-cve-details" data-ak-details="{{ activationKey }}" class="hidden"></span>
<foreman-react-component
name="CVEDetailsCard"
data-props=""
Comment on lines +164 to +167
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to solve a few problems here.

First, foreman-react-component is supposed to be able to take props directly via data-props. It then parses them as JSON and uses them. But I tried giving it data-props="{{ activationKey }}" and got an error about unexpected token in JSON. It was trying to parse "{{ activationKey }}" as JSON, which means that it was somehow getting to the DOM before AngularJS had a chance to evaluate it and replace "{{ activationKey }}" with the activation key's JSON data. So I tried putting the data in a hidden span, and then reading it below in CVEDetailsCard.js with JSON.parse(node.dataset.akDetails).

Now, it works on initial page load. But when I clicked around or used the breadcrumb switcher, the AK card wasn't updating. I found that the data-ak-details was actually updating, but only after React rendered the component. Since the data is in a DOM element and outside the React ecosystem, I needed a solution to monitor when the data attribute changed.

After some research I landed on using a MutationObserver, and connecting it to React via useRef and state. This seems to work well - it works both on initial page load, and on updates.

ng-if="activationKey.multi_content_view_environment"
></foreman-react-component>
</dl>
</div>
</div>

Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ <h2 translate>Activation Keys</h2>
<th bst-table-column="consumed"><span translate>Host Limit</span></th>
<th bst-table-column="environment"><span translate>Environment</span></th>
<th bst-table-column="contentView"><span translate>Content View</span></th>
<th bst-table-column="multi-cv"><span translate>Multi Content View Environment</span></th>
</tr>
</thead>

Expand All @@ -48,6 +49,7 @@ <h2 translate>Activation Keys</h2>
<td bst-table-cell>{{ activationKey | activationKeyConsumedFilter }}</td>
<td bst-table-cell>{{ activationKey.environment.name || "" }}</td>
<td bst-table-cell>{{ activationKey.content_view.name || "" }}</td>
<td bst-table-cell>{{ activationKey.multi_content_view_environment ? 'Yes' : 'No' }}</td>
</tr>
</tbody>
</table>
Expand Down
20 changes: 19 additions & 1 deletion test/models/content_view_environment_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,26 @@ def test_fetch_content_view_environments_ids
def test_fetch_content_view_environments_invalid_ids_does_not_mutate_array
dev = katello_environments(:dev)
input_ids = [0, 999]
assert_equal [], ContentViewEnvironment.fetch_content_view_environments(ids: input_ids, organization: dev.organization)
assert_raises(HttpErrors::UnprocessableEntity) do
ContentViewEnvironment.fetch_content_view_environments(ids: input_ids, organization: dev.organization)
end
assert_equal [0, 999], input_ids # should not have a map! which mutates the input array
end

def test_fetch_content_view_environments_mixed_validity_candlepin_names
dev = katello_environments(:dev)
assert_raises(HttpErrors::UnprocessableEntity) do
ContentViewEnvironment.fetch_content_view_environments(labels: ['published_dev_view_dev, bogus'], organization: dev.organization)
end
end

def test_fetch_content_view_environments_mixed_validity_ids
dev = katello_environments(:dev)
view = katello_content_views(:library_dev_view)
cve = Katello::ContentViewEnvironment.where(:environment_id => dev, :content_view_id => view).first
assert_raises(HttpErrors::UnprocessableEntity) do
ContentViewEnvironment.fetch_content_view_environments(ids: [cve.id, 9999], organization: dev.organization)
end
end
end
end
Loading
Loading