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-13316): fix systems count discrepancy #2171

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Fewwy
Copy link
Contributor

@Fewwy Fewwy commented Nov 21, 2024

https://issues.redhat.com/browse/RHINENG-13316

I discovered the core of the issue why this is happening, but I want to ask your advice on how to update the structure of the cvedetails page to fix this.
So, the issue is the following, in vulnerability we're using 3 different requests to grab the "total items" of systems. 2 of them are for the CVEDetailTableTitle component and they are described in fetchHybridSystemsCounts
The other one is getEntites for the CVEDetailsPage.js, the totalItems we get with useSelector from entities.total
This causes the following issue:
we render the Title "Total systems affected"
That triggers fetchHybridSystemsCounts and we get total items from that, pass it through context and render the number in title
Then inventory table renders and we fetch entities with getEntities
We get totalItems value from the redux store and render it in the table
If, for some reason the amount of total systems for this CVE changes between step 2 and 3 - we get different numbers in Title and Table
Here's an example, I console.logged values in CVEDetailTableTitle.js and totalItems in CVEDetailsPage.js. I refreshed the CVE details page several times and got a discrepancy

image

image

My fix for this - avoid using fetchHybridSystemsCount and use the one we get from get entities.

@Fewwy Fewwy requested a review from a team as a code owner November 21, 2024 08:11
@Fewwy Fewwy self-assigned this Nov 21, 2024
Copy link

jira-linking bot commented Nov 21, 2024

Commits missing Jira IDs:
8d52f7c
7912032
63f6bd6

@Fewwy Fewwy added the bug Something isn't working label Nov 21, 2024
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.14%. Comparing base (e91b55d) to head (63f6bd6).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2171   +/-   ##
=======================================
  Coverage   67.14%   67.14%           
=======================================
  Files         130      130           
  Lines        3476     3476           
  Branches     1106     1107    +1     
=======================================
  Hits         2334     2334           
  Misses       1142     1142           

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

Copy link
Collaborator

@adonispuente adonispuente left a comment

Choose a reason for hiding this comment

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

I believe what youre finding is a symptom and not a root cause. I believe the root cause is because we hard code the filter in cveDetailHelpers, for fetchHybridSystemsCount. So while inventory/other request get updated with filters, that one count never gets updated.

const filters = { id: cveName, limit: 1, advisory_available: true };

Im not against just using one, but id ask the backend folks if this count is accurate.

@Fewwy
Copy link
Contributor Author

Fewwy commented Jan 16, 2025

@adonispuente I finally got some free time to check back on this PR. My opinion still stands –> it doesn't matter that one thing is not updated when the other does. I asked the Vuln team, and they said that the isEdgeParityEnabled flag could be removed, so we don't need to make 3 requests instead of one. I think we need to review this PR, merge it. Check that the bug is fixed. In the next PR I will remove the isEdgeParityEnabled flags that are leftovers from when it was implemented.

@adonispuente
Copy link
Collaborator

@Fewwy
Your opinion can be it doesn't matter that one thing is not updated when the other does, but the ticket itself suggest the exact opposite. If the PM is fine with the number not updating then changing the number is fine and the PR can be merged, but if its required for that number to change like the ticket suggest, then itd require another look at that code block I mentioned previously

@adonispuente
Copy link
Collaborator

adonispuente commented Jan 16, 2025

In the case PM is fine with the resolution, I also noticed that certain CVEs will report a different number still, below is an example ->

Screenshot 2025-01-16 at 2 46 53 PM

Screenshot 2025-01-16 at 2 47 08 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants