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

fix(AdminSettings): test Websocket connection #13973

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Dec 10, 2024

☑️ Resolves

Allows to test websocket connection with HPB in Admin settings (before users will face it in Talk interface): client attempts to establish new websocket, then:

  • clean close on success
  • throw error if any

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

Small screen Wide screen
image image
image image

🚧 Tasks

  • Handle NC <-> HPB servers time sync issue
  • Handle other cases that could be checked?

🏁 Checklist

  • 🌏 Tested with different browsers / clients:
    • Chromium (Chrome / Edge / Opera / Brave)
    • Firefox
    • Safari
    • Talk Desktop
    • Not risky to browser differences / client

@Antreesy Antreesy added this to the 🖤 Next Major (31) milestone Dec 10, 2024
@Antreesy Antreesy self-assigned this Dec 10, 2024
@Antreesy Antreesy changed the title fix(AdminSettings): allow to retry server checks fix(AdminSettings): test Websocket connection Dec 11, 2024
@Antreesy Antreesy force-pushed the fix/noid/signaling-errors branch from 60416d3 to 42c707f Compare December 11, 2024 15:57
@Antreesy
Copy link
Contributor Author

I'm lacking knowledge on how else this connection could fail (and how to reproduce it), so wouldn't mind a second opinion here
cc @danxuliu @fancycode @SystemKeeper

@fancycode
Copy link
Member

Another thing you could test here is sending a hello request after the connection was established. This will verify that the backend url and shared secret are correctly configured in the signaling server.

@fancycode
Copy link
Member

Regarding testing: this will also fail if the signaling server url is incorrect and the request can't be upgraded to a WebSocket. This can be seen in the response (could be 404 for unknown urls or something like 400 for valid urls that don't support WebSocket), but I'm not sure if you get all the details from the JavaScript API.

@fancycode
Copy link
Member

Another thing you could test here is sending a hello request after the connection was established. This will verify that the backend url and shared secret are correctly configured in the signaling server.

As I had a HPB setup support request from a customer yesterday where the backend url was a problem, this would be really great to test from the admin settings. Maybe you could also show the backend url ithat Talk will send to the signaling server in the UI so the user can compare with the configuration of the signaling server (see https://github.com/strukturag/nextcloud-spreed-signaling/blob/0a04ea290969cfe1ebc3ef3aaeeae1bdc49ea552/server.conf.in#L125-L126).

@Antreesy
Copy link
Contributor Author

Antreesy commented Dec 18, 2024

@Antreesy Antreesy force-pushed the fix/noid/signaling-errors branch from 42c707f to 4ddb6a4 Compare December 18, 2024 13:51
@Antreesy Antreesy requested a review from danxuliu December 18, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants