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(RHINENG-7963): Bug with select all CVEs option on CVEs page #2082

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

LiorKGOW
Copy link
Member

This PR addresses issue #RHINENG-7963

When attempting to "select all" CVEs in the CVEs Page, none are selected
With this code selecting all CVEs works

The reason why this bug occurres:
If a user has Edge systems in their account, we need to convert the affecting param to be in affectingHostType
So, we use hybrid system filter to determine this.

  • If the user have both Conventional & Edge systems, we convert the affecting params to be in affectingHostType params because affecting is no longer being used in the backend in this case.
  • If the user has only Conventional systems, the backend still uses affecting param, so there is no need for the conversion.

Because affecting param is no longer being used in the backend in the first case, sending it in the request would make the request fail, or in case the request does not fail, it would return bad results

Location where this 'selectAllCVEs' option is located:
Location where this 'selectAllCVEs' option is located

@LiorKGOW LiorKGOW self-assigned this Mar 19, 2024
@LiorKGOW LiorKGOW requested a review from a team as a code owner March 19, 2024 12:27
Copy link

jira-linking bot commented Mar 19, 2024

Referenced Jiras:
https://issues.redhat.com/browse/RHINENG-7963

@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 68.75000% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 67.81%. Comparing base (715cd47) to head (8f65962).
Report is 1 commits behind head on master.

❗ Current head 8f65962 differs from pull request most recent head ebbea52. Consider uploading reports for the commit ebbea52 to get more accurate results

Files Patch % Lines
src/Helpers/MiscHelper.js 75.00% 3 Missing ⚠️
src/Helpers/APIHelper.js 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2082      +/-   ##
==========================================
- Coverage   67.84%   67.81%   -0.04%     
==========================================
  Files         131      131              
  Lines        3418     3424       +6     
  Branches     1062     1064       +2     
==========================================
+ Hits         2319     2322       +3     
- Misses       1099     1102       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/Helpers/APIHelper.js Outdated Show resolved Hide resolved
@LiorKGOW
Copy link
Member Author

Thank you for the review @bastilian !
I have addressed your comment and moved the comment in the code to be in the commit message instead 👍🏼

This PR is ready for another review 😄

@LiorKGOW LiorKGOW requested a review from bastilian March 19, 2024 13:14
src/Helpers/APIHelper.js Outdated Show resolved Hide resolved
src/Helpers/APIHelper.js Outdated Show resolved Hide resolved
@LiorKGOW
Copy link
Member Author

/retest

Copy link
Contributor

@mkholjuraev mkholjuraev left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally, I was able to select all items using an account with both conventional and edge systems.

@LiorKGOW LiorKGOW force-pushed the select-all-fix branch 2 times, most recently from db8ba17 to 8f65962 Compare March 20, 2024 15:58
@LiorKGOW
Copy link
Member Author

LiorKGOW commented Mar 20, 2024

Thank you for the review ! @mkholjuraev
I have addressed your comments and updated the PR so it is ready for another review :)

If a user has Edge systems in their account, we need to convert the affecting param to be in affectingHostType
So, we use hybrid system filter to determine this.

- If the user have both Conventional & Edge systems, we convert the affecting params to be in affectingHostType params
because affecting is no longer being used in the backend server in this case.
- If the user has only Conventional systems, the backend server still uses affecting param, so there is no need for the conversion.

Because affecting param is no longer being used in the backend server in the first case, sending it in the request would make the request fail, or in case the request does not fail, it would return bad results

Link to issue: https://issues.redhat.com/browse/RHINENG-7963
@LiorKGOW
Copy link
Member Author

Rebased and repushed after #2079 was merged to master

@mkholjuraev
Copy link
Contributor

@LiorKGOW looks good to me now. Ready to be merged now

@LiorKGOW LiorKGOW merged commit 914ee34 into RedHatInsights:master Mar 21, 2024
4 checks passed
@LiorKGOW LiorKGOW deleted the select-all-fix branch March 21, 2024 11:17
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.

4 participants