Fix health container numbers and pagination numbers #725

arbulu89 · 2022-01-20T09:06:45Z

Fix how the health container numbers are shown, to show all the numbers except the current page numbers.
Filter properly the health with pagination. Now, the health filter is done together with the others using SQL, so it is included in the pagination. Before this, as the health filter was applied after the pagination, we could get the elements in pages that don't really exist (like only having 2 hosts, but being them in page 4).
Fix the pagination total numbers and page count

In order to improve the code structure, I have moved some structs to models

nelsonkopliku

Looks good @arbulu89! Thanks for addressing this!

There's a couple of little comments, tho! eheh 😄 Nothing major.

Keep this in mind: If you rebase on main, the hosts_overview.js test should fail, as the behavior changed.
Specifically this one should show health status of the first 10 visible hosts, so the test needs to be updated, and the feature file too 😄

As an extra reminder for future discussions: I am wondering whether we are doing too much things in our controllers and if the application might need some refactor as we go further refining requirements.

web/clusters.go

web/sap_systems.go

web/services/clusters.go

arbulu89 · 2022-01-20T14:25:27Z

Looks good @arbulu89! Thanks for addressing this!

There's a couple of little comments, tho! eheh smile Nothing major.

Keep this in mind: If you rebase on main, the hosts_overview.js test should fail, as the behavior changed. Specifically this one should show health status of the first 10 visible hosts, so the test needs to be updated, and the feature file too smile

As an extra reminder for future discussions: I am wondering whether we are doing too much things in our controllers and if the application might need some refactor as we go further refining requirements.

Yes. I'm aware of this divergence in the recently added tests, and I'm working on them. Thank you for the reminder though.

nelsonkopliku · 2022-01-20T14:38:50Z

Yes. I'm aware of this divergence in the recently added tests, and I'm working on them. Thank you for the reminder though.

Great!

If you are experiencing randomic failures during Health Detection maybe this #726 could help 😉

arbulu89 · 2022-01-20T14:56:06Z

Yes. I'm aware of this divergence in the recently added tests, and I'm working on them. Thank you for the reminder though.

Great!

If you are experiencing randomic failures during Health Detection maybe this #726 could help wink

By the way, do you prefer guys if I rebase already existing e2e and add the updates here, or a new PR with that? There won't be that many changes in cypress, so maybe we can put them all together here.

nelsonkopliku · 2022-01-20T15:00:24Z

By the way, do you prefer guys if I rebase already existing e2e and add the updates here, or a new PR with that? There won't be that many changes in cypress, so maybe we can put them all together here.

I think the test needs to be updated in this PR, otherwise e2e will fail and pipeline won't proceed 🤔

arbulu89 · 2022-01-20T16:59:27Z

@nelsonkopliku Changes applied, main branch rebased and e2e test adapted!

fabriziosestito · 2022-01-21T08:31:09Z

web/clusters.go

-			return
-		}
-		pagination := NewPagination(count, pageNumber, pageSize)
+		pagination := NewPagination(len(clusterListAll), pageNumber, pageSize)


I would instead introduce another health count service method that gives as an output the health counts directly, and the delegate that to the service instead of loading again the whole list in the controller, and use the count method for the pagination lenght.

But what would this improve? In order to get the health numbers, we need to get all the clusters with the applied filters and get the health data.
We could create other service method, but it would do exactly the same as far as I see. I don't know any way to save any query to the database.
It might look cleaner, yes. But the code efficiency and in fact almost all the code would be the same.
Or am I missing something?

PD: And we cannot use the currently existing GetCount method, as we need to get the count with the filtered values, which at the end, would require to use the same sql where conditions we have now. Does it make sense to complicate the code yet more?

PD2: The unique way I see to reduce this to a unique call is to do the pagination within the code instead of using sql conditions

It might look cleaner, yes. But the code efficiency and in fact almost all the code would be the same.
Or am I missing something?

If I am not mistaken, I think that to get the health status we don't need a "select *" query that also does various preload (e.g. Tags) but just the IDs of the clusters (a select id + pluck) should be enough to be used in the check service GetAggregatedChecksResultByCluster(cluster.ID)

