Skip to content

Show users as offline if their browser view is hidden

Jano requested to merge redo_chat_server into master

Closes #892 (closed)

What does this MR do?

The current online/offline status of beta only checks, if users have the website open with a WebSocket somewhere. Because many users keep the site open in the background, especially on mobile devices, they might miss push notifications this way. That's why this merge request makes people also appear offline and the application send notifications when all of their browser tabs are in the background.

This MR also ports/refactors our "chat" server in TypeScript. This was necessary, because the structure of the old server was so buried in functions and callbacks that there was no possible way understanding the sturcture to change it, so I broke it into smaller parts. Also, the server now uses current libraries, as Restify to handle requests by our backend, Tedis as a Redis client written in TypeScript, and the integration tests use Superagent instead of the deprecated request functionality.

How confident are you it won't break things if deployed?

This is breaking. I changed the API to which our backend server talks and made it REST conforming. As we have only one chat server for beta and production, we need to either hotfix production's WebSocketConnection.php to talk to the new API, or we need to run two instances of the chat server.

The API to the clients is widely untouched, however, the options JSON has been surrounded by quotation marks in the original implementation, therefore has not been automatically converted to an object in JavaScript and needed to be parsed using JSON.parse. This call isn't necessary any more (=breaks the thing). I don't know how our native apps handle that, as they're written in Java, there's probably no automatic parsing of JSON objects?

How to deploy

Because the old API was very confusing and hard to understand (even with the overly long Docblock), I changed the API and made it REST conforming.

As we have only one instance of the "chat" server running, when deploying this to beta, the production chat server needs to be replaced. This way, two commit need to be hotfixed into production in order to make the backend talk the new API:

e2b79f35: This makes the WebSocketConnection talk to the new API. The API of WebSocketConnection remains unchanged, so it shouldn't cause problems to cherry-pick this. Because WebSocketConnection now needs a Guzzle client injected from Symfony, deleting the Symfony cache might be necessary.

4434d0ee: This one changes the API once again. Sorry. I realized that we use plural forms in most of our REST resource names, and although we don't have a convention here, I think we should have, and plural is not only used by the majority of our endpoints, but also makes more sense to me.

1c7a5f7a: This removes the JSON.parse call for the WebSocket payloads's 'o' parameter in the client. Because 'o' is now correctly formatted as JSON object, this is not necessary any more.

How to test

  1. Checkout branch locally
  2. Login as foodsaver
  3. Login as another foodsaver in another browser/private window
  4. Look at each others profiles
  5. Select another tab in one browser or minimize the window, reload the page in the other browser and look at the online status again, see that it changed
  6. Protip: You can see the online status directly using http://localhost:11338/users/{id}/is-online

Checklist

  • added a test, or explain why one is not needed/possible...
  • no unrelated changes
  • asked someone for a code review
  • joined #foodsharing-beta channel at https://slackin.yunity.org
  • added an entry to CHANGELOG.md (description, merge request link, username(s))
  • Once your MR has been merged, you are responsible to update the #foodsharing-beta Slack channel about what has been changed here. They will test your work in different browsers, roles or other settings
Edited by Jano

Merge request reports