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

MGMT-13306 - Events pagination #2004

Merged
merged 3 commits into from Apr 26, 2023

Conversation

jgyselov
Copy link
Contributor

@jgyselov jgyselov commented Apr 3, 2023

https://issues.redhat.com/browse/MGMT-13306

Cluster events:

image

Host events:

image

Video:

events_pagination.mp4

@jgyselov jgyselov force-pushed the events_pagination branch 2 times, most recently from 02cdc04 to 2cdcc54 Compare April 18, 2023 09:55
@jgyselov jgyselov added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. OCM labels Apr 18, 2023
@jgyselov jgyselov force-pushed the events_pagination branch 4 times, most recently from a1c804a to 6eebb7c Compare April 19, 2023 10:16
@jgyselov jgyselov marked this pull request as ready for review April 19, 2023 10:16
@jgyselov jgyselov requested review from a team as code owners April 19, 2023 10:16
@ammont82
Copy link
Contributor

ammont82 commented Apr 21, 2023

@jgyselov I'm reviewing the PR and I've found this:

  • When you have all events enabled you see the number of every type of event (Info-53, Warning-17,...):
    Captura desde 2023-04-21 07-55-12

  • But then when you select one event type, the rest of events put 0:
    Captura desde 2023-04-21 07-55-03

@jgyselov
Copy link
Contributor Author

@jgyselov I'm reviewint the PR and I've found this:

  • When you have all events enabled you see the number of every type of event (Info-53, Warning-17,...):
  • But then when you select one event type, the rest of events put 0:

@ammont82 Yes. The numbers in the dropdown are taken directly from the API and they correspond to the number of events of each severity with the applied filters - that includes hosts, text, and severity... So if you select e.g. "Warning", the numbers next to the rest of the severities will be zero because all of them have been filtered out.

I am aware that these numbers worked differently before - maybe that was more intuitive? But we cannot get the previous behavior with the new API, so the way I see it, we have these options:

  1. Keep things as they are implemented here - if you select any severity filter, you will see 0 next to the unselected ones.
  2. Get rid of the numbers next to severities altogether - the Hosts filter doesn't have them, maybe we don't need them here either.

The severity counts (coming from the response headers) are also used to calculate the total number of events which is then used in the Pagination component. With the second option, there would be no need for the severity counts - and we should probably ask BE to simply send us one number (the total).

What option do you think is the better one? Or can you think of some other options?

cc @nirfarkas

@ammont82
Copy link
Contributor

@jgyselov I'm reviewint the PR and I've found this:

  • When you have all events enabled you see the number of every type of event (Info-53, Warning-17,...):
  • But then when you select one event type, the rest of events put 0:

@ammont82 Yes. The numbers in the dropdown are taken directly from the API and they correspond to the number of events of each severity with the applied filters - that includes hosts, text, and severity... So if you select e.g. "Warning", the numbers next to the rest of the severities will be zero because all of them have been filtered out.

I am aware that these numbers worked differently before - maybe that was more intuitive? But we cannot get the previous behavior with the new API, so the way I see it, we have these options:

  1. Keep things as they are implemented here - if you select any severity filter, you will see 0 next to the unselected ones.
  2. Get rid of the numbers next to severities altogether - the Hosts filter doesn't have them, maybe we don't need them here either.

The severity counts (coming from the response headers) are also used to calculate the total number of events which is then used in the Pagination component. With the second option, there would be no need for the severity counts - and we should probably ask BE to simply send us one number (the total).

What option do you think is the better one? Or can you think of some other options?

cc @nirfarkas

@jgyselov I think for user maybe is confusing this implementation. I thnk the best for me is to get rid of the numbers next to severities altogether. We need @nirfarkas for UX perspective

@jkilzi
Copy link
Contributor

jkilzi commented Apr 23, 2023

@jgyselov I'm reviewint the PR and I've found this:

  • When you have all events enabled you see the number of every type of event (Info-53, Warning-17,...):
  • But then when you select one event type, the rest of events put 0:

