-
Notifications
You must be signed in to change notification settings - Fork 290
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
Conversation
<span id="ak-cve-details" data-ak-details="{{ activationKey }}" class="hidden"></span> | ||
<foreman-react-component | ||
name="CVEDetailsCard" | ||
data-props="" |
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.
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.
35b3b34
to
3a39a4b
Compare
🍏 |
rebased |
Suggested patch. This prevents an annoying UI error and worked well for me w.r.t to the angular pages and hammer ak update calls. I have not tried the react page. diff --git a/app/controllers/katello/api/v2/activation_keys_controller.rb b/app/controllers/katello/api/v2/activation_keys_controller.rb
index cd5b0ddecb..e976548207 100644
--- a/app/controllers/katello/api/v2/activation_keys_controller.rb
+++ b/app/controllers/katello/api/v2/activation_keys_controller.rb
@@ -304,7 +304,8 @@ module Katello
@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[:multi_content_view_environment] != false &&
+ (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], |
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.
Seems to work ok for me. I tested
- AK with mulitple cve - Angular UI/Hammer
- AK with single cve - Angular UI/Hammer
- AK with no cve - Angular UI/Hammer - (X button on the ui)
- The details page in react (that doesnt show content views)
LGTM. APJ
...ack/components/extensions/HostDetails/Cards/ContentViewDetailsCard/ContentViewDetailsCard.js
Outdated
Show resolved
Hide resolved
}), | ||
})), | ||
hostPermissions: PropTypes.shape({ | ||
edit_hosts: PropTypes.bool, |
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.
needs to be tailored to work with AK
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.
In AK CVEDetailsBareCard
is only for display, so we don't need to worry about permissions (yet).
} | ||
end | ||
node :candlepin_name do |cve| | ||
cve.candlepin_name |
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.
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..
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.
It's also called just label
..
I've always been fond of "candlepin_name" 😢 but I guess I can change it.
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.
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..
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 good..Ack 👍🏼
One thing we can discuss more is the Multiple Content View Environment column on the AK table..It's not controlled by the multi-CV setting..Could be unnecessary information for single-CV setups but also a good way to familiarize users of the feature if they miss the RN.
I thought about this too.. But thinking about other places in the UI that we show multi-CV, those are not controlled by setting either. And if you have multi-env AK's/hosts and turn the setting off, they will still continue to work and show in the UI. |
Merging with a couple outstanding issues to be resolved in a followup:
|
What are the changes introduced in this pull request?
Display multiCV activation key details in the web UI. Does not include changing multiCV activation keys' content view environments.
ContentViewEnvironmentDetails
card into aCVEDetailsBareCard
so we can reuse it. This was necessary because theContentViewEnvironmentDetails
was coupled to auseUrlParams
which made it not reusable enough.content_view_environments
nodes to activation key rablConsiderations taken when implementing this change?
I'm using
foreman-react-component
(its first use in Katello) to render a React component on an AngularJS page. It's a bit hacky but it works!When an activation key is single-environment, we fall back to displaying what we always have, rather than displaying the new card. This should help with existing web UI automation tests.
What are the testing steps for this pull request?
Create one or more multiCV activation keys
Test both pages
make sure edits to other fields still work
display should work fine as well