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

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

Conversation

asimregmi
Copy link
Contributor

Overview

The view details in Job History currently shows input parameters that have isHidden: true properties. This code fixes that issue as well as adds few more jobAttributes to the Output section.

Related

Changes

  • added logic to filter isHidden: true properties
  • added few more jobAttributes to render

Testing

  1. Go to History > Jobs
  2. Click on 'View Details' under Job Details and check to see if the hidden parameters are shown or not under Inputs
  3. Verify if there are two more attributes in Outputs

UI

After filtering out hidden properties

image

Notes

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #868 (f2b97be) into main (fa8c979) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #868      +/-   ##
==========================================
+ Coverage   63.33%   63.34%   +0.01%     
==========================================
  Files         428      428              
  Lines       12246    12250       +4     
  Branches     2518     2521       +3     
==========================================
+ Hits         7756     7760       +4     
  Misses       4284     4284              
  Partials      206      206              
Flag Coverage Δ
javascript 69.71% <100.00%> (+0.01%) ⬆️
unittests 56.99% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...omponents/History/HistoryViews/JobHistoryModal.jsx 82.35% <ø> (ø)
...ient/src/redux/sagas/fixtures/appdetail.fixture.js 100.00% <ø> (ø)
...ient/src/redux/sagas/fixtures/jobdetail.fixture.js 100.00% <ø> (ø)
client/src/utils/jobsUtil.js 94.20% <100.00%> (+0.35%) ⬆️

@asimregmi asimregmi marked this pull request as ready for review October 2, 2023 13:52
Copy link
Contributor

@shayanaijaz shayanaijaz left a comment

Choose a reason for hiding this comment

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

Added some comments regarding structure of the code

client/src/utils/jobsUtil.js Show resolved Hide resolved
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.

@@ -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

@asimregmi asimregmi requested a review from shayanaijaz October 6, 2023 15:02
Copy link
Contributor

@shayanaijaz shayanaijaz left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@chandra-tacc chandra-tacc left a comment

Choose a reason for hiding this comment

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

LGTM.
Can you please add unit tests to add coverage to the notes is hidden for parameters and file inputs? And missing tests for scenarios related to '_', and other conditional checks.
Lets try to add unit tests going forward. I'll do the same on my PRs.

@asimregmi
Copy link
Contributor Author

Added additional test suites for this

Copy link
Collaborator

@chandra-tacc chandra-tacc left a comment

Choose a reason for hiding this comment

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

LGTM!

@chandra-tacc chandra-tacc merged commit 31af29e into main Oct 24, 2023
6 checks passed
@chandra-tacc chandra-tacc deleted the task/WP-100--display-all-jobAttributes-via-getJobDisplayInformation branch October 24, 2023 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants