Fixes #36295 - Auto activation key selection if there's only one #10518

girijaasoni · 2023-04-14T12:37:28Z

This PR is blocked by Foreman PR:
theforeman/foreman#9688

This PR has changes t
o auto select an activation key when there's just one and to remove the "No activation key selected error" on User's arrival to the register form. This pr also has better validation formatting like the exclamation icon on the left of carter and underline on the Select dropdown when there's an error! Screenshots of the changes are attached below:

theforeman-bot · 2023-04-14T12:37:30Z

Can one of the admins verify this patch?

theforeman-bot · 2023-04-14T12:37:32Z

Issues: #36295

Ron-Lavi

Thanks @girijaasoni !

Ron-Lavi · 2023-04-16T13:20:52Z

webpack/components/extensions/RegistrationCommands/fields/ActivationKeys.js

-
+
+  useEffect(() => {
+    if(activationKeys.length==1){


Suggested change

if(activationKeys.length==1){

if (activationKeys.length === 1) {

@girijaasoni npm run lint -- --fix is your friend

Thanks @stejskalleos

Ron-Lavi · 2023-04-16T13:26:00Z

webpack/components/extensions/RegistrationCommands/fields/ActivationKeys.js

+
+  useEffect(() => {
+    if(activationKeys.length==1){
+      onChange({ activationKeys: [activationKeys[0]?.name]})


maybe you could reuse the onSelect function above instead by passing the value of the activation key?

Ron-Lavi · 2023-04-16T13:29:15Z

webpack/components/extensions/RegistrationCommands/fields/ActivationKeys.js

+    if(activationKeys.length==1){
+      onChange({ activationKeys: [activationKeys[0]?.name]})
+    }
+  }, [activationKeys]);


we should avoid objects/arrays as dependencies: https://dmitripavlutin.com/react-useeffect-infinite-loop/#21-avoid-objects-as-dependencies

Thanks @Ron-Lavi

stejskalleos

Two nits, otherwise code looks good, but haven't tested it yet.

stejskalleos · 2023-04-18T03:05:14Z

webpack/components/extensions/RegistrationCommands/fields/ActivationKeys.js

@@ -38,10 +38,16 @@ const ActivationKeys = ({
  };

  // Validate field when hostgroup is changed (host group may have some keys)
-  useEffect(() => {
+  useEffect(() => { 


NIT: Trailing space, check the configuration of VS code, Trim Trailing Whitespace should be enabled by default if I am not mistaken

stejskalleos · 2023-04-18T03:06:51Z

webpack/components/extensions/RegistrationCommands/fields/ActivationKeys.js

    handleInvalidField('Activation Keys', akHasValidValue(hostGroupId, pluginValues?.activationKeys, hostGroupActivationKeys));
  }, [handleInvalidField, hostGroupId, hostGroupActivationKeys, pluginValues]);

+  useEffect(() => {
+    if (activationKeys?.length === 1) {
+      updatePluginValues([activationKeys[0]?.name]);


If activationKeys is not empty, we can safely assume that the key has a name attribute:
updatePluginValues([activationKeys[0].name]);

girijaasoni · 2023-04-18T07:31:50Z

webpack/components/extensions/RegistrationCommands/fields/ActivationKeys.js

@@ -88,7 +94,7 @@ ActivationKeys.propTypes = {
 };

 ActivationKeys.defaultProps = {
-  activationKeys: undefined,
+  activationKeys: [],


activationKeys is set to "[]" in the useEffect of indexJs

yeah I know, but it was supposed to be [] from the beginning ...

undefined is for first form load so the field is not marked as invalid

girijaasoni · 2023-04-18T07:31:50Z

webpack/components/extensions/RegistrationCommands/fields/ActivationKeys.js

@@ -88,7 +94,7 @@ ActivationKeys.propTypes = {
 };

 ActivationKeys.defaultProps = {
-  activationKeys: undefined,
+  activationKeys: [],


activationKeys is set to "[]" in the useEffect of indexJs

Ron-Lavi

Nice work @girijaasoni !

When there is only one activation key available during host registration, the Activation Key is automatically selected.
When multiple activation keys are available during host registration, the Activation Key is not automatically selected.
The auto-selection of the Activation Key does not interfere with the ability to manually select or deselect the option.
There's a warning alert shown initially when user's have multiple AK (not sure if you are going to address it in this PR):

girijaasoni · 2023-04-19T07:05:17Z

Nice work @girijaasoni !

When there is only one activation key available during host registration, the Activation Key is automatically selected.

When multiple activation keys are available during host registration, the Activation Key is not automatically selected.

The auto-selection of the Activation Key does not interfere with the ability to manually select or deselect the option.

There's a warning alert shown initially when user's have multiple AK (not sure if you are going to address it in this PR):

That is done in the foreman pr which blocks this pr theforeman/foreman#9688

Ron-Lavi · 2023-04-19T07:55:39Z

webpack/components/extensions/RegistrationCommands/fields/ActivationKeys.js

  return (
    <FormGroup
      label={__('Activation Keys')}
      fieldId="reg_katello_ak"
      helperText={hostGroupActivationKeys && sprintf('From host group: %s', hostGroupActivationKeys)}
      helperTextInvalid={activationKeys?.length === 0 ? <a href="/activation_keys/new">{__('Create new activation key')}</a> : __('No Activation Keys selected')}
-      validated={validateAKField(hostGroupId, pluginValues?.activationKeys, hostGroupActivationKeys)}
+      helperTextInvalidIcon={<ExclamationCircleIcon />}


Not sure if we should remove the {activationKeys?.length === 0 ? <a href="/activation_keys/new">{__('Create new activation key')}</a>

Why remove it?

I don't see the "Create new activation key" link any more. I see no reason to remove it; can you please put it back?

Ron-Lavi · 2023-04-19T08:00:13Z

webpack/components/extensions/RegistrationCommands/fields/ActivationKeys.js

+    if (activationKeys?.length === 1) {
+      updatePluginValues([activationKeys[0].name]);
+    }
+  }, [activationKeys?.length]);


not entirely against using optional chaining (?.)
though proptypes should protect us in this case and we can expect it to be an array,
I think we would be ok not using it in this case

Optional chaining is required for the case when activationKeys is undefined (on form load)

stejskalleos

One nit about PropTypes otherwise works fine.

stejskalleos · 2023-04-21T05:39:02Z

webpack/components/extensions/RegistrationCommands/fields/ActivationKeys.js

@@ -88,7 +94,7 @@ ActivationKeys.propTypes = {
 };

 ActivationKeys.defaultProps = {
-  activationKeys: undefined,
+  activationKeys: [],


undefined is for first form load so the field is not marked as invalid

stejskalleos · 2023-04-21T05:39:36Z

webpack/components/extensions/RegistrationCommands/index.js

@@ -73,7 +73,7 @@ export const RegistrationActivationKeys = ({
  isLoading,
 }) => {
  useEffect(() => {
-    onChange({ activationKeys: [] });
+    onChange({ activationKeys: undefined });


PropTypes.oneOfType([PropTypes.undefined, PropTypes.array])

imho it's not a best practice to pass the value as undefined
if we are doing this in order to hide/display proper error on the field, e.g:
hide the error when the user opens the page,
and show it upon interaction with the field,

I propose to add a const [ wasInteracted, setInteraction ] = useState(false)
and then use the wasInteracted in the right places.

This should give us more control over what we are trying to achieve.

I'm not against it, it makes sense, just didn't know that it's a bad practice.

@jeremylenz @MariaAga How do you handle touched / untouched form fields in React components?

I used onBlur={() => setWasFocus(true) here: theforeman/foreman_remote_execution#784

Ron-Lavi

@girijaasoni snapshot test is failing, could you update it?
npm test -- -u

jeremylenz

Hi @girijaasoni, I left a couple code comments below :)

jeremylenz · 2023-05-24T18:25:10Z

webpack/components/extensions/RegistrationCommands/fields/ActivationKeys.js

+  useEffect(() => {
+    if (activationKeys?.length === 1) {
+      updatePluginValues([activationKeys[0].name]);
+    }
+    if (activationKeys?.length === 0) {
+      setHasInteraction(true);
+    }
+  }, [activationKeys?.length]);


In general, setting state in effects should be avoided if possible.

Rather, setting state in event handlers is much better. For hasInteraction, I think we can achieve the same thing by adding an onFocus to the <FormGroup>:

<FormGroup onFocus={() => setHasInteraction(true)}

And for the pluginValues, I would strongly prefer if that could be updated outside an effect as well, but it seems slot and fill makes that too difficult. So I'm ok with keeping that part.

Either way, this is really good reading: https://react.dev/learn/you-might-not-need-an-effect

jeremylenz · 2023-05-24T18:25:50Z

webpack/components/extensions/RegistrationCommands/fields/ActivationKeys.js

+  useEffect(() => {
+    if (activationKeys?.length === 1) {
+      updatePluginValues([activationKeys[0].name]);
+    }
+    if (activationKeys?.length === 0) {
+      setHasInteraction(true);
+    }
+  }, [activationKeys?.length]);


However this effect ends up, the dependency array needs to be correct :)

jeremylenz · 2023-05-24T18:26:59Z

webpack/components/extensions/RegistrationCommands/fields/ActivationKeys.js

+  hostGroupActivationKeys: PropTypes.oneOfType([
+    PropTypes.string,
+    PropTypes.array,
+  ]),
  hostGroupId: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
  pluginValues: PropTypes.objectOf(PropTypes.shape({})),


I got this console error; seems like this needs an update:

Warning: Failed prop type: Invalid prop `pluginValues.activationKeys` of type `array` supplied to `ActivationKeys`, expected `object`

jeremylenz · 2023-08-07T15:06:59Z

@girijaasoni any update here? This is still on our review board.

girijaasoni · 2023-08-08T08:23:10Z

@girijaasoni any update here? This is still on our review board.

Since it was for 6.15, I paused the work on it, back on it now again :D

jeremylenz

The useEffect code is looking much better and the page is mostly working well! 👍 Just a couple more minor comments below.

jeremylenz · 2023-08-11T13:36:26Z

...mponents/extensions/RegistrationCommands/__tests__/__snapshots__/ActivationKeys.test.js.snap

@@ -2,7 +2,7 @@

 exports[`ActivationKeys renders 1`] = `
 <FormGroup
-  fieldId="activation_keys_field"
+  fieldId="reg_katello_ak"


I thought QE wanted us to remove "katello" from these? Or did I have that switched around?

jeremylenz · 2023-08-11T13:39:46Z

webpack/components/extensions/RegistrationCommands/fields/ActivationKeys.js

+      }
+      helperTextInvalidIcon={<ExclamationCircleIcon />}
+      labelIcon={
+        <LabelIcon text={__('Activation key(s) for Subscription Manager.')} />


Since subscription-manager is going away at some point, this might be a good time to update the wording here.

Suggested change

<LabelIcon text={__('Activation key(s) for Subscription Manager.')} />

<LabelIcon text={__('Activation key(s) to use during registration.')} />

jeremylenz · 2023-08-11T13:46:57Z

webpack/components/extensions/RegistrationCommands/fields/ActivationKeys.js

  return (
    <FormGroup
      label={__('Activation Keys')}
      fieldId="reg_katello_ak"
      helperText={hostGroupActivationKeys && sprintf('From host group: %s', hostGroupActivationKeys)}
      helperTextInvalid={activationKeys?.length === 0 ? <a href="/activation_keys/new">{__('Create new activation key')}</a> : __('No Activation Keys selected')}
-      validated={validateAKField(hostGroupId, pluginValues?.activationKeys, hostGroupActivationKeys)}
+      helperTextInvalidIcon={<ExclamationCircleIcon />}


I don't see the "Create new activation key" link any more. I see no reason to remove it; can you please put it back?

jeremylenz

Tested all three scenarios

✔️ No AKs available
✔️ Only one AK
✔️ Multiple AKs

All works great. Nice work! APJ 👍

jeremylenz · 2023-08-21T13:37:38Z

[test katello]

jeremylenz · 2023-08-21T14:11:50Z

Test failures unrelated

jeremylenz · 2023-08-21T21:35:31Z

@girijaasoni If you rebase to current master the test failures should go away now :)

girijaasoni · 2023-08-23T14:07:48Z

[test katello]

jeremylenz · 2023-08-23T14:39:26Z

[test katello]

theforeman-bot added Waiting on contributor Not yet reviewed labels Apr 14, 2023

girijaasoni changed the title ~~Fixes #36295 - Auto activation key selection in case of one activatio…~~ Fixes #36295 - Auto activation key selection if there's only one Apr 14, 2023

Ron-Lavi reviewed Apr 16, 2023

View reviewed changes

girijaasoni force-pushed the 36295 branch from 15c96d2 to 9783099 Compare April 17, 2023 17:51

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Apr 17, 2023

stejskalleos suggested changes Apr 18, 2023

View reviewed changes

girijaasoni commented Apr 18, 2023

View reviewed changes

girijaasoni force-pushed the 36295 branch from 9783099 to 045f112 Compare April 18, 2023 11:08

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Apr 18, 2023

girijaasoni mentioned this pull request Apr 18, 2023

Fixes #36312 - Remove toast alert for register host validation theforeman/foreman#9688

Merged

girijaasoni requested review from stejskalleos and Ron-Lavi April 18, 2023 11:17

girijaasoni force-pushed the 36295 branch from 045f112 to 24e95dd Compare April 18, 2023 11:30

theforeman-bot removed the Waiting on contributor label Apr 18, 2023

Ron-Lavi reviewed Apr 19, 2023

View reviewed changes

stejskalleos suggested changes Apr 21, 2023

View reviewed changes

girijaasoni force-pushed the 36295 branch 3 times, most recently from c1f4c4a to 38409df Compare April 26, 2023 14:59

Ron-Lavi reviewed Apr 30, 2023

View reviewed changes

girijaasoni force-pushed the 36295 branch 2 times, most recently from 2f87ffd to ecc90cb Compare May 24, 2023 13:08

jeremylenz requested changes May 24, 2023

View reviewed changes

girijaasoni force-pushed the 36295 branch from ecc90cb to 46764d3 Compare August 10, 2023 10:56

girijaasoni closed this Aug 11, 2023

girijaasoni force-pushed the 36295 branch from 46764d3 to f9ce129 Compare August 11, 2023 07:28

girijaasoni reopened this Aug 11, 2023

girijaasoni requested review from jeremylenz and stejskalleos August 11, 2023 08:35

jeremylenz reviewed Aug 11, 2023

View reviewed changes

girijaasoni force-pushed the 36295 branch from 6856846 to 875c4ae Compare August 21, 2023 07:56

girijaasoni requested a review from jeremylenz August 21, 2023 07:58

jeremylenz approved these changes Aug 21, 2023

View reviewed changes

Fixes #36295 - Auto activation key selection if there's only one

4531560

girijaasoni force-pushed the 36295 branch from 875c4ae to 4531560 Compare August 23, 2023 13:56

jeremylenz merged commit e6e766a into Katello:master Aug 23, 2023
4 of 5 checks passed

	if(activationKeys.length==1){
	if (activationKeys.length === 1) {

	<LabelIcon text={__('Activation key(s) for Subscription Manager.')} />
	<LabelIcon text={__('Activation key(s) to use during registration.')} />

Fixes #36295 - Auto activation key selection if there's only one #10518

Fixes #36295 - Auto activation key selection if there's only one #10518

Conversation

girijaasoni commented Apr 14, 2023 • edited Loading

theforeman-bot commented Apr 14, 2023

theforeman-bot commented Apr 14, 2023

Ron-Lavi 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

stejskalleos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girijaasoni Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girijaasoni Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Ron-Lavi left a comment

Choose a reason for hiding this comment

girijaasoni commented Apr 19, 2023

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

stejskalleos Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

stejskalleos 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

Ron-Lavi left a 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 commented Aug 7, 2023

girijaasoni commented Aug 8, 2023

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

Choose a reason for hiding this comment

jeremylenz commented Aug 21, 2023

jeremylenz commented Aug 21, 2023

jeremylenz commented Aug 21, 2023

girijaasoni commented Aug 23, 2023

jeremylenz commented Aug 23, 2023

girijaasoni commented Apr 14, 2023 •

edited

Loading

girijaasoni Apr 18, 2023 •

edited

Loading

girijaasoni Apr 18, 2023 •

edited

Loading

stejskalleos Apr 20, 2023 •

edited

Loading