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

Automate-3144 API compliance reports node filters #3664

Merged
merged 3 commits into from
May 15, 2020
Merged

Conversation

tarablack01
Copy link
Contributor

πŸ”© Description: What code changed, and why?

When I looked into this, I realized the UI is doing the status filtering (https://github.com/chef/automate/blob/master/components/automate-ui/src/app/pages/+compliance/+reporting/+reporting-nodes/reporting-nodes.component.ts#L64).

Filtering by status is supported by backend, and won't result in results being spread across pages, so let's just move to status filtering via api.

⛓️ Related Resources

https://app.zenhub.com/workspaces/ui-team-5cba23a3b14fdc05970c8f87/issues/chef/automate/3144

πŸ‘ Definition of Done

Filtering by status is filtering via api.

πŸ‘Ÿ How to Build and Test the Change

hab studio enter
export DEVPROXY_URL=localhost
build components/automate-ui-devproxy
start_automate_ui_background
start_all_services
chef_load_nodes

βœ… Checklist

πŸ“· Screenshots, if applicable

Screen Shot 2020-05-13 at 11 41 20 AM
Screen Shot 2020-05-13 at 11 41 35 AM

Copy link

@vjeffrey vjeffrey left a comment

Choose a reason for hiding this comment

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

Screen Shot 2020-05-13 at 12 15 18

it looks like the status filter is being applied to the summary api call as well as the nodes api call - meaning the total counts and overall status are affected by the filter on the table list. i'd like ux to review to see if this is desired behavior or if the filter should only be applied to the nodes api call/only affect the list items

@tarablack01
Copy link
Contributor Author

@vjeffrey thanks for finding that and also requesting UX team verify the desired result. I know why this is happening as all the filters are processed through the parent reporting component. If this needs to be changed, I can do that. 😊

@susanev
Copy link
Contributor

susanev commented May 13, 2020

good catch! summary data should not change based on the filters. the filters appear below the summary and within the tabs. so visually folks will not expect the summary data to change as they adjust the filters.

@jonong1972
Copy link

Yeah this is wonky. I think I understand the reason why the system is doing what its doing. If I recall correctly the search was built as if a user were to search on something, that would affect the status? For instance, show me all nodes with Control X, and then reflect status of nodes based on Control X - which then would update the Summary? I could be wrong though.

@tarablack01
Copy link
Contributor Author

@vjeffrey @susanev @jonong1972 Fixed filtering to be only nodes.

Copy link

@vjeffrey vjeffrey left a comment

Choose a reason for hiding this comment

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

working great thank you!

Copy link
Contributor

@susanev susanev left a comment

Choose a reason for hiding this comment

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

cool thank you!! i think this pr now addresses #3674 too
noticed that when navigating between the tabs that the node status filter persists but the profile one does not. we prolly want those to be consistent. @jonong1972 do you want them to persist when navigating between the tabs or not?

oh wait, also the url...
if i go to nodes and select failed it changes to compliance/reports/nodes?status=failed
then if i navigate to profiles it adjusts to compliance/reports/profiles?status=failed but it shows total selected since i didnt choose the failed status on profiles.
should we fix this? or i guess that status is for nodes only, so prolly the same thing as mentioned above

okay ill stop editing this now

@jonong1972
Copy link

jonong1972 commented May 15, 2020

@susanev yes. would be great it would persist. But I think in order for that to happen for each respective tab we would probably have to change the query param from something like. "?status=failed" to "?node_status=failed&profile_status=failed"

@tarablack01
Copy link
Contributor Author

@jonong1972 @susanev the status will persist. We just need to hook up profiles with api status filtering. 😊

@tarablack01 tarablack01 merged commit 55eae95 into master May 15, 2020
@chef-expeditor chef-expeditor bot deleted the automate-3144 branch May 15, 2020 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants