Homepage UI component #809

dottorblaster · 2022-02-17T17:01:49Z

^ :-))))

stefanotorresi · 2022-02-22T09:48:58Z

screenshot please! 😸

dottorblaster · 2022-02-22T09:56:28Z

@stefanotorresi yes I wanted to see if tests passed first :-D

dottorblaster · 2022-02-22T09:59:27Z

First attempt, if we don't like it we can refine it later 😄

arbulu89

Thanks @dottorblaster ,
Some initial comments before jumping to the code.

I think we miss a 1st column like SAP systems, after the SID. This would consume the sapsystems_health value in the API.
I think Pacemaker cluster should be changed to Pacemaker clusters (in plural). As we are showing there all the involved clusters (even though by now it is one hehe)
Code related. I think the values in the database and cluster must be flipped (current 1st column is for database and the 2nd for the clusters, the data is inverted now)

dottorblaster · 2022-02-22T10:29:23Z

I think you're right @arbulu89, I skipped the sapsystems_health field as it wasn't in the initial response but I'll add it as soon as possible, thank you!

dottorblaster · 2022-02-22T10:55:52Z

Much better now:

abravosuse · 2022-02-22T11:22:36Z

Guys, may I suggest a couple of changes in display/layout?

Column "SAP Systems": rename it as "SAP instances". It's the status of the SAP instances that we are collecting under this column.
Swipe places of the "Pacemaker cluster" and "Database" columns. It's a matter of association. The database layer is close to the SAP application (instances) layer. The pacemaker cluster is close to the OS (hosts) layer.

dottorblaster · 2022-02-22T11:51:41Z

@abravosuse this way? 😬

abravosuse · 2022-02-22T12:08:55Z

Perfect @dottorblaster . Thanks!

arbulu89

Hey @dottorblaster ,
I did a more thorough review this time.
Some comments added. The unique blocker is that I think you forgot to push some commit. At least the latest image you pasted doesn't match with the code.
We can discuss about everything else.
But it looks good to me.

arbulu89 · 2022-02-22T12:22:21Z

test/e2e/cypress/integration/home.js

-      );
+  describe('The homepage has the global health component visible', () => {
+    it('should display the global health chart', () => {
+      cy.get('#homepage-component').contains('At a glance');


I think we could improve this. At least, with the currently discovered facts, check is we have 3 rows with everything on green. It is the less costly thing we can have, but I find it valuable. I think you can get it less than a half hour hehe

web/frontend/components/HealthSummary/HealthSummary.js

arbulu89 · 2022-02-22T12:26:50Z

web/frontend/components/HealthSummary/HealthSummary.js

+    return predicate[key] === label;
+  }, false);
+
+const getCounters = (data) =>


I think here we should consider to count the unknowns. Otherwise, if some summary entry has unknonws and passes, we will compute this pass, which is not entirely sure. I wouldn't count them as pass at least.
But it is true that we don't have any box for them. @abravosuse What should we do here?

Leave as it is. So if we have some unknown at worst, as pass

Add other box for the unknown

Not put other box, but not count neither

@arbulu89 for this iteration let's go with the third option: not put other box, but no count neither (I don't want an unknown to be counted as a green). But looking down the road this is how I think it should be:

distinguish between stopped and unknown as they are different things (given that grey is the sapcontrol color associated to a stopped status, I suggest we keep grey for stopped and choose another one for unknown. OR, stopped is circle grey and unknown is triangle grey).

add as many boxes as possible statuses

stablish a clear hegemony among statuses:
I think we all agree that:

all green > green

one yellow, rest green > yellow

one red, rest green/yellow > red
But how stopped or unknown weight in the overall status is something we are going to have to determine component by component. What I mean is that, for example, it's not the same to have an ABAP instance stopped than having the diagnostic agent stopped.
All this to be refined.

@dottorblaster I don't know if you read this thread. I think it is missing to be implemented.
We would need to check before the passing, if there is any unknown. So simply adding the next should work:

if (any(element, 'unknown')) { return { ...accumulator, unknown: accumulator.unknown + 1 }; }

At least, we won't have a new passing counter added in this case (unknowns won't be showed in the frontend, but at least neither false passes)

@arbulu89 thanks for the tip, actually I read the thread but I didn't get that detail 😅

web/frontend/components/HealthSummary/HealthSummary.js

arbulu89 · 2022-02-22T12:29:52Z

web/templates/home.html.tmpl

-            documents.
-        </div>
-    </section>
+  <div id="homepage-component"></div>


Question for @abravosuse : Do you think we should move the current home content to somewhere else?
Like the about page or some additional page. Just asking.
It doesn't need to done in this PR anyway

Yes, I think that eventually we will have to move it somewhere else because the home page should all be about the status of your environment. But I'd rather keep the About page as it it. That way the version info is clearly displayed and there is no confusion about it. Maybe another page under Settings called "Intro" or "First steps". To be refined as well.

dottorblaster · 2022-02-22T13:19:45Z

@arbulu89 oh man you're right, I had the code still in my local env, now I pushed it, sorry about that!

nelsonkopliku

Whoa! Great!

@arbulu89 already raised good points, mainly on the current homepage content and the e2e test.

Apart from those points, we're good I guess 😉

fabriziosestito · 2022-02-22T14:42:29Z

web/frontend/stylesheets/stylesheets.scss

+
+.health-summary-id {
+  text-align: left;
+}


not gonna miss this 😇

dottorblaster · 2022-02-22T16:34:45Z

@arbulu89 if you want you can check again, I wanted to page you privately but the major outage is preventing me to do so... 😅

stefanotorresi

LGTM

arbulu89

Hi @dottorblaster ,
Setting the PR as green (I don't think we need more reviews), but there are 2 things that must be fixed. Have a look on my latest 2 comments

arbulu89 · 2022-02-23T06:51:32Z

web/frontend/components/HealthSummary/HealthSummary.js

+    return predicate[key] === label;
+  }, false);
+
+const getCounters = (data) =>


@dottorblaster I don't know if you read this thread. I think it is missing to be implemented.
We would need to check before the passing, if there is any unknown. So simply adding the next should work:

if (any(element, 'unknown')) { return { ...accumulator, unknown: accumulator.unknown + 1 }; }

At least, we won't have a new passing counter added in this case (unknowns won't be showed in the frontend, but at least neither false passes)

arbulu89 · 2022-02-23T06:52:13Z

web/frontend/components/HealthSummary/HealthSummary.js

+                    {getHealthIcon(sapsystem_health)}
+                  </span>
+                  <span className="health-summary-cell">
+                    {getHealthIcon(clusters_health)}


This is still wrong hehe
I think the 2nd must be database_health and the 3rd clusters_health

dottorblaster self-assigned this Feb 17, 2022

dottorblaster force-pushed the homepage-ui-component branch 5 times, most recently from 35a198c to 60956be Compare February 22, 2022 08:38

dottorblaster marked this pull request as ready for review February 22, 2022 09:41

dottorblaster requested review from fabriziosestito, nelsonkopliku, arbulu89, stefanotorresi and rtorrero February 22, 2022 10:02

dottorblaster force-pushed the homepage-ui-component branch 2 times, most recently from b848ad9 to dc2b785 Compare February 22, 2022 10:05

arbulu89 reviewed Feb 22, 2022

View reviewed changes

dottorblaster force-pushed the homepage-ui-component branch 2 times, most recently from 0f7a82a to 7dc9241 Compare February 22, 2022 11:10

arbulu89 suggested changes Feb 22, 2022

View reviewed changes

dottorblaster force-pushed the homepage-ui-component branch from 7dc9241 to 95bdfa8 Compare February 22, 2022 13:19

nelsonkopliku reviewed Feb 22, 2022

View reviewed changes

fabriziosestito reviewed Feb 22, 2022

View reviewed changes

dottorblaster force-pushed the homepage-ui-component branch from 6c96272 to f0bd232 Compare February 22, 2022 16:18

stefanotorresi approved these changes Feb 22, 2022

View reviewed changes

arbulu89 approved these changes Feb 23, 2022

View reviewed changes

dottorblaster added 2 commits February 23, 2022 09:06

Implement home summary UI component

355c479

Fix tests

3f2e066

dottorblaster force-pushed the homepage-ui-component branch from 29081c0 to 3f2e066 Compare February 23, 2022 08:06

nelsonkopliku approved these changes Feb 23, 2022

View reviewed changes

dottorblaster merged commit e185d6d into trento-project:main Feb 23, 2022

dottorblaster deleted the homepage-ui-component branch February 23, 2022 10:42

dottorblaster added the addition New feature from scratch label Feb 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Homepage UI component #809

Homepage UI component #809

dottorblaster commented Feb 17, 2022

stefanotorresi commented Feb 22, 2022

dottorblaster commented Feb 22, 2022

dottorblaster commented Feb 22, 2022

arbulu89 left a comment

dottorblaster commented Feb 22, 2022

dottorblaster commented Feb 22, 2022

abravosuse commented Feb 22, 2022

dottorblaster commented Feb 22, 2022

abravosuse commented Feb 22, 2022

arbulu89 left a comment

arbulu89 Feb 22, 2022

arbulu89 Feb 22, 2022

abravosuse Feb 22, 2022

arbulu89 Feb 23, 2022

dottorblaster Feb 23, 2022

arbulu89 Feb 22, 2022

abravosuse Feb 22, 2022

dottorblaster commented Feb 22, 2022

nelsonkopliku left a comment

fabriziosestito Feb 22, 2022

dottorblaster commented Feb 22, 2022

stefanotorresi left a comment

arbulu89 left a comment

arbulu89 Feb 23, 2022

arbulu89 Feb 23, 2022

Homepage UI component #809

Homepage UI component #809

Conversation

dottorblaster commented Feb 17, 2022

stefanotorresi commented Feb 22, 2022

dottorblaster commented Feb 22, 2022

dottorblaster commented Feb 22, 2022

arbulu89 left a comment

Choose a reason for hiding this comment

dottorblaster commented Feb 22, 2022

dottorblaster commented Feb 22, 2022

abravosuse commented Feb 22, 2022

dottorblaster commented Feb 22, 2022

abravosuse commented Feb 22, 2022

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dottorblaster commented Feb 22, 2022

nelsonkopliku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dottorblaster commented Feb 22, 2022

stefanotorresi left a comment

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment