Skip to content

Conversation

daif
Copy link

@daif daif commented Dec 6, 2021

Add CustomSMS a generic provider to send SMS using any webservice gateway

Copy link
Collaborator

@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.

Can you fix the conflict?

if(!filter_var($url, FILTER_VALIDATE_URL))
{
$output->writeln('Invalid URL '.$url);
goto ReTypeUrl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

goto isn't good, maybe will be good use methods to do this using recursion.
Using specific method also will make more easy implement unit tests to this command;

$methodQuestion = new Question('Please enter the web service method (GET or POST): ');
$method = (string)$helper->ask($input, $output, $methodQuestion);

if(strtolower($method) != 'get' && strtolower($method) != 'post')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is a good practice use !== than !=


## CustomSMS
URL: any endpoint
Stability: Experimental
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that will be good implement examples of supported API payload with GET and POST.

For example:

Provider with GET:
Need accept URL on this format: <base_url_provider>?<mobile_number_parameter>=<number> ....

Provider with POST:
...

Mandatory parameters:

base_url_provider the base url of provider
mobile_number_parameter phone number parameter, get from provider, by example: phone, number, num, etc

@vitormattos
Copy link
Collaborator

Apply lint pattern with php-cs

Run the command composer cs:fix to check if have fixes to do on files that you changed. After run composer cs:fix, only commit the files that you changed.

killerbees19 added a commit to froonix/twofactor_gateway that referenced this pull request Jun 29, 2022
* Small fixes and improvements.
* Replace goto statements with while-loop.
* Automatic changes from: composer cs:fix

Signed-off-by: Christian Schrötter <[email protected]>
@killerbees19
Copy link
Contributor

@daif Take a look at this branch…

https://github.com/froonix/twofactor_gateway/tree/pr-473

I've cherry-picked your commit on top of the current master branch and fixed all conflicts. Feel free to use it at your PR.

@daif @vitormattos I've proposed some changes for the goto-issue and commited as 5d20f4d. What do you think about this alternative approach?

I've not tested these code changes or PR, but I really like the idea of a generic custom provider. Feel free to use something (or nothing) from my branch pr-473. 😎

@vitormattos
Copy link
Collaborator

@killerbees19 this PR is very old. I suggest to you to send a new PR using your branch and solving the last reviews comments if @daif don't return to work in this PR.
What you think about this?

@killerbees19
Copy link
Contributor

I suggest to you to send a new PR using your branch and solving the last reviews comments if @daif don't return to work in this PR. What you think about this?

Good idea. I'll wait some days/weeks for @daif to react on his original PR. If there's no new activity, I'll test the feature and open a new PR with my changes.

(Feel free to ping me with @killerbees19 if there's no activity after some time. Chances are high that I'll forget it 😁)

@vitormattos
Copy link
Collaborator

I already did a ping February.

@killerbees19
Copy link
Contributor

killerbees19 commented Jun 29, 2022

I already did a ping February.

Good point 😆

Anyways, I've no time at the moment. Maybe next weekend...

@iliessens
Copy link

Hi all,

Has there been any progress on this front? We would love to use the custom SMS feature to send 2FA texts through our own little gateway (Teltonika TRB140).
I tried using the proposed code, but the setup command through occ always fails with this error:

An unhandled exception has been thrown:
Error: Undefined constant OCA\TwoFactorGateway\AppInfo\Application::APP_NAME in /var/www/nextcloud/apps/twofactor_gateway/lib/Service/Gateway/SMS/Provider/CustomSMSConfig.php:61
Stack trace:
#0 /var/www/nextcloud/apps/twofactor_gateway/lib/Command/Configure.php(211): OCA\TwoFactorGateway\Service\Gateway\SMS\Provider\CustomSMSConfig->setUrl()
#1 /var/www/nextcloud/apps/twofactor_gateway/lib/Command/Configure.php(92): OCA\TwoFactorGateway\Command\Configure->configureSms()
#2 /var/www/nextcloud/3rdparty/symfony/console/Command/Command.php(255): OCA\TwoFactorGateway\Command\Configure->execute()
#3 /var/www/nextcloud/3rdparty/symfony/console/Application.php(1009): Symfony\Component\Console\Command\Command->run()
#4 /var/www/nextcloud/3rdparty/symfony/console/Application.php(273): Symfony\Component\Console\Application->doRunCommand()
#5 /var/www/nextcloud/3rdparty/symfony/console/Application.php(149): Symfony\Component\Console\Application->doRun()
#6 /var/www/nextcloud/lib/private/Console/Application.php(211): Symfony\Component\Console\Application->run()
#7 /var/www/nextcloud/console.php(100): OC\Console\Application->run()
#8 /var/www/nextcloud/occ(11): require_once('...')

I am not familiar enough with the codebase to figure out why this happens.
So, it would be awesome if this functionality could make it to the Nextcloud app store sometime 😃

Best regards,
Imre

@vitormattos
Copy link
Collaborator

/rebase

@Neustradamus
Copy link

@daif: Have you progressed on it with Nextcloud team?

@vitormattos
Copy link
Collaborator

Thank you for your contribution and the time you put into this PR.

Since then, I’ve taken over as maintainer of this app, and it has gone through a major refactor that changed much of the internal architecture and file structure. Because of that, this PR can’t be merged in its current form.

If you’re still interested in this contribution, please feel free to rebase or open a new PR against the current main branch.

Your effort is very valuable, even if we can’t merge this directly, it helps us move the app forward. Thanks again!

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.

5 participants