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

Jump to bottom

Implement MetaFilter with separate field and value #225

Merged
merged 1 commit into from
Dec 7, 2021
Merged

Implement MetaFilter with separate field and value #225

merged 1 commit into from
Dec 7, 2021

Conversation

mshriver
Copy link
Contributor

@mshriver mshriver commented Oct 12, 2021 •
edited
Loading

Alternative to #221 that is more generic. Similar to the filters used on
the run/results page, and could be an abstract replacement for their
current filters.

TODO

  • complete functions for field/value clear
  • complete function for value select
  • add checkbox to apply not operator (eq and in will be inferred) Extend MetaFilter and FilterTable Integration #230
  • handling of null value metadata (key only) :: Specific list of metadata fields that can be filtered has been added. Outside of this PR scope to further abstract this.
  • handling of non-string metadata, dates, etc. :: Specific list of metadata fields that can be filtered has been added. Outside of this PR scope to further abstract this.
  • Rebase to account for Auth

Preview

Nothing selected

image

Field selected with no values available

image

Field selected with values available/selected

image

@mshriver mshriver force-pushed the classify-filters-generic-filter branch from 8b3b6c5 to 6443eac Compare October 12, 2021 21:07
@mshriver mshriver force-pushed the classify-filters-generic-filter branch from 6443eac to b8986f8 Compare October 13, 2021 12:19
@mshriver mshriver force-pushed the classify-filters-generic-filter branch from b8986f8 to 464aef4 Compare October 13, 2021 20:43
@mshriver
Copy link
Contributor Author

@john-dupuy @rsnyman @jjaquish At this point, I think all the basics are working for this filter implementation on classify failures.

I'd love to hear your thoughts on the patterns I've used here, the class/function structure, everything. If it doesn't fall flat on its face, we can lay plans to build on this MetaFilter class for use on all FilterTable pages.

Thanks for your feedback and review time! I appreciate whatever advice you have here.

@mshriver mshriver force-pushed the classify-filters-generic-filter branch from 464aef4 to de31fc2 Compare October 13, 2021 20:46
@john-dupuy
Copy link
Contributor

When clearing the value selection via the "x" in the select, I noticed it leaves the <fieldOption> eq part, making the table return no values.

@mshriver mshriver marked this pull request as draft October 14, 2021 15:35
@mshriver
Copy link
Contributor Author

Thanks @john-dupuy! I should be able to work through those issues today.

@mshriver mshriver force-pushed the classify-filters-generic-filter branch 4 times, most recently from b8037a1 to 68494eb Compare October 14, 2021 20:27
@mshriver
Copy link
Contributor Author

@john-dupuy All comments should be addressed/fixed/updated now.

I've added a utility for the small function to convert from UI filter object(s) to an API filter string, and changed to a TypeAheadMutli select type with I think provides a better display of selected items.

@mshriver mshriver marked this pull request as ready for review October 14, 2021 20:35
@mshriver mshriver mentioned this pull request Oct 15, 2021
Closed
@mshriver mshriver force-pushed the classify-filters-generic-filter branch from 68494eb to a274ab1 Compare October 19, 2021 12:04
@mshriver
Copy link
Contributor Author

mshriver commented Oct 19, 2021 •
edited
Loading

@jjaquish @rsnyman I think I'm ready for a second review for the scope of this PR.

I definitely want to take this functionality further in a few directions, but likely not in the scope of this PR.

  1. Filtering for at least metadata.statuses fields - the values load, but are an array with two values, and the filtering itself doesn't work.
  2. Expand MetaFilter functionality with the filtering functions that are repeated on FilterTable based components. Hook FilterTable filter updating into MetaFilter directly so the two can be cleanly used together on run/results pages as well.
  3. Add an inversion checkbox to control the operator for not in and not equals
  4. Value filter field counts don't take into account active filters on the FilterTable, since MetaFilter is a bit disconnected from the table itself. This should be far easier to address when # 2 above is implemented.

