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 won't dismiss when using Ingress #31

Closed
Intecpsp opened this issue Jan 9, 2023 · 33 comments · Fixed by #70
Closed

Notifications won't dismiss when using Ingress #31

Intecpsp opened this issue Jan 9, 2023 · 33 comments · Fixed by #70

Comments

@Intecpsp
Copy link

Intecpsp commented Jan 9, 2023

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
Copy link
Author

Intecpsp commented Jan 9, 2023

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
Copy link
Owner

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
Copy link

cdubz commented Jan 10, 2023

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
Copy link

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
Copy link

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

@jwilters
Copy link

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
Copy link

Would it be possible to just disable the notifications?

@OttPeterR
Copy link
Owner

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

@jvenborg
Copy link

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
Copy link

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
Copy link

Anything we can do to help this fix along?

Thanks a lot :)

@OttPeterR
Copy link
Owner

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
Copy link

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
Copy link
Contributor

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
Copy link

synnack commented Sep 5, 2023

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
Copy link

synnack commented Sep 5, 2023

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
Copy link
Owner

@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
Copy link

cdubz commented Sep 5, 2023

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
Copy link
Contributor

MrApplejuice commented Sep 6, 2023

@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
Copy link

synnack commented Oct 1, 2023

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
Copy link
Contributor

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
Copy link

cdubz commented Oct 6, 2023

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
Copy link

rmarskell commented Oct 16, 2023

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
Copy link

synnack commented Nov 12, 2023

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
Copy link

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 connect with Portainer to the babybuddy container.
  • I add the line at the end to the file /app/babybuddy/babybuddy/settings/base.py mentioned by @synnack
  • I save.

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
Copy link
Contributor

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
Copy link

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
Copy link

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

@Lockie85
Copy link

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
Copy link

Zanaras commented Mar 6, 2024

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
Copy link

@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
Copy link
Owner

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

@Lockie85
Copy link

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

You legend, thanks so much!

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

Successfully merging a pull request may close this issue.