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.

Sign up for GitHub

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

Jump to bottom

Add OVH SMS gateway #316

Merged
merged 5 commits into from
Feb 3, 2020
Merged

Add OVH SMS gateway #316

merged 5 commits into from
Feb 3, 2020

Conversation

piti-diablotin
Copy link
Contributor

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

@ChristophWurst
Copy link
Member

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
Member

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

@ChristophWurst
Copy link
Member

Code looks good btw :)

@piti-diablotin
Copy link
Contributor Author

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
Member

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
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
Contributor Author

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
lib/Service/Gateway/SMS/Provider/Ovh.php Outdated Show resolved Hide resolved
lib/Service/Gateway/SMS/Provider/Ovh.php Outdated Show resolved Hide resolved
lib/Service/Gateway/SMS/Provider/Ovh.php Outdated Show resolved Hide resolved
lib/Service/Gateway/SMS/Provider/Ovh.php Outdated Show resolved Hide resolved
@ChristophWurst
Copy link
Member

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 <[email protected]>

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 <[email protected]>

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

Signed-off-by: Jordan Bieder <[email protected]>

Improve documentation since it seems not that easy

Signed-off-by: Jordan Bieder <[email protected]>

Update Admin Documentation.md

Signed-off-by: Jordan Bieder <[email protected]>

Update Admin Documentation.md

Signed-off-by: Jordan Bieder <[email protected]>

Update Admin Documentation.md

Signed-off-by: Jordan Bieder <[email protected]>

Fix php syntax

Signed-off-by: Jordan Bieder <[email protected]>

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-contrib/css-loader@v3.4.1...v3.4.2)

Signed-off-by: dependabot-preview[bot] <[email protected]>

[tx-robot] updated from transifex

[tx-robot] updated from transifex

Add sms77 to help and fix case selection string

Signed-off-by: Olav Seyfarth <[email protected]>

Update OVH sms provider to remove ovh/ovh dependency

Signed-off-by: Jordan Bieder <[email protected]>

Update OVH sms provider to remove ovh/ovh dependency

Update ovh endpoints

Signed-off-by: Jordan Bieder <[email protected]>

Fix typos

Signed-off-by: Jordan Bieder <[email protected]>

Fix typos bis

Signed-off-by: Jordan Bieder <[email protected]>

Fix POST request

Signed-off-by: Jordan Bieder <[email protected]>

Remove ovh/ovh with composer

Signed-off-by: Jordan Bieder <[email protected]>

Stupid commit to get the DCO ?!

Signed-off-by: Jordan Bieder <[email protected]>

Add requested corrections

Signed-off-by: Jordan Bieder <[email protected]>

Fix indentation

Signed-off-by: Jordan Bieder <[email protected]>
@piti-diablotin
Merge branch 'master' into master
fa06de3
@piti-diablotin
Copy link
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 <[email protected]>
@ChristophWurst
Copy link
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
Member

What happened to this file ?!

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

@piti-diablotin
Copy link
Contributor Author

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
Contributor Author

How should I make tests to improve code coverage ?

@piti-diablotin
Copy link
Contributor Author

Hi,
Could we proceed ?

@ChristophWurst
Copy link
Member

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
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
Open
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