john-dupuy
john-dupuy approved these changes Oct 21, 2021
View reviewed changes
Copy link
Contributor

@john-dupuy john-dupuy left a comment

Choose a reason for hiding this comment

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

I noticed when you're including the skips/xfails the value counts are only updated when you've cleared the selected field. If you leave a field selected and then hit the checkbox, the value counts are not updated.

But I think we can leave this for later. LGTM - nice work on this!

This was referenced Oct 21, 2021
Open
Open
@mshriver
Copy link
Contributor Author

@john-dupuy thanks for your diligence in testing - I think the value count behavior tied to skip/xfail is the same root cause as already applied filters not impacting value count. Definitely something that should be addressed with continued improvement of this component, better event callbacks and filter state management between FilterTable and MetaFilter.

@mshriver mshriver mentioned this pull request Oct 21, 2021
Open
@mshriver mshriver force-pushed the classify-filters-generic-filter branch from a274ab1 to f6f5997 Compare October 21, 2021 18:06
@rsnyman
Copy link
Contributor

rsnyman commented Oct 21, 2021

Looks good to me. I think John caught most of the issues.

rsnyman
rsnyman requested changes Nov 15, 2021
View reviewed changes
Copy link
Contributor

@rsnyman rsnyman left a comment

Choose a reason for hiding this comment

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

Now that the Auth PR has been merged, there's an updated way to get data from the API, which I've given an example of in a comment. Also, you're going to need to rebase and fix some conflicts, it seems. Once that's done, we're good to go.

frontend/src/components/filtertable.js Outdated Show resolved Hide resolved
@mshriver mshriver force-pushed the classify-filters-generic-filter branch from f6f5997 to 520d675 Compare December 3, 2021 14:39
@mshriver
Implement MetaFilter with separate field and value
1b9c0ac 
Add a metadata filtering class for use on filtertable result pages.
Adding to classify failures first, where its needed most right now.

This could be applied to run and results views as well, replacing their
current filtering selections.

Add some lines to .gitignore for vscode and venv
@mshriver mshriver force-pushed the classify-filters-generic-filter branch from 520d675 to 1b9c0ac Compare December 6, 2021 14:22
@mshriver
Copy link
Contributor Author

mshriver commented Dec 6, 2021

@rsnyman @john-dupuy this has now been rebased with the auth changes, local testing is consistent with before rebase.