PD: And we cannot use the currently existing GetCount method, as we need to get the count with the filtered values, which at the end, would require to use the same sql where conditions we have now.

By abstracting the filtering to a new method we can shape a GetCount with the filters, I don't think this would complicate the code a lot.

No, the get the health we need to preload the tags, as we need to filter them. And we would need to get the id, name, cluster_type and sid. And we would need to get the checks results too. So the unique difference would to we discard only the number of hosts and the number of resources

fabriziosestito · 2022-01-21T08:35:27Z

web/hosts.go

-		hContainer := NewHostsHealthContainer(hostList)
+		pagination := NewPagination(len(hostListAll), pageNumber, pageSize)
+
+		hContainer := NewHostsHealthContainer(hostListAll)


Same as for the cluster comment, we fetch the list twice, while we just need the health count. I understand that queries are to be done to aggregate the data in the service anyway, but fetching the whole list with preloaded data seems overkill. It also helps with clarity in the controller which I feel is doing too much stuff ATM.

fabriziosestito · 2022-01-21T08:39:37Z

web/sap_systems.go

 		if err != nil {
 			_ = c.Error(err)
 			return
 		}

-		count, err := sapSystemsService.GetDatabasesCount()
+		filterTags, err := sapSystemsService.GetAllDatabasesTags()


Same as above, I don't think we need to do the whole table fetching.

fabriziosestito · 2022-01-21T08:41:20Z

@arbulu89 thanks for addressing this! I have some doubts about how we fetch the paginated / non-paginated list twice in the contoller, since it seems that we can delegate some stuff to the service and maybe improve the fact that we load the whole preloaded list twice.

nelsonkopliku · 2022-01-21T09:26:29Z

@nelsonkopliku Changes applied, main branch rebased and e2e test adapted!

Ah great stuff! Hope you did't encounter too many quirks 😅

I also took a look at the test/e2e/cypress/fixtures/hosts-overview/hosts_overview.feature and actually it does seem to still properly enough describe the feature, so no actual change is required there.

nelsonkopliku

For me it's a green light as long as the discussion with @fabriziosestito is sorted out.

In that regards, here's my two cents:
I think we all agree that we need to discuss about the codebase and code design itself, related also to the domain we're modeling.

So whatever we do here now, it might be anyway subject of change again, so let's do just the good enough.

nelsonkopliku · 2022-01-21T13:22:46Z

