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

fix(ui): calculate completed scans count correctly #898

Merged
merged 5 commits into from
Nov 3, 2023

Conversation

adamtagscherer
Copy link
Contributor

@adamtagscherer adamtagscherer commented Nov 3, 2023

Description

Fix for: #894
On the Dashboard menu the Completed scans widgets shows every scan not only the completed ones.
This PR fixes this issue by checking the scans for their state property.

Type of Change

[x] Bug Fix
[ ] New Feature
[ ] Breaking Change
[x] Refactor
[ ] Documentation
[ ] Other (please describe)

Checklist

  • I have read the contributing guidelines
  • Existing issues have been referenced (where applicable)
  • I have verified this change is not present in other open pull requests
  • Functionality is documented
  • All code style checks pass
  • New code contribution is covered by automated tests
  • All new and existing tests pass

@adamtagscherer adamtagscherer added bug Something isn't working javascript Pull requests that update Javascript code component:ui Issues related to UI/Frontend labels Nov 3, 2023
@adamtagscherer adamtagscherer self-assigned this Nov 3, 2023
@adamtagscherer adamtagscherer requested a review from a team as a code owner November 3, 2023 10:46

This comment has been minimized.

This comment has been minimized.

@chrisgacsal chrisgacsal changed the title Calculate completed scans count correctly fix(ui): calculate completed scans count correctly Nov 3, 2023
paralta
paralta previously approved these changes Nov 3, 2023
Copy link
Contributor

@paralta paralta left a comment

Choose a reason for hiding this comment

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

LGTM 👍 thanks for fixing this!

lgecse
lgecse previously approved these changes Nov 3, 2023
Copy link
Member

@lgecse lgecse left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @adamtagscherer! :)

@adamtagscherer adamtagscherer dismissed stale reviews from lgecse and paralta via 03445cb November 3, 2023 14:53
gallotamas
gallotamas previously approved these changes Nov 3, 2023
@adamtagscherer adamtagscherer changed the title fix(ui): calculate completed scans count correctly Proper completed scans count Nov 3, 2023
@adamtagscherer adamtagscherer changed the title Proper completed scans count fix(ui): calculate completed scans count correctly Nov 3, 2023
Copy link
Contributor

@Tehsmash Tehsmash left a comment

Choose a reason for hiding this comment

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

I think we should be using the $filter here instead of loading every scan into the UI and then filtering it. e.g.

$filter=state eq 'Aborted' or state eq 'Failed' or state eq 'Done'

This comment has been minimized.

Copy link

github-actions bot commented Nov 3, 2023

Hey!

Your images are ready:

  • ghcr.io/openclarity/vmclarity-apiserver-dev:pr898-05aa82e1afa5675f6ba9a8536687f84152f0d25c
  • ghcr.io/openclarity/vmclarity-orchestrator-dev:pr898-05aa82e1afa5675f6ba9a8536687f84152f0d25c
  • ghcr.io/openclarity/vmclarity-ui-backend-dev:pr898-05aa82e1afa5675f6ba9a8536687f84152f0d25c
  • ghcr.io/openclarity/vmclarity-ui-dev:pr898-05aa82e1afa5675f6ba9a8536687f84152f0d25c
  • ghcr.io/openclarity/vmclarity-cli-dev:pr898-05aa82e1afa5675f6ba9a8536687f84152f0d25c

@adamtagscherer adamtagscherer added this pull request to the merge queue Nov 3, 2023
Merged via the queue into main with commit 7cbbc42 Nov 3, 2023
33 checks passed
@adamtagscherer adamtagscherer deleted the completed-scans-count branch November 3, 2023 17:07
@akpsgit
Copy link
Contributor

akpsgit commented Nov 6, 2023

I see that "Risky Findings" was changed to "Findings" in https://github.com/openclarity/vmclarity/pull/898/files#diff-a7f123acafc6f879519bf54b3e58eb3ebab608f6d1c23a8107b883eee501e8cfR18

The idea was not to count packages, but only findings with Risks.
Also, maybe worth making such changes if needed in a separated PR.

@adamtagscherer
Copy link
Contributor Author

I see that "Risky Findings" was changed to "Findings" in https://github.com/openclarity/vmclarity/pull/898/files#diff-a7f123acafc6f879519bf54b3e58eb3ebab608f6d1c23a8107b883eee501e8cfR18

The idea was not to count packages, but only findings with Risks. Also, maybe worth making such changes if needed in a separated PR.

The problem with this was that the label said "Risky Findings" although we queried every finding without narrowing the query at all.
We can change this query to only GET the Risky Findings but then we have to decide which Findings are risky, so what is the definition of a Risky Finding?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component:ui Issues related to UI/Frontend javascript Pull requests that update Javascript code
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants