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.

Sign up for GitHub

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

task/WP-100: Display all jobAttributes via getJobDisplayInformation #868

Merged
merged 5 commits into from
Oct 24, 2023
Merged

task/WP-100: Display all jobAttributes via getJobDisplayInformation #868

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cce5f20
filtered out hidden inputs
asimregmi Oct 2, 2023
9675034
added more jobAttributes
asimregmi Oct 2, 2023
f76fa7f
added a new function filterHiddenObjects to combine filters
asimregmi Oct 6, 2023
c7bfc84
added test suites for filter input and parameters
asimregmi Oct 23, 2023
f2b97be
Merge branch 'main' into task/WP-100--display-all-jobAttributes-via-g…
chandra-tacc Oct 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions client/src/components/History/HistoryViews/JobHistoryModal.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ function JobHistoryContent({
const outputDataObj = {
'Job Name': jobName,
'Output Location': outputLocation,
'Archive System': jobDetails.archiveSystemId,
'Archive Directory': jobDetails.archiveSystemDir,
};

const resubmitJob = () => {
Expand Down
33 changes: 31 additions & 2 deletions client/src/utils/jobsUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { object } from 'prop-types';
Copy link
Contributor

Choose a reason for hiding this comment

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

can remove this unused import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this unused import

import { getSystemName } from './systems';

const TERMINAL_STATES = [`FINISHED`, `CANCELLED`, `FAILED`];
Expand Down Expand Up @@ -59,9 +60,13 @@ export function getAllocatonFromDirective(directive) {
* Get display values from job, app and execution system info
*/
export function getJobDisplayInformation(job, app) {
const fileInputs = JSON.parse(job.fileInputs);
const fileInputs = JSON.parse(job.fileInputs).filter((obj) =>
obj.notes != null ? !JSON.parse(obj.notes).isHidden : true
);
const parameterSet = JSON.parse(job.parameterSet);
const parameters = parameterSet.appArgs;
const parameters = parameterSet.appArgs.filter((obj) =>
obj.notes != null ? !JSON.parse(obj.notes).isHidden : true
);
const envVariables = parameterSet.envVariables;
const schedulerOptions = parameterSet.schedulerOptions;
const display = {
Expand Down Expand Up @@ -104,6 +109,13 @@ export function getJobDisplayInformation(job, app) {
// a webhookUrl will be a required input for interactive jobs,
// but we want to hide that input

// display.parameters.filter((input) => {
// const matchingParameter = app.definition.inputs.find((obj => {
// return input.id === obj.id;
// }));
// console.log(matchingParameter)
// });

// filter non-visible
// display.inputs.filter((input) => {
// const matchingParameter = app.definition.inputs.find((obj) => {
Expand All @@ -124,6 +136,22 @@ export function getJobDisplayInformation(job, app) {
// return true;
// });

// Note: Code below also filters v3 parameters by cross referenceing but utilizes more resources
// The quicker workaround solution is implemented in Line 63 and 65 and utilizes Array.filter method
/*
display.parameters.filter((input) => {
const matchingParameter = app.definition.jobAttributes.parameterSet.appArgs.find((obj) => {
return input.label === obj.name
});

console.log("matching", matchingParameter)
if (matchingParameter && matchingParameter.notes.isHidden) {
display.parameters.splice(display.parameters.findIndex(item => item.label))
}
return true
})
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

Are the concerns/suggestions in the comments covered by this PR? If so, they can likely be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the comments Line 139 - 153 since the initial workaround to filter the parameters worked by finding the matchingParameter first and checking to see if it has a isHidden: true property on the object. My solution (Line 63-69) just maps through the parameterSet.appArgs object array and checks to see if the property isHidden. (it doesn't need to find the matchingParameter unlike previous implementation). I wasn't sure of why it was implemented that way so I left it into a comment so I can come back to it if there was a specific design choice and need to implement it this way. The comment Line 104-137 was from before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed these comments.

const workPath = envVariables.find(
(env) => env.key === '_tapisJobWorkingDir'
);
Expand All @@ -148,6 +176,7 @@ export function getJobDisplayInformation(job, app) {
// ignore if there is problem using the app definition to improve display
}
}

return display;
}

Expand Down