test/e2e/cypress/integration/hosts_overview.js

                    const selectedOption = `#bs-select-1-${index}`
                    cy.get(selectedOption).click()
                    cy.wait('@filterByHealthStatus').then(() => {
                        expectedHostsWithThisHealth == 0 && cy.get('.table.eos-table').contains('There are currently no records to be shown')
                        expectedHostsWithThisHealth > 0 && cy.get('.tn-hostname').its('length').should('eq', expectedHostsWithThisHealth)
+                        cy.get('.pagination-count').should('contain', `${expectedHostsWithThisHealth} items`)


As a suggestion for the future:

This could be a custom cy command https://docs.cypress.io/api/cypress-api/custom-commands, and we can think of it as a custom assertion for pagination.

Something along these lines:

Cypress.Commands.add('paginationShouldMatch', (itemsCount) => { cy.get('.pagination-count').should('contain', `${itemsCount} items`) cy.get('.page-item').its('length').should('eq', Math.ceil(itemsCount/100)+2) })

which then becomes accessible as

cy.paginationShouldMatch(expectedHostsWithThisHealth) cy.paginationShouldMatch(expectedRelatedHosts) cy.paginationShouldMatch(expectedTaggedHosts)

in the different variants.

Look, don't bother changing it now, as I understand it wouldn't add much value right now 😄
Just wanted to share the cypress concept of custom command that might become more and more useful as we go.

arbulu89 · 2022-01-24T12:40:34Z

Hey @fabriziosestito ,
I would need you to remove your blocker to merge this PR

fabriziosestito · 2022-01-24T12:50:52Z

Hey @fabriziosestito , I would need you to remove your blocker to merge this PR

@arbulu89 sorry thought i did it already, now it is green

arbulu89 added the bug Something isn't working label Jan 20, 2022

arbulu89 linked an issue Jan 20, 2022 that may be closed by this pull request

Health overview should give information about all the hosts #691

Closed

arbulu89 marked this pull request as ready for review January 20, 2022 09:38

arbulu89 requested review from fabriziosestito, nelsonkopliku, rtorrero, dottorblaster and stefanotorresi January 20, 2022 09:38

arbulu89 force-pushed the hotfix/fix-pagination-numbers-health branch from d6bb91d to 1decd4c Compare January 20, 2022 09:39

nelsonkopliku reviewed Jan 20, 2022

View reviewed changes

web/clusters.go Outdated Show resolved Hide resolved

web/clusters.go Outdated Show resolved Hide resolved

web/sap_systems.go Outdated Show resolved Hide resolved

web/services/clusters.go Outdated Show resolved Hide resolved

arbulu89 added 6 commits January 20, 2022 17:56

Visualize the health container and pagination number properly

c8c5678

Filter the hosts properly by health

e222e35

Filter the clusters properly by health

b1ad59a

Update the tests to use the new filters

288eca0

Rename paginated lists variable names

730fd83

Move last checks result query to checks service

1c80b6e

arbulu89 force-pushed the hotfix/fix-pagination-numbers-health branch from 1decd4c to 057902b Compare January 20, 2022 16:58

Improve hosts filtering and pagination e2e tests

e69fbba

arbulu89 force-pushed the hotfix/fix-pagination-numbers-health branch from 057902b to e69fbba Compare January 20, 2022 17:00

fabriziosestito suggested changes Jan 21, 2022

View reviewed changes

fabriziosestito reviewed Jan 21, 2022

View reviewed changes

fabriziosestito self-requested a review January 21, 2022 10:00

nelsonkopliku approved these changes Jan 21, 2022

View reviewed changes

fabriziosestito approved these changes Jan 24, 2022

View reviewed changes

arbulu89 merged commit bb29191 into trento-project:main Jan 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix health container numbers and pagination numbers #725

Fix health container numbers and pagination numbers #725

arbulu89 commented Jan 20, 2022 •

edited

nelsonkopliku left a comment

arbulu89 commented Jan 20, 2022

nelsonkopliku commented Jan 20, 2022 •

edited

arbulu89 commented Jan 20, 2022

nelsonkopliku commented Jan 20, 2022 •

edited

arbulu89 commented Jan 20, 2022

fabriziosestito Jan 21, 2022

arbulu89 Jan 21, 2022

arbulu89 Jan 21, 2022

fabriziosestito Jan 21, 2022

arbulu89 Jan 21, 2022

fabriziosestito Jan 21, 2022

fabriziosestito Jan 21, 2022

fabriziosestito commented Jan 21, 2022

nelsonkopliku commented Jan 21, 2022

nelsonkopliku left a comment

nelsonkopliku Jan 21, 2022

arbulu89 commented Jan 24, 2022

fabriziosestito commented Jan 24, 2022

Fix health container numbers and pagination numbers #725

Fix health container numbers and pagination numbers #725

Conversation

arbulu89 commented Jan 20, 2022 • edited

nelsonkopliku left a comment

Choose a reason for hiding this comment

arbulu89 commented Jan 20, 2022

nelsonkopliku commented Jan 20, 2022 • edited

arbulu89 commented Jan 20, 2022

nelsonkopliku commented Jan 20, 2022 • edited

arbulu89 commented Jan 20, 2022

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

fabriziosestito commented Jan 21, 2022

nelsonkopliku commented Jan 21, 2022

nelsonkopliku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbulu89 commented Jan 24, 2022

fabriziosestito commented Jan 24, 2022

arbulu89 commented Jan 20, 2022 •

edited

nelsonkopliku commented Jan 20, 2022 •

edited

nelsonkopliku commented Jan 20, 2022 •

edited