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

Fixes #24829: Replace compliance chart with Score chart and and new details score charts #5648

Merged
merged 1 commit into from May 24, 2024
Merged

Fixes #24829: Replace compliance chart with Score chart and and new details score charts #5648

merged 1 commit into from May 24, 2024

Conversation

VinceMacBuche
Copy link
Member

@VinceMacBuche VinceMacBuche requested a review from fanf May 2, 2024 21:23
@VinceMacBuche
Copy link
Member Author

Commit modified

@VinceMacBuche VinceMacBuche force-pushed the enh_24829/replace_compliance_chart_with_score_chart_and_and_new_details_score_charts branch from 8b5df0f to 6e2f39e Compare May 2, 2024 21:25
@fanf
Copy link
Member

fanf commented May 3, 2024

It seems that there is a problem with the percent computing:
image

fanf
fanf reviewed May 3, 2024
View reviewed changes
webapp/sources/rudder/rudder-web/src/main/scala/com/normation/rudder/web/snippet/HomePage.scala
case D => "#fedc04"
case E => "#f0940e"
case F => "#da291c"
case NoScore => ""
Copy link
Member

Choose a reason for hiding this comment

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

Why not grey ?

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact i think we want to filter Nodes without a score, but i don't know really. Yes in this case we should display them in grey

@VinceMacBuche
Copy link
Member Author

Commit modified

@VinceMacBuche VinceMacBuche force-pushed the enh_24829/replace_compliance_chart_with_score_chart_and_and_new_details_score_charts branch from 6e2f39e to a646b03 Compare May 6, 2024 13:03
@VinceMacBuche
Copy link
Member Author

PR updated with a new commit

fanf
fanf reviewed May 15, 2024
View reviewed changes
webapp/sources/rudder/rudder-web/src/main/scala/com/normation/rudder/web/snippet/HomePage.scala Outdated
@@ -336,6 +314,7 @@ class HomePage extends StatefulSnippet {
, ${data.toJsCmd}
, ${diagramColor.toJsCmd}
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not used anymore and should be removed

@fanf
Copy link
Member

fanf commented May 15, 2024

The problem with percent is because the total number of node is not the same in the array than in the total :

  • call js function doughnutChart('nodeCompliance', nodeCompliance, allNodes, nodeCompliance.colors, complianceHColors);
    With:
nodeCompliance:
        {
            "labels": ["E", "F"],
            "values": [1, 148],
            "colors": ["#f0940e", "#da291c"]
        }

allNodes: 130

I have disabled and pending nodes, which seems to be excluded from allNodes, but not from the data given with label/values. It must be consistent, likely by being all computed server-side and used everywhere

@fanf fanf force-pushed the enh_24829/replace_compliance_chart_with_score_chart_and_and_new_details_score_charts branch from 78a4233 to c4b14b3 Compare May 23, 2024 13:55
fanf
fanf reviewed May 23, 2024
View reviewed changes
webapp/sources/rudder/rudder-web/src/main/javascript/rudder/homePage.js
@@ -114,8 +114,8 @@ function homePage (
globalCompliance
, globalGauge
, nodeCompliance
, nodeComplianceColors
Copy link
Member

Choose a reason for hiding this comment

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

that's not used anywhere

fanf
fanf reviewed May 23, 2024
View reviewed changes
webapp/sources/rudder/rudder-web/src/main/javascript/rudder/homePage.js
@@ -183,8 +184,21 @@ function homePage (
$("#gauge-value").text(globalGauge+"%");

var complianceHColors = nodeCompliance.colors.map(x => complianceHoverColors[x]);
doughnutChart('nodeCompliance', nodeCompliance, allNodes, nodeCompliance.colors, complianceHColors);
Copy link
Member

Choose a reason for hiding this comment

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

the count is now computed directly in doughnutChart function

fanf
fanf reviewed May 23, 2024
View reviewed changes
webapp/sources/rudder/rudder-web/src/main/javascript/rudder/homePage.js

var context = $("#"+id)
var count = data.values.length < 1 ? 0 : data.values.reduce((a, b) => a + b, 0);
Copy link
Member

Choose a reason for hiding this comment

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

in place of count, compute the sum of values here.
I didn't see a case where it should be ok to have a different count, and all calling place in scala backend use the same source of data to build both

@VinceMacBuche
Copy link
Member Author

PR rebased

@VinceMacBuche VinceMacBuche force-pushed the enh_24829/replace_compliance_chart_with_score_chart_and_and_new_details_score_charts branch from c4b14b3 to 4c4d9ab Compare May 23, 2024 17:45
@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant merged commit 8ff84ae into Normation:branches/rudder/8.1 May 24, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fanf fanf fanf left review comments

Assignees
No one assigned
Labels
Ready for merge
Projects
None yet
Milestone
No milestone
3 participants
@VinceMacBuche @fanf @Normation-Quality-Assistant