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 search and filter functionalities to GScan #499

Merged
merged 15 commits into from
Dec 8, 2020

Conversation

kinow
Copy link
Member

@kinow kinow commented Sep 21, 2020

These changes close #337

Improves the GScan component adding

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • No documentation update required.

@kinow kinow added this to the 0.3 milestone Sep 21, 2020
@kinow kinow self-assigned this Sep 21, 2020
@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2020

Codecov Report

Merging #499 into master will decrease coverage by 0.53%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #499      +/-   ##
==========================================
- Coverage   63.29%   62.75%   -0.54%     
==========================================
  Files          44       42       -2     
  Lines         970      964       -6     
  Branches       59       57       -2     
==========================================
- Hits          614      605       -9     
- Misses        338      340       +2     
- Partials       18       19       +1     
Flag Coverage Δ
#unittests 62.75% <50.00%> (-0.54%) ⬇️

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

Impacted Files Coverage Δ
src/components/cylc/gscan/GScan.vue 56.00% <50.00%> (-15.43%) ⬇️
...omponents/graphqlFormGenerator/components/List.vue 0.00% <0.00%> (-33.34%) ⬇️
src/components/cylc/workflow/Toolbar.vue 91.66% <0.00%> (ø)
src/components/graphqlFormGenerator/FormInput.vue 69.56% <0.00%> (ø)
src/components/cylc/Toolbar.vue
src/components/cylc/ConnectionStatus.vue

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a6931c...448af7f. Read the comment docs.

@kinow
Copy link
Member Author

kinow commented Sep 21, 2020

Search is done. I have not added debounce, nor implemented the two-step filter as in the tree view as for now we don't have very large lists of workflows. We can fine-tune it during reviews or in follow-up PR's 👍

Here's what it looks like now.

GIFrecord_2020-09-21_135705

@kinow kinow changed the title Add search (without debounce for now) to GScan Add search and filter functionalities to GScan Sep 21, 2020
@kinow
Copy link
Member Author

kinow commented Sep 21, 2020

Figuring out how to show so many states in a single tooltip 😄

image

For now I think a scrollbar will be enough for the initial version.

image

@kinow
Copy link
Member Author

kinow commented Sep 22, 2020

Tests are broken for now, but it would be better to get a pre-review of the new functionality before I spend some time fixing them.

