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

Assign turn server protocols to template #198

Merged

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Dec 19, 2016

Ref #197

Undefined index: turnServerProtocols at /var/www/nextcloud/apps/spreed/templates/settings-admin.php#35
Undefined index: turnServerProtocols at /var/www/nextcloud/apps/spreed/templates/settings-admin.php#38
Undefined index: turnServerProtocols at /var/www/nextcloud/apps/spreed/templates/settings-admin.php#41

Fix #151
Fix #185

@Ivansss @LukasReschke

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@codecov-io
Copy link

codecov-io commented Dec 19, 2016

Current coverage is 5.68% (diff: 33.33%)

Merging #198 into master will decrease coverage by 2.45%

@@            master      #198   diff @@
========================================
  Files           13        13          
  Lines         1008       968    -40   
  Methods         68        66     -2   
  Messages         0         0          
  Branches         0         0          
========================================
- Hits            82        55    -27   
+ Misses         926       913    -13   
  Partials         0         0          

Powered by Codecov. Last update 435b8ac...f93c8e5

@techc0de
Copy link

Hi,

I couldn't find the link for the solution.

@jancborchardt
Copy link
Member

@tx7 you could either manually apply the changes made – you can see them at https://github.com/nextcloud/spreed/pull/198/files
Or, if you got the app via git, just check out this branch by doing git checkout issue-197-assign-turn-server-protocol-to-template

@techc0de
Copy link

Thanks.

@jancborchardt
Copy link
Member

@tx7 can you test and confirm if it fixes the issue? :)

@techc0de
Copy link

I can't confirm it yet b/c I have another issue: “too many redirects”.
NC11 is broken. NC10 is best yet.

@jancborchardt
Copy link
Member

@tx7 does that issue appear with Nextcloud 11 or with the Video calls app? If with Nextcloud, please open an issue at https://github.com/nextcloud/server/issues/ to help us fix it :)

@techc0de
Copy link

techc0de commented Dec 28, 2016

“Too many redirects” is a NC issue.
I probably have to re-install Nextcloud tonight.

@techc0de
Copy link

The above solution fixed it.

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member Author

@Ivansss can you review this?

Signed-off-by: Joas Schilling <coding@schilljs.com>
Copy link
Member

@Ivansss Ivansss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@nickvergessen nickvergessen merged commit 50c73e4 into master Jan 10, 2017
@nickvergessen nickvergessen deleted the issue-197-assign-turn-server-protocol-to-template branch January 10, 2017 18:55
@techc0de
Copy link

techc0de commented Jan 11, 2017

Hi guys,

Just updated to NC11.0.1, and using the spreed video call from Github (https://codeload.github.com/nextcloud/spreed/zip/master), but error persist.

Undefined index: turnServerProtocols at /media/54bf67db-da31-4c50-bb3c-27140944b223/www/nextcloud/apps/spreed/templates/settings-admin.php#41
Undefined index: turnServerProtocols at /media/54bf67db-da31-4c50-bb3c-27140944b223/www/nextcloud/apps/spreed/templates/settings-admin.php#38

@nickvergessen
Copy link
Member Author

We will publish a new version to the app store soon.
Since it doesn't happen here, I think you might have made some invalid changes ;)

@techc0de
Copy link

This is inconsistent.
I thought the package from Github should be the latest updates.

@nickvergessen
Copy link
Member Author

Yes, but also your log is impossible, because line 38 is <?php p($l->t('UDP and TCP')) ?> and therefor can not cause Undefined index: turnServerProtocols at /media/54bf67db-da31-4c50-bb3c-27140944b223/www/nextcloud/apps/spreed/templates/settings-admin.php#38

@techc0de
Copy link

Do I need to change the STUN server's settings or left it by default?

@nickvergessen
Copy link
Member Author

You only need to change stuff, when it's not working with the default values or you don't trust our service.

@techc0de
Copy link

No offense, but I don't know if I should trust 3rd party service these days.
Too many hackers, and spy.....

@nickvergessen
Copy link
Member Author

Yeah well that's all we can say.

STUN is not very critical, it's just used for IP resolution before connecting.
TURN (which has no default) pipes all the audio and video through it, so that is more critical to privacy and hacking.

@techc0de
Copy link

techc0de commented Jan 14, 2017

marcoambrosini pushed a commit that referenced this pull request Oct 9, 2019
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.

5 participants