Fixes #31492 - Add basic hosts and inputs page #556

MariaAga · 2020-12-10T14:17:10Z

To only include the hosts selection method (without the actual selection), show host chips and show inputs.
Depends on: #554 for the input templates

MariaAga · 2021-04-09T08:17:49Z

webpack/JobWizard/steps/CategoryAndTemplate/CategoryAndTemplate.js

+      <Title headingLevel="h2" className="wizard-title">
+        {__('Category And Template')}
+      </Title>
+      <Text component={TextVariants.p}>{__('All fields are required.')}</Text>


Suggested change

<Title headingLevel="h2" className="wizard-title">

{__('Category And Template')}

</Title>

<Text component={TextVariants.p}>{__('All fields are required.')}</Text>

<div className="wizard-title">

<Title headingLevel="h2">{__('Category And Template')}</Title>

<Text component={TextVariants.p}>{__('All fields are required.')}</Text>

</div>

MariaAga · 2021-07-06T14:52:20Z

@LaViro This is also ready for a review

xprazak2 · 2021-07-12T07:03:06Z

Looks nice. A couple of thoughts

I might be missing something but there are resource categories in the dropdown select - how do we use it to select the targeted hosts? If user selects for example 'Host group', how do we know which hostgroup(s) the job should target?
Edit: I just noticed Fixes #32314 - Add target selection in job wizard #578. It seems like it is addressed there.
Should selecting a different category clear the chips?
I see warnings in the console:
React does not recognize the 'isRequired' prop on a DOM element
React does not recognize the 'labelText' prop on a DOM element
If there is a required input, should transitioning to the next step be allowed?

webpack/JobWizard/steps/HostsAndInputs/__tests__/TemplateInputs.test.js

MariaAga · 2021-07-12T08:28:00Z

I don't think so? I don't know why a user might change a template in the middle of the form but if they selected the wrong one by mistake it would not be fun to re-select hosts
is fixed in this pr: Refs #31459 - remove snapshot testing in the job wizard #607
will be addressed here: https://projects.theforeman.org/issues/32667

adamruzicka · 2021-07-12T08:44:41Z

Needs a rebase

MariaAga · 2021-07-12T14:36:24Z

rebased and changed the tests

Ron-Lavi

Great work @MariaAga !
PR looks great and since it's small and readable,
I would suggest adding here some actual data

webpack/JobWizard/JobWizard.js

webpack/JobWizard/steps/CategoryAndTemplate/CategoryAndTemplate.js

webpack/JobWizard/steps/HostsAndInputs/TemplateInputs.js

webpack/JobWizard/JobWizard.js

MariaAga · 2021-07-13T10:12:56Z

