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

Add support for filtering and pagination on job output #9208

Merged

Conversation

mabashian
Copy link
Member

SUMMARY

link #6612
link #5906

This PR adds the ability to filter job events and also includes logic to handle fetching filtered job events across different pages.

Note that the verbosity dropdown included in #5906 is not included in this work. I don't think that's possible without api changes.

As part of this work, I converted JobOutput.jsx from a class based component to a functional component. I've tried my best to make sure that all existing functionality has remained the same by comparing the experience of this branch to devel.

Like the old UI, the output filter is disabled while the job is running.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • UI

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

let pageSize;

if (startIndex === stopIndex) {
page = startIndex;
Copy link
Contributor

@jakemcdermott jakemcdermott Feb 2, 2021

Choose a reason for hiding this comment

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

I think there's an off-by-one here.

Suggested change
page = startIndex;
page = startIndex + 1;

A reproducer for the bug is when startIndex = 50 and stopIndex = 50:

Screenshot from 2021-02-02 15-20-50

@jakemcdermott
Copy link
Contributor

jakemcdermott commented Feb 5, 2021

@mabashian
Based on what I can see from my testing, this PR resolves the issue with job results not updating until finished, (e.g: #9112).

It would be good to make a note of it in the changelog

Comment on lines 655 to 710
if (hasContentLoading) {
return <ContentLoading />;
}
Copy link
Member

Choose a reason for hiding this comment

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

To reduce "flashing" during search, we could move <ContentLoading/> further down into the Job Output.

<AutoSizer nonce={window.NONCE_ID} onResize={handleResize}>
  {({ width, height }) => {
    return (
      <>
      {hasContentLoading ? (
        <div style={{ width }}>
          <ContentLoading />
        </div>
      ) : (
        <List
          ref={ref => {
            registerChild(ref);
            listRef.current = ref;
          }}
 ...

output

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@mabashian
Copy link
Member Author

@jakemcdermott @marshmalien OK, I think I've got this one rebased and ready to go with y'alls suggested changes.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@jakemcdermott
Copy link
Contributor

@mabashian lgtm 👍 note: There's another merge conflict from a recent pr

Copy link
Member

@marshmalien marshmalien left a comment

Choose a reason for hiding this comment

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

This is awesome. Thanks for refactoring the JobOutput into a functional component - that work will go a long way.

@unlikelyzero unlikelyzero added state:needs_test type:feature prioritized on a feature board labels Feb 12, 2021
@unlikelyzero
Copy link

unlikelyzero commented Feb 14, 2021

Note: this is our highest priority this week.

@mabashian can you rebase?

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@one-t one-t self-assigned this Feb 22, 2021
@one-t
Copy link
Contributor

one-t commented Feb 23, 2021

Hey, folks- found this issue while I was poking around:

If you provide the wrong type of input for a filter you will have to navigate out and back to the job details page to start over

  • Steps:
    • Open a project update
    • do an advanced search, with project_update as the key
    • provide a non-integer value (a)
    • image

@one-t one-t assigned akus062381 and unassigned one-t Feb 23, 2021
@dimovstanimir
Copy link

@unlikelyzero do you think that this feature could be ready and included in newest AWX release?

@unlikelyzero
Copy link

@mabashian "while you're in there" can you update the locators identified in #9495

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@mabashian
Copy link
Member Author

In reference to #9495, I've added a commit to add ouiaId's to the following things:

Relaunch (Dropdown version which allows users to select all or just failed hosts):

Screen Shot 2021-03-18 at 2 19 59 PM

Relaunch (Regular button version):

Screen Shot 2021-03-18 at 2 21 21 PM

Download:

Screen Shot 2021-03-18 at 2 22 01 PM

Delete:

Screen Shot 2021-03-18 at 2 22 56 PM

Cancel:

Screen Shot 2021-03-18 at 2 23 36 PM

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@unlikelyzero
Copy link

unlikelyzero commented Mar 19, 2021

Findings

  • Host Details Standard Out and Standard Error always appear empty (User error?)
    Screen Shot 2021-03-19 at 8 59 51 AM

  • I don't appear to be able to filter on 'Event (event) Host OK'. Host Unreachable works

  • Cancel Job button appears after JT completes (regression)

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@unlikelyzero
Copy link

@mabashian any chance you could add data-ouia-ids on the page controls?

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 7a512c1 into ansible:devel Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:ui type:feature prioritized on a feature board
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants