OttPeterR / addon-babybuddy

BabyBuddy, wrapped into a Home Assistant addon
35 stars 13 forks source link

Notifications won't dismiss when using Ingress #31

Closed Intecpsp closed 7 months ago

Intecpsp commented 1 year ago

Screenshot_20230108-215149~3

When using the HA app and using ingress, the notifications never go away. Even hitting the x on them, they will just reappear the next time the screen is shown.

I don't have this behavior when navigating to the URL from a web browser, just utilizing ingress. Is this a possible cache issue navigating through the HA app?

Intecpsp commented 1 year ago

UPDATE: This is also an issue in the web version of HA. But still not an issue when navigating to Baby Buddy directly (http://homeassistant.local:8000/)

OttPeterR commented 1 year ago

I see it happening through ingress but not when accessing BB via the port, so I'm thinking it has something to do with the nginx server running inside the addon.

@cdubz - how do those notifications work in BB? and does it sound plausible that the frontend sending the 'x' signal is being dropped by a (my own) bad nginx configuration? Also is there a way to put in a timeout on them so they go away on their own?

cdubz commented 1 year ago

Notifications are just Django's built in messages framework. They are added to and removed from the session by Django core. The only thing the close button does it remove the HTML node on the current page. Not sure what could lead to this behavior... something messed up with session storage somehow.

tango2590 commented 1 year ago

Restarting the add-on clears the sticky notifications. In fact, logging out and logging back in (without reloading the add-on) also gets rid of them.

SpencerDub commented 1 year ago

This is also occurring for me. Logging out and in clears them, but then the new ones start accumulating again.

jwilters commented 1 year ago

I also have the same on my BB setup. Great that we have an work around by logging out and back in. Any update on a solution?

Frankenberrypi commented 1 year ago

Would it be possible to just disable the notifications?

OttPeterR commented 1 year ago

Thats a decent work around, maybe that could be worked into Baby Buddy v2? @cdubz

jvenborg commented 1 year ago

Firstly: THANK you for creating this addon. It is excellent!

I am experiencing the exact same issue on all devices - phone, web pr. IP or web pr. "homeassistant.local".

However, while the inability to dismiss them are of course of inconvenience, the fact that they stack on top of each other and on top of the screen is quite frustrating. Eventually, it means you end up having a complete screen with notifications (at least on mobile) instead of the actual interface. And you then have to scroll down. Attached a screen dump if helpful.

Could a very temporary solution be to stack them on button of screen instead on top? Thank you in advance! signal-2023-06-19-154528_002

tango2590 commented 1 year ago

When using ingress to bypass the login, manually logging out and back in does not clear the notifications. Only a restart of the add-on will clear them. While the notifications are handy, if a fix isn't a high priority, I'd rather they just be disabled in the meantime.

gioele-antoci commented 1 year ago

Anything we can do to help this fix along?

Thanks a lot :)

OttPeterR commented 1 year ago

I've tried a few things that haven't turned out to work for whatever issue is happening that isn't clearing the session cache of the notifications. My thought for what could be next is making a toggle in the Baby Buddy app itself (not this addon) which would stop all notifications all together.

viniciuscmartins commented 1 year ago

I found out that if you open the admin-> database admin and click on go back to website it will clear them. Not sure why this happens, but it's a workaround.

MrApplejuice commented 1 year ago

Hey all! I recognized another type of error with home assistant, namely that tags do not work an stumbled over this one when looking further.

I spent an hour yesterday on this and tracked the issue down to the home assistant ingress-server rewriting the cookie header, I think. It might also be the browser that incorrectly assigned the cookie to the homeassistant domain and then disallows cookie modification in the iframe that home assistant uses to delete the cookie for the ingress-path based on some security options set in the response headers... whatever the case, this is super-messy to debug!

However, I think I found a "robust and deployable" workaround that could be used for the home assistant addon: Add

MESSAGE_STORAGE = "django.contrib.messages.storage.session.SessionStorage"

to the config and switch over to session-storage for messages (default is "cookies"). Seems to work like a charm. Would that be acceptable?

synnack commented 1 year ago

I added MrAppleJuice's fix to my /app/babybuddy/babybuddy/settings/base.py in my container and it did solve the problem for me.

synnack commented 1 year ago

I also think it's neater to store it on the server side rather than in a never-ending cascade of cookies, so I'm pretty happy with the MrAppleJuice fix :)

OttPeterR commented 1 year ago

@MrApplejuice thats some good sleuthing you've done! I appreciate the work. I'll add that to a new release when I can get around to it or if you want to open a PR then I'll take care of it from there 😄

cdubz commented 1 year ago

Maybe we should make this part of the ENABLE_HOME_ASSISTANT_SUPPORT flag? Happy to merge a PR for that if it makes sense.

MrApplejuice commented 1 year ago

@MrApplejuice thats some good sleuthing you've done! I appreciate the work. I'll add that to a new release when I can get around to it or if you want to open a PR then I'll take care of it from there 😄

Please take it and run with it if you find the time. I am here at home a sick child right now and cannot do anything productive atm... Did not even mange to finish the tags-issue that persists with the current homeassistant-based deployments...

