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

Customizable chart top values (dropdown choices) #8826

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

miyamotoh
Copy link
Contributor

@miyamotoh miyamotoh commented Jun 14, 2023

Depends on

Proposing to make the "chart top values" (currently hardcoded to 10) externalized to settings.yaml and thus customizable. It'd read the below setting, if present, and use it. If this is good, I'm happy to create a PR for the Core with the default value (10).

:reporting
  :chart_top_values: 20

This controls how long/big this dropdown goes;
image

@miyamotoh miyamotoh requested a review from a team as a code owner June 14, 2023 18:07
@miyamotoh miyamotoh marked this pull request as draft June 14, 2023 19:10
@miyamotoh
Copy link
Contributor Author

Noticed that the graph field in miq_reports table shows the chosen value as a text, instead of an integer. Put this in WIP until a fix is found...

@miyamotoh miyamotoh changed the title Customizable chart top values (dropdown choices) [WIP] Customizable chart top values (dropdown choices) Jun 14, 2023
@miq-bot miq-bot added the wip label Jun 14, 2023
@miyamotoh miyamotoh force-pushed the customizable-chart-top-values branch from dbfcedb to 0fe84a1 Compare June 14, 2023 20:46
@miyamotoh miyamotoh changed the title [WIP] Customizable chart top values (dropdown choices) Customizable chart top values (dropdown choices) Jun 15, 2023
@miq-bot miq-bot removed the wip label Jun 15, 2023
@jeffibm jeffibm changed the title Customizable chart top values (dropdown choices) [WIP] Customizable chart top values (dropdown choices) Jun 15, 2023
@miq-bot miq-bot added the wip label Jun 15, 2023
@miyamotoh
Copy link
Contributor Author

miyamotoh commented Jun 15, 2023

I added a .to_i upon reading in the form selected value, which seems to be a sensible thing to do, though I don't know how it was "working" without before. Maybe .to_i is also needed for GRAPH_MAX_COUNT initialization, too, and now my local server/db is somehow plagued??

Anyway, I'm ready for bringing this forward, and thus will try to remove the Draft state (like last night)...

@miyamotoh miyamotoh changed the title [WIP] Customizable chart top values (dropdown choices) Customizable chart top values (dropdown choices) Jun 15, 2023
@miyamotoh miyamotoh marked this pull request as ready for review June 15, 2023 15:28
@miq-bot miq-bot removed the wip label Jun 15, 2023
@miyamotoh miyamotoh force-pushed the customizable-chart-top-values branch from 0fe84a1 to 2594c31 Compare June 16, 2023 16:36
@jeffibm jeffibm requested a review from Fryguy July 5, 2023 11:11
GRAPH_MAX_COUNT = 10
GRAPH_MAX_COUNT = ::Settings.reporting.chart_top_values || 10
Copy link
Member

Choose a reason for hiding this comment

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

This is happening at constant time, so if the user changes it you'd have to reboot the worker/server. Instead of a constant, this should probably move to a method to allow the runtime lookup.

Additionally, we try to avoid having "hidden" values in the settings. Instead, it's probably preferable to only use settings, and then just set the value in the settings to 10.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fryguy I'm afraid that I'm not versed to come up with the best way to do what you are suggesting, but PTAL at my latest force-push. It appears to achieve the dynamic behavior on UI.

In the meantime, I opened this PR on Core to support this logic.

@miyamotoh miyamotoh force-pushed the customizable-chart-top-values branch from 2594c31 to 98e8964 Compare July 24, 2023 18:11
@@ -1202,7 +1205,7 @@ def set_form_vars
@edit[:new][:graph_other] = @rpt.graph[:other] ? @rpt.graph[:other] : false
else
@edit[:new][:graph_type] = @rpt.graph
@edit[:new][:graph_count] = GRAPH_MAX_COUNT
@edit[:new][:graph_count] = chart_top_values
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 needs to be

Suggested change
@edit[:new][:graph_count] = chart_top_values
@edit[:new][:graph_count] = self.class.chart_top_values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my tests, chart_top_values didn't work, nor did self.class.chart_top_values. The only one I got working is ReportController::Reports::Editor.chart_top_values, which looks odd right in the module defining file itself, but... anyway, PTAL at the latest force-push.

@Fryguy
Copy link
Member

Fryguy commented Jul 24, 2023

Please also include a default value hardcoded in the settings and set to 10 (I think it needs to be a core PR and then make this PR dependent on that one)

@Fryguy
Copy link
Member

Fryguy commented Jul 24, 2023

@miyamotoh Just for my clarity, what does this value represent? Is this a dashboard top 10 thing only, or is this any chart report? If the former, I wonder if we should more clearly name the option :dashboard_chart_top_values, or something. On that note, does this also affect dashboard tables (that is, things that aren't charts)?

@miyamotoh
Copy link
Contributor Author

@Fryguy PTAL the opening description of this PR, as I added a little screenshot.

@Fryguy
Copy link
Member

Fryguy commented Jul 24, 2023

ohhh thanks...that's much clearer now

@Fryguy
Copy link
Member

Fryguy commented Jul 24, 2023

updated the OP to show it depends on ManageIQ/manageiq#22632

@Fryguy
Copy link
Member

Fryguy commented Jul 24, 2023

Weird question. If someone picks a large number, say 10, and saves the report, then an admin changes the settings to a smaller number, say 5, then what happens to existing reports. I'm ok if they continue to show 10, but we need to make sure the report editor doesn't blow up.

@miyamotoh
Copy link
Contributor Author

It jumps back to "3." If I set the max to 2, you'd get the below, can't change the previous selection, Save button even grayed out. Even if you get the Save button enabled by changing the display-other selection, the subsequent Save won't change the top-values in the DB.

image

@miyamotoh miyamotoh force-pushed the customizable-chart-top-values branch from 98e8964 to 59b237a Compare July 24, 2023 19:27
@Fryguy Fryguy closed this Jul 27, 2023
@Fryguy Fryguy reopened this Jul 27, 2023
@miyamotoh miyamotoh force-pushed the customizable-chart-top-values branch from b74716c to 710ff48 Compare August 2, 2023 14:48
@jeffibm
Copy link
Member

jeffibm commented Aug 3, 2023

Hey @miyamotoh , could you please check with the failing specs..

Signed-off-by: Hiro Miyamoto <miyamotoh@us.ibm.com>
@miyamotoh miyamotoh force-pushed the customizable-chart-top-values branch from 710ff48 to 93fc67c Compare August 3, 2023 12:54
@miq-bot
Copy link
Member

miq-bot commented Aug 3, 2023

Checked commit miyamotoh@93fc67c with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
3 files checked, 0 offenses detected
Everything looks fine. 🏆

@miyamotoh
Copy link
Contributor Author

@jeffibm PTAL at the latest force-push. Thanks!

@agrare agrare merged commit 30eb548 into ManageIQ:master Aug 10, 2023
9 checks passed
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.

5 participants