I have not included workflow host nor Cylc version, as I think I would need to iterate the workflows and collect their hosts and versions. Not sure if there are plans to add this in GraphQL (don't think so? But we can add it later).

image

@cylc/core would anyone be willing to play and see what needs changing, what I missed, any bugs, etc, please? 😬

GIFrecord_2020-09-22_153118

@kinow
Copy link
Member Author

kinow commented Sep 22, 2020

BTW, I tried to add the magnifier icon, as in the design sketch. Unfortunately Material Design puts the icon out of the text field. There might be some way to move the magnify icon inside the text field to match the sketch, but that would probably require some further investigation (i.e. read source code of vuetify in TypeScript, and docs, more than 1/2 hours work at least).

Also tried the dark directive, to have the tooltip (that's a menu actually 🤷‍♂️, but users won't notice the difference) dark as in the sketch. The dark directive uses CSS styles from the vuetify dark theme, which is much darker. And in case we end up using light/dark from Vuetify, we could have to hack with the colors or theme. So I thought it would be simpler to have the tooltip working first, and then tweak colors, margins, fonts, styles in general 👍

But my goal was to be as close to the design sketch as possible.

@hjoliver
Copy link
Member

Looks good @kinow ; I'll try to take a closer look tomorrow. I think the other selection criteria can be added later (and will presumably require updating the GraphQL schema and the workflow data sent by the UIS).

@oliver-sanders
Copy link
Member

oliver-sanders commented Sep 22, 2020

Figuring out how to show so many states in a single tooltip 😄

The good news is that we will kill off a few of those task states pre-release.

Are you filtering for task state or job state? (there are only 5 job states).

@kinow
Copy link
Member Author

kinow commented Sep 22, 2020

Are you filtering for task state or job state? (there are only 5 job states).

workflow state, and task state. The UI displays each state in the enum TaskState. Then the filter just checks for the intersection of those states, with the states in workflow.stateTotals (I believe these are task states?).

If the intersection is not empty, the workflow stays in the UI.

@oliver-sanders
Copy link
Member

oliver-sanders commented Sep 23, 2020

states in workflow.stateTotals (I believe these are task states?).

Yep those are task states, however job states are just a subset of task states:

  • submitted
  • running
  • succeeded
  • failed
  • submit-failed

(should probably stick that in an enum along with task states at some point)

Because GScan is displaying jobs (squares) it would make more sense to filter by job states rather than task states (circles).

Since SoD I think task states will always reflect the most recent job state (as cylc reset was retired) so we can get away with using task states for now.

I think this may need a rethink at some point in the future, we have talked about displaying tasks rather than jobs in GScan too.

@hjoliver
Copy link
Member

@kinow
Copy link
Member Author

kinow commented Sep 29, 2020

Rebased, resolved conflicts, and tested again. Ready for a pre-review. If the UI elements, and the functionality are OK, I will add the unit/e2e tests, and then it should be ready for the final review 👍

@oliver-sanders
Copy link
Member

Looks good, only found one niggle which is that stopped workflows don't get filtered out when filtering by task state.

Screenshot 2020-09-29 at 09 37 13

@kinow
Copy link
Member Author

kinow commented Oct 1, 2020

Looks good, only found one niggle which is that stopped workflows don't get filtered out when filtering by task state.

@oliver-sanders ah, good catch! I was intentionally applying task states to only running workflows. Removed that if statement in the latest commit. When you first initialize Cylc UI and Cylc 8, you probably won't see any stopped workflows.

That's because GraphQL workflows.stateTotals will return empty. But if you start, then stop a workflow, it should work as expected I think 👍

image

@kinow
Copy link
Member Author

kinow commented Oct 6, 2020

@hjoliver if you have time later, would you mind taking a look at the way the WIP search/filter are working here, please? If you find no issues with how it works, I will add the test/changelog and mark as ready for review :) Thanks!

@hjoliver
Copy link
Member

hjoliver commented Oct 6, 2020

I'm not seeing stopped flows on this branch, except briefly if I switch away and come back again. 🤔

foo

@kinow
Copy link
Member Author

kinow commented Oct 7, 2020

I'm not seeing stopped flows on this branch, except briefly if I switch away and come back again. thinking

There's an explanation for that. When you come back, the UI takes some ms/seconds to re-apply the filter. The filter is using the task states. By default it shows every workflow that has any task state. Offline/stopped workflows won't have any states, unless you have executed them at least once since the UIS was started.

Thinking about it, I am keen to leave that filter disabled by default, and show everything, every workflow regardless of the task state. I think that makes more sense. If you want to limit by task state, then you have to choose the states and filter.

@hjoliver
Copy link
Member

hjoliver commented Oct 7, 2020

Ah, makes sense.

Thinking about it, I am keen to leave that filter disabled by default, and show everything, every workflow regardless of the task state. I think that makes more sense. If you want to limit by task state, then you have to choose the states and filter.

Agreed.

Offline/stopped workflows won't have any states, unless you have executed them at least once since the UIS was started.