Maybe we should make this part of the ENABLE_HOME_ASSISTANT_SUPPORT flag? Happy to merge a PR for that if it makes sense.

I am not super-sure if that makes sense. I see why it would be handy to have those coupled, but it is a different type of setting. My suggestion would be to leave the two separate but then just add a custom config to the settings-directory, maybe as part of this repo, that sets all the options in one package... I leave the decision and details to you though :-)

synnack commented 1 year ago

I'm also happy to create a PR with just this setting always enabled. I don't see any downsides to be honest, this way notifications are stored in the session on the server side until dismissed, instead of in cookies.

MrApplejuice commented 1 year ago

I don't see any downsides to be honest, this way notifications are stored in the session on the server side until dismissed

Generally, I agree but I do not know how heavy-weight the database operations involved end up being. The nice thing about the browser-cookie is that is scales with the number of users. I think that storing it is in the server session will hammer the database for each and every notification. That is probably fine for the home-assistant hosted babybuddy instances, because it is likely that those are used in a setting where one instance serves one (small) family. However, I do not know if this would scale nicely in bugger setups, where for example, multiple baby buddy instance might share one central database.

I think this is for @cdubz to decide. My gut-feeling still tells me that one would want this in a home-assistant specific config file.

cdubz commented 1 year ago

Yeah I'm not keen on making this change generally. @MrApplejuice if you don't think this fits under ENABLE_HOME_ASSISTANT_SUPPORT I'd recommend having this addon generate and use it's own settings file so it can be tweaked as needed for specific things like this.

rmarskell commented 1 year ago

Generally, I agree but I do not know how heavy-weight the database operations involved end up being. The nice thing about the browser-cookie is that is scales with the number of users. I think that storing it is in the server session will hammer the database for each and every notification. That is probably fine for the home-assistant hosted babybuddy instances, because it is likely that those are used in a setting where one instance serves one (small) family. However, I do not know if this would scale nicely in bugger setups, where for example, multiple baby buddy instance might share one central database.

Typically when talking about session vs cookie storage in a HTTP server context, the session data is not stored in the app database, it's in files on the server the session was started on (note: it's possible the web server uses some kind of DB to store the sessions, but it's not in the context I think you're thinking of). You can kind of imagine them as server-side cookies managed by the web server itself. The user would get a cookie with their session ID and that ID would be used to find which session file has their data on subsequent requests.

The main benefit to cookie over session storage is when you scale out. If you had multiple servers, the session data would only exist on the server it was created on, so if the user got load-balanced to another one, they would send their session ID and get nothing back. Whereas with cookie storage, they can just pass all the data along with their request and the server doesn't have to go look anything up because it's all there in the cookie.

I would venture to say that it's unlikely people are load balancing their BB instances on HA so it's probably safe for that context, but I could also be wrong. 🙃

synnack commented 1 year ago

Even when load balancing between multiple frontend servers you'd have to sync user session state between django instances through a distributed memory database like redis, a few bytes more should have very little impact there.

matthieudevipa commented 1 year ago

First, thanks for all the great work !

I tried to implement the solution of @MrApplejuice but it does not seem to work on my side. I am new to this and I think I missing something. Currently I do the following :

I still have the issue when I go back to the user interface. Is there another step I need to execute ? Should I somehow "reload" the base.py file so the change is in effect ?

PS : I know that each time the container will restart or there will be an update, that change will disappear.

MrApplejuice commented 1 year ago

You will need to restart the container to reload the python-bits after . Or, of course, reload the python bits directly if you docker exec your way to the run-time. Just killing all python processes should do the trick, gunicorn will spin them up again.

I am not familiar with Portainer though, so I cannot comment on the lifetime of the container ecosystem you are using. For me a simple docker restart does not destroy the locally configured options inside the container though.

matthieudevipa commented 1 year ago

Killing all the python processes (by pkill -9 python ) worked for me. Thanks !

(but a docker restart or restarting the add-on through the HA UI do make me lose the modification in the base.py file)

nosecreek commented 10 months ago

Any more updates on this? Would go a long way to making Baby Buddy usable from Home Assistant.

Lockie85 commented 9 months ago

I'm also getting this issue with Baby Buddy installed as an Addon within Home Assistant. I'd love to know if there is a solution to this or if a fix is perhaps in the works? Many thanks for the awesome app!

Zanaras commented 8 months ago

While not an ideal workaround, I did work around this issue by no accessing it via ingress. That is, enabling the web interface port, setting it to one that was available (like 8000), restarting the add-on, waiting a few seconds for it to finish loading, then just accessing it directly: http://homeassistant.local:8000

Lockie85 commented 7 months ago

@OttPeterR really appreciate if you could find the time to resolve this issue. I work around the problem via "Database Admin > View Site". Which clears the notifications for me but unfortunately not those who are not flagged as "staff".

Many Thanks

OttPeterR commented 7 months ago

Going to make a new release with the fix made by @MrApplejuice

Lockie85 commented 7 months ago

Going to make a new release with the fix made by @MrApplejuice

You legend, thanks so much!