The next pr adds data and has 750+ lines so I think its best to keep it split (#578) .
Also this pr uses real user input in the templates.

webpack/JobWizard/JobWizardConstants.js

adamruzicka · 2021-07-26T11:58:30Z

Could we set minimum width and height on the input fields?

By default it looks good

but you can resize it by hand into something like

MariaAga · 2021-07-27T16:09:08Z

added min size to the css, but generally I don't think it's a design issue that the user can do that. In the old form it was possible as well

adamruzicka · 2021-08-03T08:58:11Z

Looks good to me. @LaViro could you do a final review here?

Ron-Lavi

Thanks @MariaAga ! works nicely,
left a few comments

webpack/JobWizard/JobWizard.js

Ron-Lavi · 2021-08-24T11:53:23Z

webpack/JobWizard/steps/AdvancedFields/__tests__/AdvancedFields.test.js

@@ -72,7 +64,7 @@ describe('AdvancedFields', () => {
      .simulate('click');

    expect(wrapper.find('.pf-c-wizard__nav-link.pf-m-current').text()).toEqual(
-      'Target Hosts'
+      'Target hosts and inputs'


should it be WIZARD_TITLES.hostsAndInputs ?

We usually dont use consts in tests. I think this could cause issues if there is an issue with the constant, for example if it's undefined or has a typo.

Ron-Lavi · 2021-08-24T11:54:03Z

webpack/JobWizard/steps/HostsAndInputs/__tests__/SelectedChips.test.js

+    );
+    await act(async () => {
+      await fireEvent.click(
+        screen.getByText('Target hosts and inputs', { selector: 'button' })


Suggested change

screen.getByText('Target hosts and inputs', { selector: 'button' })

screen.getByText(WIZARD_TITLES.hostsAndInputs, { selector: 'button' })

Ron-Lavi

ACK, thanks @MariaAga !
test error seem unrelated, happens due to minor version update of PF iiuc

adamruzicka

Let's get this in

adamruzicka · 2021-08-25T10:18:58Z

Thank you @MariaAga & @LaViro !

(cherry picked from commit f6b84c8)

MariaAga changed the title ~~Add hosts job wizard~~ Fixes #31492 - Add basic hosts and inputs page Dec 10, 2020

theforeman-bot added Needs testing Not yet reviewed labels Dec 10, 2020

MariaAga force-pushed the add-hosts-job-wizard branch from c71818e to f825171 Compare April 9, 2021 08:10

MariaAga commented Apr 9, 2021

View reviewed changes

MariaAga mentioned this pull request Apr 16, 2021

Fixes #32314 - Add target selection in job wizard #578

Merged

MariaAga mentioned this pull request Jun 25, 2021

Fixes #32847 - job wizard description field with user data #605

Merged

MariaAga force-pushed the add-hosts-job-wizard branch from f825171 to 7bef8d4 Compare July 2, 2021 17:18

MariaAga marked this pull request as ready for review July 6, 2021 14:51

xprazak2 reviewed Jul 12, 2021

View reviewed changes

webpack/JobWizard/steps/HostsAndInputs/__tests__/TemplateInputs.test.js Outdated Show resolved Hide resolved

MariaAga force-pushed the add-hosts-job-wizard branch 2 times, most recently from 9093181 to 94f3124 Compare July 12, 2021 14:35

Ron-Lavi reviewed Jul 13, 2021

View reviewed changes

webpack/JobWizard/JobWizardConstants.js Outdated Show resolved Hide resolved

MariaAga force-pushed the add-hosts-job-wizard branch 2 times, most recently from a6bd51b to dc45ae4 Compare July 13, 2021 13:15

This was referenced Jul 15, 2021

Fixes #31460 - add description field to job wizard #555

Merged

Fixes #32667 - add job wizard form validation #622

Merged

MariaAga force-pushed the add-hosts-job-wizard branch from dc45ae4 to f1df785 Compare July 21, 2021 08:20

adamruzicka added the Waiting on contributor label Jul 27, 2021

MariaAga force-pushed the add-hosts-job-wizard branch from f1df785 to 9f98b89 Compare July 27, 2021 16:08

theforeman-bot removed the Waiting on contributor label Jul 27, 2021

Ron-Lavi reviewed Aug 24, 2021

View reviewed changes

MariaAga and others added 3 commits August 24, 2021 16:38

Fixes #31492 - Add basic hosts and inputs page

222ae33

Refs #31492 - add WizardTitle

f8b7865

Refs #31492 - after review

2290106

MariaAga force-pushed the add-hosts-job-wizard branch from 9f98b89 to 2290106 Compare August 24, 2021 15:51

Ron-Lavi approved these changes Aug 25, 2021

View reviewed changes

theforeman-bot removed the Not yet reviewed label Aug 25, 2021

adamruzicka approved these changes Aug 25, 2021

View reviewed changes

adamruzicka merged commit f6b84c8 into theforeman:master Aug 25, 2021

adamruzicka pushed a commit that referenced this pull request Aug 31, 2021

Fixes #31492 - Add basic hosts and inputs page (#556)

6d88701

(cherry picked from commit f6b84c8)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #31492 - Add basic hosts and inputs page #556

Fixes #31492 - Add basic hosts and inputs page #556

MariaAga commented Dec 10, 2020

MariaAga Apr 9, 2021

MariaAga commented Jul 6, 2021

xprazak2 commented Jul 12, 2021 •

edited

MariaAga commented Jul 12, 2021 •

edited

adamruzicka commented Jul 12, 2021

MariaAga commented Jul 12, 2021

Ron-Lavi left a comment

MariaAga commented Jul 13, 2021 •

edited

adamruzicka commented Jul 26, 2021

MariaAga commented Jul 27, 2021

adamruzicka commented Aug 3, 2021

Ron-Lavi left a comment

Ron-Lavi Aug 24, 2021

MariaAga Aug 24, 2021

Ron-Lavi Aug 24, 2021

Ron-Lavi left a comment

adamruzicka left a comment

adamruzicka commented Aug 25, 2021

	screen.getByText('Target hosts and inputs', { selector: 'button' })
	screen.getByText(WIZARD_TITLES.hostsAndInputs, { selector: 'button' })

Fixes #31492 - Add basic hosts and inputs page #556

Fixes #31492 - Add basic hosts and inputs page #556

Conversation

MariaAga commented Dec 10, 2020

MariaAga Apr 9, 2021

Choose a reason for hiding this comment

MariaAga commented Jul 6, 2021

xprazak2 commented Jul 12, 2021 • edited

MariaAga commented Jul 12, 2021 • edited

adamruzicka commented Jul 12, 2021

MariaAga commented Jul 12, 2021

Ron-Lavi left a comment

Choose a reason for hiding this comment

MariaAga commented Jul 13, 2021 • edited

adamruzicka commented Jul 26, 2021

MariaAga commented Jul 27, 2021

adamruzicka commented Aug 3, 2021

Ron-Lavi left a comment

Choose a reason for hiding this comment

Ron-Lavi Aug 24, 2021

Choose a reason for hiding this comment

MariaAga Aug 24, 2021

Choose a reason for hiding this comment

Ron-Lavi Aug 24, 2021

Choose a reason for hiding this comment

Ron-Lavi left a comment

Choose a reason for hiding this comment

adamruzicka left a comment

Choose a reason for hiding this comment

adamruzicka commented Aug 25, 2021

xprazak2 commented Jul 12, 2021 •

edited

MariaAga commented Jul 12, 2021 •

edited

MariaAga commented Jul 13, 2021 •

edited