Skip to content

Commit

Permalink
Bug/WP-677: Dynamic exec system bug fixes - validation and allocation…
Browse files Browse the repository at this point in the history
… list (#964)

* validationOnMount should not validate on re-render

* Remove debug info

* Remove more extraneous changes
  • Loading branch information
chandra-tacc authored Sep 3, 2024
1 parent f872d32 commit c1077ef
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 18 deletions.
29 changes: 16 additions & 13 deletions client/src/components/Applications/AppForm/AppForm.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,11 @@ export const AppSchemaForm = ({ app }) => {
defaultHost?.endsWith(s)
);
return {
allocations: getAllocationList(app, state.allocations),
allocations: getAllocationList(
app,
state.allocations,
allocationToExecSysMap
),
portalAlloc: state.allocations.portal_alloc,
jobSubmission: state.jobs.submit,
hasDefaultAllocation:
Expand Down Expand Up @@ -312,7 +316,7 @@ export const AppSchemaForm = ({ app }) => {
},
});
};

const isFirstRender = React.useRef(true);
const appFields = FormSchema(app);
const initialAllocation = getDefaultAllocation(allocations, portalAlloc);
// This additional form state setup is needed for exec system and queue system
Expand Down Expand Up @@ -386,10 +390,10 @@ export const AppSchemaForm = ({ app }) => {
} else if (!allocations.length) {
jobSubmission.error = true;
jobSubmission.response = {
message: isAppUsingDynamicExecSystem
message: isAppUsingDynamicExecSystem(app)
? `You need an allocation to run this application.`
: `You need an allocation on ${getSystemName(
app.exec_sys.host
app.execSystems[0].host
)} to run this application.`,
};
missingAllocation = true;
Expand Down Expand Up @@ -503,7 +507,7 @@ export const AppSchemaForm = ({ app }) => {
</>
)}
<Formik
validateOnMount
validateOnMount={isFirstRender.current}
initialValues={initialValues}
initialTouched={initialValues}
validationSchema={(props) => {
Expand All @@ -521,7 +525,7 @@ export const AppSchemaForm = ({ app }) => {
values.execSystemLogicalQueue
);
const currentExecSystems = formState.execSystems;
const schema = Yup.object({
return Yup.object({
parameterSet: Yup.object({
...Object.assign(
{},
Expand All @@ -537,10 +541,7 @@ export const AppSchemaForm = ({ app }) => {
name: Yup.string()
.max(64, 'Must be 64 characters or less')
.required('Required'),
execSystemId: getExecSystemIdValidation(
app,
initialValues.allocation
),
execSystemId: getExecSystemIdValidation(app),
execSystemLogicalQueue: isJobTypeBATCH(app)
? Yup.string()
.required('Required.')
Expand All @@ -565,15 +566,14 @@ export const AppSchemaForm = ({ app }) => {
'You need an allocation with access to at least one system to run this application.',
function (value) {
const isValidExecSystems =
!isAppUsingDynamicExecSystem ||
(isAppUsingDynamicExecSystem &&
!isAppUsingDynamicExecSystem(app) ||
(isAppUsingDynamicExecSystem(app) &&
currentExecSystems.length > 0);
return isValidExecSystems;
}
)
: Yup.string().notRequired(),
});
return schema;
});
}}
onSubmit={(values, { setSubmitting, resetForm }) => {
Expand Down Expand Up @@ -720,6 +720,9 @@ export const AppSchemaForm = ({ app }) => {
!hasStorageSystems ||
jobSubmission.submitting ||
(app.definition.jobType === 'BATCH' && missingAllocation);
React.useEffect(() => {
isFirstRender.current = false;
}, []);
return (
<Form>
<HandleDependentFieldChanges
Expand Down
12 changes: 11 additions & 1 deletion client/src/components/Applications/AppForm/AppForm.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,13 @@ describe('AppSchemaForm', () => {
const { getByText, container } = renderAppSchemaFormComponent(store, {
...helloWorldAppFixture,
execSystems: updatedExecSystems,
definition: {
...helloWorldAppFixture.definition,
notes: {
...helloWorldAppFixture.definition.notes,
dynamicExecSystems: ['maverick99'],
},
},
});

console.log(container.innerHTML);
Expand All @@ -460,7 +467,10 @@ describe('AppSchemaForm', () => {
const execSystemDropDown = container.querySelector(
'select[name="execSystemId"]'
);
expect(execSystemDropDown).toBeNull();
const options = execSystemDropDown.querySelectorAll(
'option:not([disabled]):not([hidden])'
);
expect(options.length).toBe(0);
});

it('does not display exec system and allocation UI for FORK jobs', async () => {
Expand Down
12 changes: 8 additions & 4 deletions client/src/components/Applications/AppForm/AppFormUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -399,16 +399,20 @@ export const isJobTypeBATCH = (app) => {
};

/**
* Build list of all allocation if using dynamic exec syste,
* Otherwise, only provides allocation list matching
* If using dynamic exec system,
* build list of all allocations with available hosts.
* Otherwise, only provide allocation list matching
* the execution host.
* @param {*} app
* @param {*} allocations
* @param {Map(any, any))} map of allocation name to list of exec systems.
* @returns List of type String
*/
export const getAllocationList = (app, allocations) => {
export const getAllocationList = (app, allocations, allocationToExecSysMap) => {
if (isAppUsingDynamicExecSystem(app)) {
return allocations.active.map((alloc) => alloc['projectName']);
return [...allocationToExecSysMap.keys()].filter(
(alloc) => allocationToExecSysMap.get(alloc).length > 0
);
}

const matchingExecutionHost = Object.keys(allocations.hosts).find(
Expand Down

0 comments on commit c1077ef

Please sign in to comment.