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

Feature/add sipgate sms support #482

Merged

Conversation

rotdrop
Copy link
Contributor

@rotdrop rotdrop commented Feb 14, 2022

No description provided.

@rotdrop rotdrop force-pushed the feature/add-sipgate-sms-support branch from 56c235d to 2bb10d0 Compare February 14, 2022 18:28
@rotdrop
Copy link
Contributor Author

rotdrop commented Feb 14, 2022

I known, #454, still just for the record ...

@rotdrop rotdrop force-pushed the feature/add-sipgate-sms-support branch from 2bb10d0 to c16146b Compare February 14, 2022 19:41
Copy link
Contributor

@vitormattos vitormattos left a comment

Choose a reason for hiding this comment

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

The Signal changes isn't of other PR? Maybe split in two PR will be more easy to do review.

lib/Service/Gateway/Signal/Gateway.php Outdated Show resolved Hide resolved
lib/Service/Gateway/Signal/Gateway.php Outdated Show resolved Hide resolved
@@ -91,7 +91,7 @@ public function startSetup(IUser $user, string $gatewayName, string $identifier)
try {
$gateway->send($user, $identifier, "$verificationNumber is your Nextcloud verification code.");
} catch (SmsTransmissionException $ex) {
throw new VerificationTransmissionException('could not send verification code');
throw new VerificationTransmissionException('could not send verification code', $ex->getCode(), $ex);
Copy link
Contributor

Choose a reason for hiding this comment

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

On what place the app catch the 2th and 3th argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The log-viewer will display a hierarchy of exception. This is handy for debugging. Otherwise you just see the top-level exception which provides zero information on what was going wrong.

Copy link
Contributor

@vitormattos vitormattos left a comment

Choose a reason for hiding this comment

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

Create integration tests

Maybe, will be a good time to implement integration tests mocking HTTP request to the gateways.

This is a good package to do this: https://packagist.org/packages/donatj/mock-webserver

Was easy going. Most difficult thing was to figure out the necessary ACL
for the access-token at the sipgate-site (sessions:sms:write).

Signed-off-by: Claus-Justus Heine <himself@claus-justus-heine.de>
@rotdrop rotdrop force-pushed the feature/add-sipgate-sms-support branch from c16146b to 7f1e999 Compare February 14, 2022 19:52
@vitormattos
Copy link
Contributor

I think that you created this branch over branch of PR #481

@rotdrop
Copy link
Contributor Author

rotdrop commented Feb 14, 2022

I think that you created this branch over branch of PR #481

Yes; I already stated this as reply to your comment and also rebased to the master branch.

@vitormattos vitormattos merged commit 1ee18f9 into nextcloud:master Feb 14, 2022
@vitormattos
Copy link
Contributor

@rotdrop can you put documentation to this on doc/Admin Documentation.md ?

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 this pull request may close these issues.

2 participants