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 #31460 - add description field to job wizard #555

Merged

Conversation

MariaAga
Copy link
Member

@MariaAga MariaAga commented Dec 7, 2020

Adds the final field to the advanced step. Depends on #554
As the step to edit the template inputs doesn't exist yet, it doesn't use the value of the input but the name of the input instead.

New:
Screenshot from 2020-12-07 11-43-51
onclick it moves to this:

Screenshot from 2020-12-07 11-43-47

old:
image (3)

Original generate description is here:

function regenerate_description(thing) {
var fieldset = $(thing).closest('fieldset');
var dict = load_keys(fieldset);
var format = fieldset.find('.description_format').val();
fieldset.find('.description').val(String.format(format, dict));
}
function load_keys(parent) {
var dict = {};
var pattern = $(parent).find(".description_format").val();
var re = new RegExp("%\\{([^\\}]+)\\}", "gm");
var match = re.exec(pattern);
while(match != null) {
dict[match[1]] = $(parent).find("#" + match[1]).val();
match = re.exec(pattern);
}
dict['job_category'] = $('#job_invocation_job_category').val();
dict['template_name'] = $('.job_template_selector:visible span.select2-chosen').html();
return dict;
}

@MariaAga
Copy link
Member Author

MariaAga commented Jul 6, 2021

@LaViro can you take a look? test failures are broken snapshots after pf4 update

Copy link
Member

@Ron-Lavi Ron-Lavi left a comment

Choose a reason for hiding this comment

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

Thanks @MariaAga,
looks nice! left a few comments,
on my end, it actually doesn't remove the braces wrapper when switching to preview:
Run job

webpack/JobWizard/JobWizard.js Outdated Show resolved Hide resolved
webpack/JobWizard/steps/AdvancedFields/AdvancedFields.js Outdated Show resolved Hide resolved
webpack/JobWizard/steps/AdvancedFields/DescriptionField.js Outdated Show resolved Hide resolved
@MariaAga MariaAga force-pushed the advanced-description-fields-exp branch from dcebace to 171235d Compare July 13, 2021 10:27
@MariaAga
Copy link
Member Author

MariaAga commented Jul 13, 2021

@LaViro Fixed the braces wrapper issue. I didn't include the advanced template and regular templates split in this pr before.

@MariaAga MariaAga force-pushed the advanced-description-fields-exp branch from 171235d to e6e88c0 Compare July 13, 2021 10:42
Comment on lines +22 to +44
({
data: {
advanced_template_inputs,
effective_user,
job_template: { executionTimeoutInterval, description_format },
},
}) => {
const advancedTemplateValues = {};
const advancedInputs = advanced_template_inputs;
if (advancedInputs) {
advancedInputs.forEach(input => {
advancedTemplateValues[input.name] = input?.default || '';
});
}
setAdvancedValues(currentAdvancedValues => ({
...currentAdvancedValues,
effectiveUserValue: effective_user?.value || '',
timeoutToKill: executionTimeoutInterval || '',
templateValues: advancedTemplateValues,
description: description_format || '',
}));
},
[]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
({
data: {
advanced_template_inputs,
effective_user,
job_template: { executionTimeoutInterval, description_format },
},
}) => {
const advancedTemplateValues = {};
const advancedInputs = advanced_template_inputs;
if (advancedInputs) {
advancedInputs.forEach(input => {
advancedTemplateValues[input.name] = input?.default || '';
});
}
setAdvancedValues(currentAdvancedValues => ({
...currentAdvancedValues,
effectiveUserValue: effective_user?.value || '',
timeoutToKill: executionTimeoutInterval || '',
templateValues: advancedTemplateValues,
description: description_format || '',
}));
},
[]
({
data: {
advanced_template_inputs = [],
effective_user,
job_template: { executionTimeoutInterval, description_format },
},
}) => {
const templateValues = {};
advanced_template_inputs.forEach(input => {
templateValues[input.name] = input?.default || '';
});
setAdvancedValues(currentAdvancedValues => ({
...currentAdvancedValues,
effectiveUserValue: effective_user?.value || '',
timeoutToKill: executionTimeoutInterval || '',
templateValues,
description: description_format || '',
}));
},
[]

