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

notifications on notifications drawer are cleaned after browser hard refresh for non admin users #1338

Open
sgratch opened this issue Dec 3, 2020 · 9 comments

Comments

@sgratch
Copy link
Member

sgratch commented Dec 3, 2020

All notifications on notifications drawer are vanished after hard refresh or logout/login for non admin user.

For reproducing:

  1. login as a specific non admin user to web-ui
  2. try to generate few backend generated notifications (e.g. by running a diskless vm).
  3. Check that the expected notifications appear on notifications drawer and counter badge reflects the right number.
  4. logout/login or run hard refresh (ctrl+f5).

Expected result:
All notifications should still appear on notifications drawer and counter badge should stay the same as before the logout/refresh

Actual result:
The notifications drawer is empty.

Note that this issue is not reproduced for admin user, i.e. all notifications are kept as it should after refresh/logout.

@sgratch sgratch changed the title Notiticaitons on notifications drawer are vanished after browser hard refresh for non admin users notifications on notifications drawer are cleaned after browser hard refresh for non admin users Dec 3, 2020
@bamsalem
Copy link
Contributor

@sgratch
From what I found it seems like the issue is permissions/REST issue and cannot be fixed from the web UI.
I queried the REST using REST client the response to t1 user is filtered (t1 has no permissions to fetch unfiltered data) and not includes some of the events/messages.
The filter is the difference between those queries:
https://github.com/oVirt/ovirt-web-ui/blob/master/src/ovirtapi/transport.js#L69

In the response to admin user I could find the disappeared events of t1 user.
I can share the json response files if you want so.

@sgratch
Copy link
Member Author

sgratch commented Jan 18, 2021

@sjd78 based on a dup old issue #971 - do you remember the design for events on web-ui?

@sjd78
Copy link
Member

sjd78 commented Feb 23, 2021

@sjd78 based on a dup old issue #971 - do you remember the design for events on web-ui?

So yeah - the notifications on web-ui are a combination of events from rest and things that the app pushes in there. The events are fetched once at login time. I don't think they're ever fetched again. Also, any notifications added by the front end App itself are 100% ephemeral. I'm not sure the cost to build this out would even have a reasonable ROI. For example, what should happen if a user is logged in on 2 browsers and clears notifications on 1 of the browsers?

To be able to rebuild the event list on App refresh (or on login from another browser or private tab etc)...

  • All events generated by the front end would need to be stored somewhere, probably as events on rest to be shared across App sources
  • An event fetcher / refresher / manager would need to be running to keep events in sync with the 1 event source
  • Probably other things I can't think of right now

@sjd78
Copy link
Member

sjd78 commented Feb 23, 2021

@sgratch
From what I found it seems like the issue is permissions/REST issue and cannot be fixed from the web UI.
I queried the REST using REST client the response to t1 user is filtered (t1 has no permissions to fetch unfiltered data) and not includes some of the events/messages.
The filter is the difference between those queries:
https://github.com/oVirt/ovirt-web-ui/blob/master/src/ovirtapi/transport.js#L69

The filter is set true for non-admin users and false for admin users (most of the time at least - there is a system setting to also filter admin users that tends to get set in large envs like brq-dev). But you're correct, regular users must include the Filter: true header to avoid permission errors.

In the response to admin user I could find the disappeared events of t1 user.
I can share the json response files if you want so.

Yeah visibility of the events matter. After the events are fetched, I believe they are also marked as viewed. In that way, if you login once, events are loaded and marked as "read", the next login will not load those same events. If there are over 100 events (I think 100 is the fetch size at login), then it can look like it is re-fetching the events, but it is really fetching older events. I'd have to look at the code again to triple check it works that way. That was always a gap that got ignored.

@sgratch
Copy link
Member Author

sgratch commented Feb 23, 2021

So yeah - the notifications on web-ui are a combination of events from rest and things that the app pushes in there. The events are fetched once at login time. I don't think they're ever fetched again. Also, any notifications added by the front end App itself are 100% ephemeral. I'm not sure the cost to build this out would even have a reasonable ROI. For example, what should happen if a user is logged in on 2 browsers and clears notifications on 1 of the browsers?

@sjd78 it's accepted for now as a known limitation to avoid persistence of notifications added by the web-ui App itself since it's really more complicated to implement.
This issue is more for handling the backend events for non-admin users:

  1. All backend events on web-ui are cleaned after hard refresh except old ones - this is wrong since the user still wants to see those. It's ok to start with an empty drawer after login but not after refresh. We need to think if/how to handle this.
  2. Not all user error events generated on backend are even displayed by web-ui. Some of them are missed/filtered out from start.

@bamsalem
Copy link
Contributor

@sjd78

Yeah visibility of the events matter. After the events are fetched, I believe they are also marked as viewed. In that way, if you login once, events are loaded and marked as "read", the next login will not load those same events.

From what I saw it's not marked as read, the same errors fetched every refresh/ login.

If there are over 100 events (I think 100 is the fetch size at login), then it can look like it is re-fetching the events, but it is really fetching older events. I'd have to look at the code again to triple check it works that way. That was always a gap that got ignored.

I tested that and I saw the events in any refresh/login have the same ids, so it's probably works different.

@bamsalem
Copy link
Contributor

@sgratch
In the session we had earlier this week we raised few questions, so now I'll share with you what I found:

  1. Filter: true Header: I tried to identify if the problem is in the backend filter so I queried (as an admin user) the events REST call via web ui and REST client, in the first query the filter header value was true and in the second it was false and I found that in both (REST client and web UI there are missing events between the second and the first response:
    Filter: false:
    filterFalse

Filter: true:
filterTrue

  1. Pools events: I tried to generate a backend pool event but the only event I could generate was related to the pool VM and then it wasn't affected by the filter.

IMO it's problem with permissions or with the backend filter...

@sgratch
Copy link
Member Author

sgratch commented Feb 25, 2021

In the session we had earlier this week we raised few questions, so now I'll share with you what I found:

1. `Filter: true` Header: I tried to identify if the problem is in the backend filter so I queried (as an admin user) the events REST call via web ui and REST client, in the first query the filter header value was true and in the second it was false and I found that in both (REST client and web UI there are missing events between the second and the first response:

This is very strange since the same type of event is sometimes filtered out and sometimes not. All those events appear on webadmin audit log?

2. Pools events: I tried to generate a backend pool event but the only event I could generate was related to the pool VM and then it wasn't affected by the filter.

That sound ok to me.

IMO it's problem with permissions or with the backend filter...

If an event generated by admin user appears on webadmin, appears on web-ui without the filter and doesn't appear with the filter then we need to understand why it is filtered out by the backend. It might be related to what Scott mentioned above.

@bamsalem
Copy link
Contributor

bamsalem commented Mar 2, 2021

@sgratch

This is very strange since the same type of event is sometimes filtered out and sometimes not. All those events appear on webadmin audit log?

Yes, it's not disappeared from the webAdmin log.

If an event generated by admin user appears on webadmin, appears on web-ui without the filter and doesn't appear with the filter then we need to understand why it is filtered out by the backend. It might be related to what Scott mentioned above.

I tested that is the same events by the id so I'm not sure what the cause but it's defiantly not dismissed on the refresh...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants