Gal/311498 visualize events to physical storage page #8429

galoiring · 2022-08-30T17:18:41Z

This PR is about the new widget added to the storage system manager page , that shows the event on a graph (per storage system) that show the distribution between fix and unfixed event.

agrare · 2022-08-31T18:19:45Z

@galoiring if this is dependent on other PRs can you please list them in the description?

galoiring · 2022-08-31T18:23:13Z

@galoiring if this is dependent on other PRs can you please list them in the description?

This PR isn't depend on other PR's..

agrare · 2022-08-31T18:24:16Z

This PR is about the new widget added to the storage system manager page , that shows the event on a graph (per storage system) that show the distribution between fix and unfixed event.

Isn't that widget not merged yet? ManageIQ/manageiq#22081

galoiring · 2022-08-31T18:28:45Z

This PR is about the new widget added to the storage system manager page , that shows the event on a graph (per storage system) that show the distribution between fix and unfixed event.

Isn't that widget not merged yet? ManageIQ/manageiq#22081

if you meant it regarding to PR #22081 , this has similarity but operate in a different way.

app/javascript/components/provider-dashboard-charts/events-pie-chart/index.js

agrare · 2022-08-31T21:01:32Z

if you meant it regarding to PR #22081 , this has similarity but operate in a different way.

Do we need both? I'm confused what these two do and why do them two different ways

galoiring · 2022-09-01T10:56:15Z

if you meant it regarding to PR #22081 , this has similarity but operate in a different way.

Do we need both? I'm confused what these two do and why do them two different ways

In the general dashboard we wish to display general distribution of fixed vs unfixed event for all managers and storage systems.
In a specific manager's dashboard we wish to display only the events from that manager, and this by distribution of both the storage system from which they came and their fixed/unfixed status

agrare · 2022-09-02T17:04:20Z

@jeffibm @GilbertCherrie @DavidResende0 can you guys review this? I'm surprised how much ruby code is required here I thought the report definitions were yaml but I might be missing something.

jeffibm · 2022-09-05T03:55:54Z

Hey @galoiring, could you please squash the commits if it's ready to be reviewed & merged? Could you also provide a before/after screenshot of full page for sprint review documentation purpose.

galoiring · 2022-09-05T10:34:04Z

Hey @galoiring, could you please squash the commits if it's ready to be reviewed & merged? Could you also provide a before/after screenshot of full page for sprint review documentation purpose.

I will squash right now.
Regards to the 'before' image, this is a new widget

p.s
@jeffibm I deleted you as reviewer by mistake , sorry .

jeffibm · 2022-09-06T13:53:05Z

Hi @galoiring , could you please add few steps on how/where to check this in UI.
The Code seems good, just need to check the screens in local env.

galoiring · 2022-09-06T16:32:23Z

Hi @galoiring , could you please add few steps on how/where to check this in UI.
The Code seems good, just need to check the screens in local env.

@jeffibm
yes sir.
this widget to be found inside the Storage->manager (after connecting some storage manager)
(similar to the Heat Map Graph for example)

jeffibm · 2022-09-12T03:40:51Z

can you post a full-page screenshot here?

galoiring · 2022-09-12T09:07:48Z

can you post a full-page screenshot here?

This is for example , with two storage managers.

jeffibm · 2022-09-12T09:46:46Z

Hey, I am not sure if am doing it right. but this is what I see in local env.

galoiring · 2022-09-13T07:59:37Z

@jeffibm
While trying to reproduce your error, I was creating a new cinder manager (via the rails console).
As you can see here it is working..
(This is on Vm)

jeffibm · 2022-09-13T13:48:01Z

@jeffibm While trying to reproduce your error, I was creating a new cinder manager (via the rails console). As you can see here it is working.. (This is on Vm)

great..could you also have a look into the lint errors please..

galoiring · 2022-09-14T13:29:02Z

@jeffibm While trying to reproduce your error, I was creating a new cinder manager (via the rails console). As you can see here it is working.. (This is on Vm)

great..could you also have a look into the lint errors please..

@jeffibm
I fix what I can.
the loops can't be combined, need to iterate two times..

galoiring · 2022-09-18T08:59:20Z

@GilbertCherrie @agrare @jeffibm
kind reminder, thanks :)

jeffibm · 2022-09-20T05:12:27Z

@GilbertCherrie @agrare @jeffibm
kind reminder, thanks :)

hey, could you please have a look at the lint error by making use of the rubucop feature as mentioned in the above comment. once that is done we can merge this PR...

galoiring · 2022-09-20T07:53:50Z

@GilbertCherrie @agrare @jeffibm
kind reminder, thanks :)

hey, could you please have a look at the lint error by making use of the rubucop feature as mentioned in the above comment. once that is done we can merge this PR...

@jeffibm
great, I fix all the error

…unfixed event by storage system.

miq-bot · 2022-09-20T09:42:02Z

Checked commit Autosde@a012cbc with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
4 files checked, 2 offenses detected

app/views/ems_storage/_show_block_storage_dashboard.html.haml

⚠️ - Line 10 - Avoid using instance variables in partials views
⚠️ - Line 7 - Avoid using instance variables in partials views

app/services/ems_storage_dashboard_service.rb

It was added in a just merged PR: ManageIQ#8429 There is no need for interpolation here. Fixes a brakeman warning: Confidence: Medium Category: SQL Injection Check: SQL Message: Possible SQL injection Code: EmsEvent.where("ems_id=#{@ems_id}") File: ../../.gem/ruby/2.7.6/bundler/gems/manageiq-ui-classic-dd7330eae02e/app/services/ems_storage_dashboard_service.rb Line: 102

jrafanie · 2022-09-20T15:46:41Z

@galoiring I've opened a PR to fix a security scan that started failing after this was merged, see: #8455

app/services/ems_storage_dashboard_service.rb

app/javascript/components/carbon-charts/stackBarChart.js

Fryguy · 2022-09-20T16:03:08Z

I don't think this was ready for merge, unfortunately - I still don't understand the use case. I'm also concerned this isn't going through the API, or perhaps it is? I don't understand how the chart gets the data.

app/services/ems_storage_dashboard_service.rb

galoiring · 2022-09-21T08:42:00Z

@agrare
I try to fix it by your suggestion,
can you reopen this PR ?

agrare · 2022-09-21T17:07:18Z

Hey @galoiring you can't reopen a PR that has been merged but you can create a new branch and open a new PR

galoiring · 2022-09-22T07:21:14Z

Hey @galoiring you can't reopen a PR that has been merged but you can create a new branch and open a new PR

iv opened a new one.
thanks.

#8457

It was added in a just merged PR: ManageIQ#8429 There is no need for interpolation here. Fixes a brakeman warning: Confidence: Medium Category: SQL Injection Check: SQL Message: Possible SQL injection Code: EmsEvent.where("ems_id=#{@ems_id}") File: ../../.gem/ruby/2.7.6/bundler/gems/manageiq-ui-classic-dd7330eae02e/app/services/ems_storage_dashboard_service.rb Line: 102

galoiring changed the title ~~[WIP] Gal/311498 visualize events to physical storage page~~ Gal/311498 visualize events to physical storage page Aug 31, 2022

galoiring force-pushed the Gal/311498_Visualize_events_to_physical_storage_page branch from f7b5551 to 020213b Compare August 31, 2022 09:19

Fryguy assigned agrare Aug 31, 2022

Fryguy added the enhancement label Aug 31, 2022

agrare requested review from GilbertCherrie and jeffibm August 31, 2022 18:19

agrare reviewed Aug 31, 2022

View reviewed changes

app/javascript/components/provider-dashboard-charts/events-pie-chart/index.js Show resolved Hide resolved

galoiring force-pushed the Gal/311498_Visualize_events_to_physical_storage_page branch from c27f846 to 1e4bb46 Compare September 5, 2022 10:35

galoiring requested review from agrare and removed request for GilbertCherrie and jeffibm September 6, 2022 08:05

agrare requested review from GilbertCherrie and jeffibm September 6, 2022 13:22

galoiring force-pushed the Gal/311498_Visualize_events_to_physical_storage_page branch from 1e4bb46 to fab9ba9 Compare September 14, 2022 10:46

galoiring force-pushed the Gal/311498_Visualize_events_to_physical_storage_page branch 3 times, most recently from 6f4b889 to f35bd31 Compare September 14, 2022 11:27

galoiring force-pushed the Gal/311498_Visualize_events_to_physical_storage_page branch from f35bd31 to 3402d2b Compare September 20, 2022 07:48

galoiring requested a review from a team as a code owner September 20, 2022 07:48

galoiring force-pushed the Gal/311498_Visualize_events_to_physical_storage_page branch from 3402d2b to 6766129 Compare September 20, 2022 09:34

added widget inside storage manager that shows distribution of fixed/…

a012cbc

…unfixed event by storage system.

galoiring force-pushed the Gal/311498_Visualize_events_to_physical_storage_page branch from 6766129 to a012cbc Compare September 20, 2022 09:39

jeffibm approved these changes Sep 20, 2022

View reviewed changes