Copy link
Member Author

Choose a reason for hiding this comment

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

advancedTemplateValues and templateValues are different as there are advanced_template_inputs and template_inputs

Copy link
Member

Choose a reason for hiding this comment

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

I just removed the const declaration from line 30 and used advanced_template_inputs directly,

and then renamed advancedTemplateValues on line 29 to templateValues
so in line 40 you can use it directly as templateValues, instead of:
templateValues: advancedTemplateValues,

Copy link
Member Author

Choose a reason for hiding this comment

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

templateValues is a defined const already that is used in this pr: #556

const [templateValues, setTemplateValues] = useState({}); // TODO use templateValues in advanced fields - description
const [selectedHosts, setSelectedHosts] = useState(['host1', 'host2']);
const dispatch = useDispatch();
const setDefaults = useCallback(response => {
const responseJob = response.data;
const advancedTemplateValues = {};
const defaultTemplateValues = {};
const inputs = responseJob.template_inputs;
const advancedInputs = responseJob.advanced_template_inputs;
if (advancedInputs) {
advancedInputs.forEach(input => {
advancedTemplateValues[input.name] = input?.default || '';
});
}
if (inputs) {
inputs.forEach(input => {
defaultTemplateValues[input.name] = input?.default || '';
});
}
setTemplateValues(defaultTemplateValues);
setAdvancedValues(currentAdvancedValues => ({

Copy link
Member

Choose a reason for hiding this comment

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

alright, then let's revisit it on one of the final PRs, so it won't cause lot of refactoring

<DescriptionField
inputs={templateInputs}
value={advancedValues.description}
setValue={newValue => setAdvancedValues({ description: newValue })}
Copy link
Member

Choose a reason for hiding this comment

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

a cool shortcut I thought about, but just a small nit

Suggested change
setValue={newValue => setAdvancedValues({ description: newValue })}
setValue={description => setAdvancedValues({ description })}

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 a good suggestion, but I'm worried that if we decide to have a const named description in the component it would create issues. (for example if we destruct advancedValues)

webpack/JobWizard/steps/AdvancedFields/DescriptionField.js Outdated Show resolved Hide resolved
webpack/JobWizard/steps/AdvancedFields/DescriptionField.js Outdated Show resolved Hide resolved
@MariaAga MariaAga force-pushed the advanced-description-fields-exp branch from e6e88c0 to e002b26 Compare July 13, 2021 11:48
webpack/JobWizard/steps/AdvancedFields/DescriptionField.js Outdated Show resolved Hide resolved
Comment on lines +22 to +44
({
data: {
advanced_template_inputs,
effective_user,
job_template: { executionTimeoutInterval, description_format },
},
}) => {
const advancedTemplateValues = {};
const advancedInputs = advanced_template_inputs;
if (advancedInputs) {
advancedInputs.forEach(input => {
advancedTemplateValues[input.name] = input?.default || '';
});
}
setAdvancedValues(currentAdvancedValues => ({
...currentAdvancedValues,
effectiveUserValue: effective_user?.value || '',
timeoutToKill: executionTimeoutInterval || '',
templateValues: advancedTemplateValues,
description: description_format || '',
}));
},
[]
Copy link
Member

Choose a reason for hiding this comment

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

I just removed the const declaration from line 30 and used advanced_template_inputs directly,

and then renamed advancedTemplateValues on line 29 to templateValues
so in line 40 you can use it directly as templateValues, instead of:
templateValues: advancedTemplateValues,

@MariaAga MariaAga force-pushed the advanced-description-fields-exp branch from e002b26 to 1487774 Compare July 20, 2021 09:37
@MariaAga MariaAga force-pushed the advanced-description-fields-exp branch from 1487774 to edf2849 Compare July 20, 2021 10:37
Copy link
Member

@Ron-Lavi Ron-Lavi left a comment

Choose a reason for hiding this comment

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

Thanks @MariaAga !
@adamruzicka can you have a final look?

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Looks good

@adamruzicka adamruzicka merged commit c24078a into theforeman:master Jul 20, 2021
@adamruzicka
Copy link
Contributor

Thank you @MariaAga & @LaViro !

adamruzicka pushed a commit that referenced this pull request Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants