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

WX-915 Restore Job Lists Page #780

Merged
merged 37 commits into from
Mar 23, 2023
Merged

WX-915 Restore Job Lists Page #780

merged 37 commits into from
Mar 23, 2023

Conversation

JVThomas
Copy link
Collaborator

@JVThomas JVThomas commented Mar 13, 2023

Addresses WX-915

PR restores the job lists page by restoring the Job Lists Component to the UI routing as well as providing missing variable arguments for sub-components. PR also updates backend encoding/decoding logic to account for changes to how str and bytes are treated in Python 3.

In Python 2, str was used for both text and bytes, making str data ambiguous. So for Python 2, assigning either text or bytes to a str variable for a JSON response would resolve peacefully. In Python 3 bytes are distinct from str, so passing bytes as part of a JSON response will now throw an error.

In terms of updates, byte tokens generated for front-end pagination now need to be decoded into strings before being sent off as part of a JSON response. For pagination queries, incoming str tokens need to be encoded as bytes before they can be processed in later steps.

To test it out, you'll should follow the steps outlined in #779 for a mock Terra setup since there's enough dev data to test the pagination flow, but feel free to use a local Cromwell instance if you have enough data saved locally.

NOTE: You'll also need to run docker compose build due to updates to jm_util methods.

…of component ngInit, updated tsconfig settings for test runners, still working on silent refresh
@JVThomas JVThomas changed the base branch from master to wx-723 March 13, 2023 12:08
@JVThomas JVThomas marked this pull request as ready for review March 14, 2023 12:03
@JVThomas JVThomas changed the title WX-915 WX-915 Restore Job Lists Page Mar 14, 2023
@jgainerdewar
Copy link
Collaborator

Oh man, I'm embarrassed if this single issue was the thing preventing it from working. 😂

We commented some tests out as part of disabling this page, so we should un-comment them and ensure they pass. We'll also want to:

  • Fully revert BT-758 404 for job list #757
  • Restore the button on the job detail page that links to the job list. It was removed here. (See also the change in that PR to the job-details.component template)

@JVThomas
Copy link
Collaborator Author

Oh man, I'm embarrassed if this single issue was the thing preventing it from working. 😂

We commented some tests out as part of disabling this page, so we should un-comment them and ensure they pass. We'll also want to:

  • Fully revert BT-758 404 for job list #757
  • Restore the button on the job detail page that links to the job list. It was removed here. (See also the change in that PR to the job-details.component template)

Got it, but I don't think I need to do a full reversion of BT-758. The PageNotFound component is nice to have for invalid, non-root paths.

@jgainerdewar
Copy link
Collaborator

Can you rebase this on master? That may happen automatically if wx-723 is deleted.

@jgainerdewar
Copy link
Collaborator

I see a few weird styling things, and a few functional things that I'm fairly certain worked before.

Misaligned arrows:
Screen Shot 2023-03-20 at 3 19 43 PM

This badge showing the status we're filtering on has a weird doubled effect. Also, clicking on the X does not remove it (though clicking the small X on the far right does).
Screen Shot 2023-03-20 at 3 18 48 PM

This gear is supposed to display a menu that enables users to configure the columns displayed in the table. I think the contents of this popup should be a list of columns? There's a chance this is a config issue on my side.
Screen Shot 2023-03-20 at 3 23 30 PM

Still poking around but wanted to get this stuff out.

@JVThomas JVThomas changed the base branch from wx-723 to master March 21, 2023 12:28
@JVThomas
Copy link
Collaborator Author

JVThomas commented Mar 21, 2023

I see a few weird styling things, and a few functional things that I'm fairly certain worked before.

Misaligned arrows: Screen Shot 2023-03-20 at 3 19 43 PM

This badge showing the status we're filtering on has a weird doubled effect. Also, clicking on the X does not remove it (though clicking the small X on the far right does). Screen Shot 2023-03-20 at 3 18 48 PM

This gear is supposed to display a menu that enables users to configure the columns displayed in the table. I think the contents of this popup should be a list of columns? There's a chance this is a config issue on my side. Screen Shot 2023-03-20 at 3 23 30 PM

Still poking around but wanted to get this stuff out.

The tag not closing and the missing column list are both bugs. The x on the far right of the tags seems intentional. This is a coded button that, on click, clears all tags.

I've update the PR to handle the bug fixes and associated test.

The misaligned arrows have been fixed with css updates

@jgainerdewar
Copy link
Collaborator

Looking good! I tested the filtering and pagination and they seem to be working correctly. Few more small UI things I noticed:

  • The badges for status filtering no longer look doubled but still have a gray outline on the right and left
  • The dropdown for selecting page size isn't styled as a dropdown, you have to click on the number to tell it's interactable

Couple things that I don't know are regressions but want to point out just in case:

  • When I add a comment to a run, I need to refresh the page before my update is reflected in the UI.
  • The Set Metadata button appears when I check off workflows but is grayed out. This corresponds to the bulk-edit component. Have you found a way to test that bulk editing works?

@JVThomas
Copy link
Collaborator Author

JVThomas commented Mar 21, 2023

Looking good! I tested the filtering and pagination and they seem to be working correctly. Few more small UI things I noticed:

  • The badges for status filtering no longer look doubled but still have a gray outline on the right and left
  • The dropdown for selecting page size isn't styled as a dropdown, you have to click on the number to tell it's interactable

Couple things that I don't know are regressions but want to point out just in case:

  • When I add a comment to a run, I need to refresh the page before my update is reflected in the UI.
  • The Set Metadata button appears when I check off workflows but is grayed out. This corresponds to the bulk-edit component. Have you found a way to test that bulk editing works?

The first two might be CSS issues or it could be a third party library. Either way I'll try to fix them with a CSS update

The comment issue seems like a reques/ response issue, looking into it as well

The metadata being greyed out is interesting because it's visible for me
Screen Shot 2023-03-21 at 12 09 51 PM

In your capabilities_config.json file, each of the displayField objects can have an optional key bulkEditable. If any one of them has it set to true the button should be enabled

That said, I can assume that bulk upload works like the the comment update? If so I'll test that out locally and see what's up

@jgainerdewar
Copy link
Collaborator

In your capabilities_config.json file, each of the displayField objects can have an optional key bulkEditable. If any one of them has it set to true the button should be enabled

Nice, this worked. It does work like the individual field editing, I need to reload the page in order to see the update.

@JVThomas
Copy link
Collaborator Author

JVThomas commented Mar 22, 2023

I've updated the table size css and provided a front-end solution for the label update bug. For bug context, the app always re-queries jobs for the table after a label updates exactly like it would on initialization. So for default table values, it'll query about 4 pages worth of jobs.

The interesting thing is that the queried jobs don't have the updated label values in the response even though the request occurs after the label update succeeds. I haven't figured out why, but in terms of the front-end I can assume that if the label updates went through then I can manually apply the update to the queried jobs.

As for the weird box-shadow on the filter tags, I'm still working on it (I didn't see it as high of a priority as the above mentioned issues) but I don't see it as a blocker if it isn't resolved.

Copy link
Collaborator

@jgainerdewar jgainerdewar left a comment

Choose a reason for hiding this comment

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

I agree that the box-shadow on the filter tags isn't a blocker. 👍

Thank you so much for all your work on this, feels so good to get this page working again!

@JVThomas JVThomas merged commit 54ab6b4 into master Mar 23, 2023
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