Request signaling server trial

[MorrisJobke]

This is a little form inside the talk settings to request a trial from the newly announced hosted signaling server by Struktur AG (see https://nextcloud.com/blog/open-sourcing-talk-back-end-rc-of-talk-9-brings-lots-of-improvements/).

As of now it looks like this:

Things that need to be done:

• select fields for language and country (based on the lists we have for the personal settings maybe)
• view for status of the requested trial - basically once the trial is requested it is in "pending" state and it just needs to display the current status (it's a single request to the API and then showing that content)
• fetching the current status (see above) and caching this on the Nextcloud server for some minutes
• dropping the status if the account was deleted on the server side (server here means Struktur's server) replaced by an "delete account button"
• periodically fetching the status if it is pending to check if the credentials are available and then fill them into the signaling server settings
• set the correct API server URL
• send admins notifications about updated signaling server credentials
• add warning text that the signaling server trial will replace added signaling servers
• button to delete account

Pending things to decide:

• limits are not returned by the hpb-service yet: Request signaling server trial #3620 (comment)
• 3 times similar code in the error handling of the API requests (but with small differences) Request signaling server trial #3620 (comment)
• modal for the "Delete" button Request signaling server trial #3620 (comment)

[fancycode]

Added some feedback and please change the wording from "trail" to "trial".

[MorrisJobke]

Any idea how to make the whole parsing mess in https://github.com/nextcloud/spreed/pull/3620/files#diff-a862c0a156d4c35c3617618db80227a4R85-R257 a lot more structured? Maybe @rullzer, @kesselb or @ChristophWurst? I feel really bad about this mess of code, but it's just quite some cases you want to have covered and all have different error handling 🙈 some throw exceptions, some return empty values, some require to call separate functions to fetch the error (looking at you json_decode)

And then you want to have a bunch of logging and a bunch of user facing error messages

And then there is basically the same again. I would like to extract those two big blocks (above and below // account is now properly requested) into private methods for better readability. Also the second part needs to be executed from a background job as well and thus would be nice to have in a service class.

[MorrisJobke]

Any idea how to make the whole parsing mess in https://github.com/nextcloud/spreed/pull/3620/files#diff-a862c0a156d4c35c3617618db80227a4R85-R257 a lot more structured? Maybe @rullzer, @kesselb or @ChristophWurst? I feel really bad about this mess of code, but it's just quite some cases you want to have covered and all have different error handling 🙈 some throw exceptions, some return empty values, some require to call separate functions to fetch the error (looking at you json_decode)

And then you want to have a bunch of logging and a bunch of user facing error messages

And then there is basically the same again. I would like to extract those two big blocks (above and below // account is now properly requested) into private methods for better readability. Also the second part needs to be executed from a background job as well and thus would be nice to have in a service class.

I moved them to a service class that translates all cases in one of two exceptions (one for API, connection or response errors the user can't solve and one for user problems like wrong input or non-public server).

Now the controller is far better readable and only does the actual logic it needs to do.

spreed/lib/Controller/HostedSignalingServerController.php

Lines 87 to 106 in 8d9214a

	try {
	$registerAccountData = new RegisterAccountData(
	$url,
	$name,
	$email,
	$language,
	$country
	);
	$accountId = $this->hostedSignalingServerService->registerAccount($registerAccountData);
	$accountInfo = $this->hostedSignalingServerService->fetchAccountInfo($accountId);
	$this->config->setAppValue('spreed', 'hosted-signaling-server-account', json_encode($accountInfo));
	} catch (HostedSignalingServerAPIException $e) { // API or connection issues
	return new DataResponse(['message' => $e->getMessage()], Http::STATUS_INTERNAL_SERVER_ERROR);
	} catch (HostedSignalingServerInputException $e) { // user solvable issues
	return new DataResponse(['message' => $e->getMessage()], Http::STATUS_BAD_REQUEST);
	}
	return new DataResponse($accountInfo);

[MorrisJobke]

Most is done. Only one disclaimer sentence needs to be added that signaling server is overwritten. And we need to check how to drop accounts from Nextcloud again (I would do that if the Struktur API returns that the account was deleted).

[MorrisJobke]

And we need to check how to drop accounts from Nextcloud again (I would do that if the Struktur API returns that the account was deleted).

Problem with this would be that then if the API is not reachable, but the webserver or proxy returns a 404 the account would also vanish. Maybe we should still add the option to delete the account? And only then remove it from the settings page?

[MorrisJobke]

How to run this locally:

• HPB service:
  • fetching the hpb-service code
  • comment the HTTPS check in api.go line 16: return NewValidationFailed("https_required")
  • run make build
  • run rm -rf accounts.sqlite3 && bin/spreed-hpbservice (that easily resets the state of the service on every new run)
• run two nextcloud instances under different ports (if the built in web server is used, because they can only serve one concurrent request, but the authentication round trip of the hpb-services requires 2 concurrent)
  • php -S 0.0.0.0:8001 and php -S 0.0.0.0:8002
• build and install Talk

Testing:

• go to admin talk settings and enter the IP and port of the other web server (the one you don't use to browse the admin settings)
• do further tests

[MorrisJobke]

@fancycode While doing a full test run I noticed that those fields are not yet returned.

@nickvergessen Either leave it as it is now (the limits are only shown in the UI if present anyways) or coordinate with @fancycode if they are added.

[MorrisJobke]

The error case handling is very similar for all three requests. This maybe refactored a bit, but I don't want to mix them too much as some status codes are different for the three requests. So it's up to you to move it all in one method or leave it that verbose.

[MorrisJobke]

There is some custom styling for the delete button (I used the one from the "enable unsupported app" in the app management).

[MorrisJobke]

Maybe we want a modal here to ask if the account really should be deleted. Up to you. Also after deletion a re-registration is not possible.

[ChristophWurst]

I think you can just do this in data()

[nickvergessen]

Works

Resolved

[MorrisJobke]

Thanks @nickvergessen 🚀