Skip to content

Add OVH SMS gateway#316

Merged
ChristophWurst merged 5 commits into
nextcloud:masterfrom
piti-diablotin:master
Feb 3, 2020
Merged

Add OVH SMS gateway#316
ChristophWurst merged 5 commits into
nextcloud:masterfrom
piti-diablotin:master

Conversation

@piti-diablotin
Copy link
Copy Markdown
Contributor

@piti-diablotin piti-diablotin commented Jan 17, 2020

  • Add the ovh api to be able to use the gateway sms with ovh (website)
  • Documentation added of OVH in the Administration file.
  • It has been tested with the test function and on my testing setting (NC16)

PS:Sorry for the different names in the commits I did not take care of it.

@dependabot-preview @piti-diablotin
Bump css-loader from 3.4.1 to 3.4.2
2772cbe 
Bumps [css-loader](https://github.com/webpack-contrib/css-loader) from 3.4.1 to 3.4.2.
- [Release notes](https://github.com/webpack-contrib/css-loader/releases)
- [Changelog](https://github.com/webpack-contrib/css-loader/blob/master/CHANGELOG.md)
- [Commits](webpack/css-loader@v3.4.1...v3.4.2)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>
@ChristophWurst
Copy link
Copy Markdown
Member

ChristophWurst commented Jan 20, 2020

Thanks a lot for this 🙏

Unfortunately that ovh/ovh package pulls in Guzzle. This will cause conflicts with server and other apps. The upstream package has to sort out ovh/php-ovh#34, then we can integrate ovh/ovh in Nextcloud 19 in combination with https://packagist.org/packages/christophwurst/nextcloud-http-client.

@ChristophWurst ChristophWurst mentioned this pull request Jan 20, 2020
Open
@ChristophWurst
Copy link
Copy Markdown
Member

ChristophWurst commented Jan 20, 2020

Let's see if this can be driven forward ovh/php-ovh#41 (comment) :)

@ChristophWurst
Copy link
Copy Markdown
Member

ChristophWurst commented Jan 20, 2020

Code looks good btw :)

@piti-diablotin
Copy link
Copy Markdown
Contributor Author

piti-diablotin commented Jan 21, 2020

Thanks for the comments.

Since for the provider to work, only one GET and one POST requests are used, do you think I could rewrite the authentication mechanism by hand with the IClientService and get ride of ovh/ovh ?
Pros are we do not depend on ovh/ovh anymore
Cons are need to rewrite the authentication and may not be maintained (but seeing their responsiveness ...) ?
Doc is available here

@ChristophWurst
Copy link
Copy Markdown
Member

ChristophWurst commented Jan 21, 2020

Sure! That is also what we use for most other gateway integrations :)

We can also migrate to the updated ovh package later 😉

@piti-diablotin
Copy link
Copy Markdown
Contributor Author

piti-diablotin commented Jan 21, 2020 •
edited
Loading

I have update the Provider with the IClientService from NC.
I have tested with test function provided by the app and it seems working.
The ovh/ovh Api are commented but still working.
I also have removed the composer package

Would it be possible to merge it now ?

@piti-diablotin
Copy link
Copy Markdown
Contributor Author

piti-diablotin commented Jan 21, 2020

I'm sorry I can't get the DCO despite the git commit -s !!! What am I doing wrong ?

ChristophWurst
ChristophWurst requested changes Jan 21, 2020
View reviewed changes
Comment thread lib/Service/Gateway/SMS/Provider/Ovh.php Outdated
Comment thread lib/Service/Gateway/SMS/Provider/Ovh.php Outdated
Comment thread lib/Service/Gateway/SMS/Provider/Ovh.php Outdated
Comment thread lib/Service/Gateway/SMS/Provider/Ovh.php Outdated
@ChristophWurst
Copy link
Copy Markdown
Member

ChristophWurst commented Jan 21, 2020

I'm sorry I can't get the DCO despite the git commit -s !!! What am I doing wrong ?

The sign-off has to be present on all of your commits. I would recommend to use fixup's during development and do a final squash before the merge so it's just one commit in the end.

Jordan Bieder and others added 2 commits January 21, 2020 22:34
@piti-diablotin
Ovh SMS gateway integration
e2942b1 
- Add configuration class
- Add Provider class

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>

Integrate Ovh gateway

- add php API with composer
- install the new dependencies
- Add configuration for ovh
- Update ProviderFactory for ovh

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>

Debug Ovh sms gateway.
Has been tested with the test function

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>

Improve documentation since it seems not that easy

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>

Update Admin Documentation.md

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>

Update Admin Documentation.md

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>

Update Admin Documentation.md

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>

Fix php syntax

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>

Bump css-loader from 3.4.1 to 3.4.2

Bumps [css-loader](https://github.com/webpack-contrib/css-loader) from 3.4.1 to 3.4.2.
- [Release notes](https://github.com/webpack-contrib/css-loader/releases)
- [Changelog](https://github.com/webpack-contrib/css-loader/blob/master/CHANGELOG.md)
- [Commits](webpack/css-loader@v3.4.1...v3.4.2)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

[tx-robot] updated from transifex

[tx-robot] updated from transifex

Add sms77 to help and fix case selection string

Signed-off-by: Olav Seyfarth <nursoda@users.noreply.github.com>

Update OVH sms provider to remove ovh/ovh dependency

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>

Update OVH sms provider to remove ovh/ovh dependency

Update ovh endpoints

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>

Fix typos

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>

Fix typos bis

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>

Fix POST request

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>

Remove ovh/ovh with composer

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>

Stupid commit to get the DCO ?!

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>

Add requested corrections

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>

Fix indentation

Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>
@piti-diablotin
Merge branch 'master' into master
fa06de3
@piti-diablotin
Copy link
Copy Markdown
Contributor Author

piti-diablotin commented Jan 21, 2020 •
edited
Loading

I squashed everything. But Travis fails :(
At least DCO seems fine.
Why would I have App "Two-Factor Gateway" cannot be installed because it is not compatible with this version of the server. whereas I did not changed anything in the setting of the app ... ? Did I messed up something ? the composer.lock file ?

@piti-diablotin
What happened to this file ?!
10c002a 
Signed-off-by: Jordan Bieder <jordan.bieder@geduld.fr>
@ChristophWurst
Copy link
Copy Markdown
Member

ChristophWurst commented Jan 23, 2020 •
edited
Loading

We have to fix CI as the nextcloud server versions changed recently. Thus the incompatibility failure.

It's as simple as this: nextcloud/mail#2523. So, if you do not mind, please open a separate PR with those changes, then we can rebase this branch here and CI should be happy again :)

@ChristophWurst
Copy link
Copy Markdown
Member

ChristophWurst commented Jan 23, 2020

What happened to this file ?!

LOL at this clickbait title. Made me want to know what happened :)

@piti-diablotin
Copy link
Copy Markdown
Contributor Author

piti-diablotin commented Jan 23, 2020

LOL at this clickbait title. Made me want to know what happened :)

Just my eyes that were not in front of the holes :)

See #320 for the version bump.

@piti-diablotin
Copy link
Copy Markdown
Contributor Author

piti-diablotin commented Jan 23, 2020

How should I make tests to improve code coverage ?

@piti-diablotin
Copy link
Copy Markdown
Contributor Author

piti-diablotin commented Feb 3, 2020

Hi,
Could we proceed ?

@ChristophWurst
Copy link
Copy Markdown
Member

ChristophWurst commented Feb 3, 2020

How should I make tests to improve code coverage ?

You can see https://github.com/nextcloud/twofactor_gateway/tree/master/tests for reference. But generally this app doesn't have many tests, unfortunately.

ChristophWurst
ChristophWurst approved these changes Feb 3, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks good!

@ChristophWurst ChristophWurst merged commit dd953d2 into nextcloud:master Feb 3, 2020
@ChristophWurst ChristophWurst mentioned this pull request May 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@piti-diablotin @ChristophWurst