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

Add option to select an office in Twinfield #164

Merged

Conversation

lenvanessen
Copy link
Contributor

This allows the user to select the office currently used in Twinfield. This is needed for the BrowseData endpoint. Also fixes #163

https://accounting2.twinfield.com/webservices/documentation/#/FAQ

@lenvanessen
Copy link
Contributor Author

@willemstuursma can you review this and how do you feel about the approach?

@lenvanessen
Copy link
Contributor Author

Btw, If the approach is good I'll update the tests as well

@willemstuursma
Copy link
Contributor

willemstuursma commented Jan 29, 2020

Hi @lenvanessen. Thanks for contributing this PR.

I don't work for Twinfield so it took me some time to find time to look at this PR.

I feel it can be improved by merging the SelectOfficeService you created with the anonymous class in \PhpTwinfield\Secure\WebservicesAuthentication::login. I would then rename it the SessionService. The anonymous class would be removed.

Otherwise it looks fine.

If you could make this change and ideally add some tests, I will make a timely release.

@lenvanessen
Copy link
Contributor Author

Sure thing, gonna work on it now. Wasn't sure about merging the two, because the session.asmx on login.twinfield.com is different from the one from the cluster accounting.twinfield.com, it's missing the SelectCompany method. But it does have the Logon method so I think we'll be able to merge the two.

Gonna update the PR soon!

@willemstuursma
Copy link
Contributor

Cool. Planning on adding any test cases? Can you confirm it all still works?

@lenvanessen
Copy link
Contributor Author

Just updated the test case, you might want to double-check them. I'm not that familiar with testing Soap, as when Soap was popular I was still in diapers;)

All test cases show no issues, and also manual testing shows it's properly switching the office as well as authenticating and fetching data.

@willemstuursma willemstuursma merged commit eb0cb0f into php-twinfield:master Feb 9, 2020
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.

How do you set the office for the Browse data?
2 participants