@john-dupuy john-dupuy merged commit ea332f2 into ibutsu:master Dec 7, 2021
rsnyman added a commit to rsnyman/ibutsu-server that referenced this pull request Jan 18, 2022
@rsnyman
Release 2.0.0\n\n* Estimate count on test-history page
3183ed2 
* Small improvements to the test history tab
* Hide username/password related components when user login is disabled (ibutsu#279)
* Add test history tab to Result page (ibutsu#276)
* Address some issues raised by static analysis (ibutsu#278)
* Convert USER_LOGIN_ENABLED to bool if given as env var
* Get a user's primary e-mail on GitHub if they have no public e-mail address (ibutsu#272)
* Add a way to disabled basic auth for non-superadmins
* Add get_user_list endpoint
* Fix keycloak login (ibutsu#270)
* Update docker image names (ibutsu#267)
* Show login progress feedback (ibutsu#266)
* Add build_deploy script for App-SRE builds (ibutsu#265)
* A single jUnit XML file is a single test run, refactor the importer to take this into account
* Some code refactoring after static analysis. (ibutsu#245)
* Implement MetaFilter with separate field and value (ibutsu#225)
* Adjustments for app-sre deployment (ibutsu#256)
* Fix small FE bug on user profile page
* Add superadmin user after upgrading db
* Add a task for adding users/project owners
* Add superadmin to the ocp template files
* Add ability to create superadmin user on startup
* Allow logging in by hitting 'Enter'
* Require superadmin token for running admin task
* Update pods script to create an admin user, a project, standardise on echo, and make the output prettier. (ibutsu#247)
* Some small fixes to ENV vars
* Split templates into one file for each
* Update @greatsumini/react-facebook-login
* Fix Jenkins Job View
* Allow superadmins to update projects
* Support adding users to projects
* Add project info to the profile page
* Filter runs/results on user projects, if none specified
* Bump url-parse from 1.5.1 to 1.5.3 in /frontend (ibutsu#234)
* Bump tmpl from 1.0.4 to 1.0.5 in /frontend (ibutsu#233)
* Bump tar from 6.1.0 to 6.1.11 in /frontend (ibutsu#232)
* Add authentication and authorisation to Ibutsu
* Promote user property data to metadata
* Add babel core dependency
* Switch from 'babel-eslint' to '@babel/eslint-parser'
* Update and re-apply pre-commit (ibutsu#216)
* Support adding artifacts to test runs (ibutsu#215)
@rsnyman rsnyman mentioned this pull request Jan 18, 2022
Merged
jjaquish pushed a commit to jjaquish/ibutsu-server that referenced this pull request Aug 9, 2022
@mshriver @jjaquish
Implement MetaFilter with separate field and value (ibutsu#225)
a2664b8 
Add a metadata filtering class for use on filtertable result pages.
Adding to classify failures first, where its needed most right now.

This could be applied to run and results views as well, replacing their
current filtering selections.

Add some lines to .gitignore for vscode and venv
jjaquish pushed a commit to jjaquish/ibutsu-server that referenced this pull request Aug 9, 2022
@rsnyman @jjaquish
Release 2.0.0\n\n* Estimate count on test-history page
1afda61 
* Small improvements to the test history tab
* Hide username/password related components when user login is disabled (ibutsu#279)
* Add test history tab to Result page (ibutsu#276)
* Address some issues raised by static analysis (ibutsu#278)
* Convert USER_LOGIN_ENABLED to bool if given as env var
* Get a user's primary e-mail on GitHub if they have no public e-mail address (ibutsu#272)
* Add a way to disabled basic auth for non-superadmins
* Add get_user_list endpoint
* Fix keycloak login (ibutsu#270)
* Update docker image names (ibutsu#267)
* Show login progress feedback (ibutsu#266)
* Add build_deploy script for App-SRE builds (ibutsu#265)
* A single jUnit XML file is a single test run, refactor the importer to take this into account
* Some code refactoring after static analysis. (ibutsu#245)
* Implement MetaFilter with separate field and value (ibutsu#225)
* Adjustments for app-sre deployment (ibutsu#256)
* Fix small FE bug on user profile page
* Add superadmin user after upgrading db
* Add a task for adding users/project owners
* Add superadmin to the ocp template files
* Add ability to create superadmin user on startup
* Allow logging in by hitting 'Enter'
* Require superadmin token for running admin task
* Update pods script to create an admin user, a project, standardise on echo, and make the output prettier. (ibutsu#247)
* Some small fixes to ENV vars
* Split templates into one file for each
* Update @greatsumini/react-facebook-login
* Fix Jenkins Job View
* Allow superadmins to update projects
* Support adding users to projects
* Add project info to the profile page
* Filter runs/results on user projects, if none specified
* Bump url-parse from 1.5.1 to 1.5.3 in /frontend (ibutsu#234)
* Bump tmpl from 1.0.4 to 1.0.5 in /frontend (ibutsu#233)
* Bump tar from 6.1.0 to 6.1.11 in /frontend (ibutsu#232)
* Add authentication and authorisation to Ibutsu
* Promote user property data to metadata
* Add babel core dependency
* Switch from 'babel-eslint' to '@babel/eslint-parser'
* Update and re-apply pre-commit (ibutsu#216)
* Support adding artifacts to test runs (ibutsu#215)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsnyman rsnyman rsnyman approved these changes

@john-dupuy john-dupuy john-dupuy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mshriver @john-dupuy @rsnyman