This might change once the UIS (possibly via cylc scan in the stopped flow case) can read flow DBs. (But, that's not the problem of this PR).

@hjoliver
Copy link
Member

hjoliver commented Oct 7, 2020

With so many task states (albeit fewer of them by the time we release 8.0) can we have a select-all/deselect-all option?

@kinow
Copy link
Member Author

kinow commented Oct 7, 2020

With so many task states (albeit fewer of them by the time we release 8.0) can we have a select-all/deselect-all option?

By coincidence, I am working on this at this right moment! 😬 Just trying to align the checkbox next to the filter name, then will push the new commits.

@codecov-io
Copy link

codecov-io commented Oct 7, 2020

Codecov Report

Merging #499 (87b7c05) into master (4366457) will increase coverage by 0.13%.
The diff coverage is 89.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #499      +/-   ##
==========================================
+ Coverage   83.92%   84.06%   +0.13%     
==========================================
  Files          58       58              
  Lines        1045     1073      +28     
  Branches       74       77       +3     
==========================================
+ Hits          877      902      +25     
- Misses        146      149       +3     
  Partials       22       22              
Flag Coverage Δ
e2e 54.54% <89.65%> (+0.83%) ⬆️
unittests 76.74% <75.86%> (-0.06%) ⬇️

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

Impacted Files Coverage Δ
src/graphql/queries.js 100.00% <ø> (ø)
src/services/mock/checkpoint.js 100.00% <ø> (ø)
src/components/cylc/gscan/GScan.vue 78.57% <89.65%> (+10.71%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4366457...87b7c05. Read the comment docs.

</v-list>
</v-card>
</v-menu>
</div>
Copy link
Member Author

Choose a reason for hiding this comment

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

These filters are not looking really nice here and in the Tree component or view. Have added a post-it to take a better look, see if we can unify or find a better way to organize them. If so, will create a follow-up issue 👍

/**
* The filtered workflows. This is the result of applying the filters
* on the workflows prop.
* @type {[
Copy link
Member Author

Choose a reason for hiding this comment

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

This is GH UI. Valid JS & jsdoc.

@kinow
Copy link
Member Author

kinow commented Oct 7, 2020

Done @hjoliver

GIFrecord_2020-10-07_152939

Tested locally. It looked intuitive to me.

First implementation was selecting all only if none was selected. That was a bit strange to use.

Also tried using a checkbox to the right of the text (i.e. "workflow states" [ ]). But aligning that v-checkbox component is annoyingly difficult, because of the way Vuetify creates multiple elements with so many classes that it's hard to guess where the extra padding or margin is coming from.

Then realized that it could just be another checkbox, aligned with the other check boxes. It became "easier to read"? Not sure if that makes sense, but I found the final product good enough 😬

Could you take another look, please, @hjoliver ?

Thanks

@hjoliver
Copy link
Member

hjoliver commented Oct 7, 2020

Looks good in your gif - I'll try it myself later, after getting the xtrigger PR tests workings....

@kinow
Copy link
Member Author

kinow commented Nov 8, 2020

Not sure if Codecov hasn't updated, but it's showing in its UI +0.14% (unit+e2e combined): https://app.codecov.io/gh/cylc/cylc-ui/compare/499

@kinow
Copy link
Member Author

kinow commented Nov 26, 2020

Rebased, conflicts fixed.

@kinow
Copy link
Member Author

kinow commented Nov 26, 2020

This branch has a problem sorting workflows. The problem has been fixed on #537 , I've added the fix here, but would have to add the updated tests too, so reverted the change.

When reviewing, take a look at the search and filter functionality only 👍 unless that PR is merged first and this is rebased.

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

🎆

Search and filter look good to me.

@kinow
Copy link
Member Author

kinow commented Dec 7, 2020

Rebased fixing conflicts, but now unit tests failing 😥

@kinow
Copy link
Member Author

kinow commented Dec 8, 2020

Tests fixed. Did a quick manual testing, everything looks to be in order 👍

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Nice, looks great @kinow 👍 tested as working well.

@hjoliver hjoliver merged commit d884bb2 into cylc:master Dec 8, 2020
@kinow kinow deleted the search-filter-gscan branch December 8, 2020 01:18
@kinow
Copy link
Member Author

kinow commented Dec 8, 2020

Thanks @hjoliver !!! 🍾

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.

GScan: search and filter
5 participants