@ammont82 Yes. The numbers in the dropdown are taken directly from the API and they correspond to the number of events of each severity with the applied filters - that includes hosts, text, and severity... So if you select e.g. "Warning", the numbers next to the rest of the severities will be zero because all of them have been filtered out.
I am aware that these numbers worked differently before - maybe that was more intuitive? But we cannot get the previous behavior with the new API, so the way I see it, we have these options:

  1. Keep things as they are implemented here - if you select any severity filter, you will see 0 next to the unselected ones.
  2. Get rid of the numbers next to severities altogether - the Hosts filter doesn't have them, maybe we don't need them here either.

The severity counts (coming from the response headers) are also used to calculate the total number of events which is then used in the Pagination component. With the second option, there would be no need for the severity counts - and we should probably ask BE to simply send us one number (the total).
What option do you think is the better one? Or can you think of some other options?
cc @nirfarkas

@jgyselov I think for user maybe is confusing this implementation. I thnk the best for me is to get rid of the numbers next to severities altogether. We need @nirfarkas for UX perspective

@jgyselov you mentioned that the reason we do not get the other severities is related to the way the API works. So I guess that in the current state of the implementation, if I send an API request like: GET v2/events?...&severities=warning then the API returns only the Severity-Count-Warning header (perhaps all the other Severity-Count-* headers are also there but they are zeroed) . If that's correct, maybe we could ask @danmanor to change this behavior, so we always get all the counts the way it was before?
Anyway my take on this is this implementation is already better than what we had, it mitigates the issue that froze the browser and almost crushed the BE. So from that POV what we have here is already worthy. I see the counters issue as an UX enhancement.

jkilzi
jkilzi previously approved these changes Apr 23, 2023
@danmanor
Copy link

@jgyselov @jkilzi I think that if we want to have the severity count, it makes sense to have the filtered numbers, as it represents the counts of the events on the user side. I can change it to be the total counts, and provide an additional count of the filtered evetns in order to calculate offsets. LMK what you wish to do, btw looks good!

@jgyselov
Copy link
Contributor Author

If we wanted the behavior to be as close as possible to the way it was before, the severity counts would have to be

  • total counts of events of the respective severity across all possible pages,
  • with regards to the rest of the applied filters (hosts, cluster-level, message,...)
  • but not taking the severities filter itself into account

And we would need to receive information about the total number of events (with the severities filter applied) on top of that.


Personally, I am okay with either of the three options:

  1. Keep things are they are.
  2. Remove the counts.
  3. Change the severity counts to be as described above.

From the UI POV, these would require relatively small changes (if any) and could be done as a follow-up.

@jkilzi
Copy link
Contributor

jkilzi commented Apr 24, 2023

If we wanted the behavior to be as close as possible to the way it was before, the severity counts would have to be

  • total counts of events of the respective severity across all possible pages,
  • with regards to the rest of the applied filters (hosts, cluster-level, message,...)
  • but not taking the severities filter itself into account

And we would need to receive information about the total number of events (with the severities filter applied) on top of that.

Personally, I am okay with either of the three options:

  1. Keep things are they are.
  2. Remove the counts.
  3. Change the severity counts to be as described above.

From the UI POV, these would require relatively small changes (if any) and could be done as a follow-up.

Agreed, let's open a follow up enhancement task, detached from this epic. I think we should go with the 3rd option in the list above.

@jkilzi jkilzi added this to the v2.19 milestone Apr 24, 2023
@jgyselov jgyselov removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 25, 2023
ammont82
ammont82 previously approved these changes Apr 25, 2023
ammont82
ammont82 previously approved these changes Apr 25, 2023
@ammont82 ammont82 merged commit 489cc94 into openshift-assisted:master Apr 26, 2023
1 check passed
jtomasek pushed a commit to jtomasek/assisted-installer-ui that referenced this pull request May 9, 2023
* Events pagiantion - update API

* Events pagination

---------

Co-authored-by: Montse Ortega <ammont82@users.noreply.github.com>
jtomasek pushed a commit that referenced this pull request May 9, 2023
* Events pagiantion - update API

* Events pagination

---------

Co-authored-by: Montse Ortega <ammont82@users.noreply.github.com>
@jgyselov jgyselov deleted the events_pagination branch October 2, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants