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

Missing scope for inital set up when requesting tenants, incorrect examples and other feedback #29

Open
williamthowe opened this issue Apr 1, 2020 · 7 comments

Comments

@williamthowe
Copy link

williamthowe commented Apr 1, 2020

Hey, thanks for the the library, looks very nice - however I'm just setting it up and have some feedback:

  1. You need to add accounting.settings.read to the default provided scopes in config/xero-laravel-lf.php, because you get 0 tenants back from the initial set up call example you provide which had me floundering for a bit.
  2. Some documentation (or link to) about setting up the token in Xero would be helpful
  3. Clear step to add the fields you need in the DB
  4. Your example code for accessing data is incorrect and has inconsistent data:

You save the access token and tenant like so:

$user->xero_access_token = json_encode($accessToken);
$user->tenant_id = $selectedTenant->tenantId;

but then call it:

$xero = new XeroApp(
            new AccessToken(json_decode($user->xero_oauth_2_access_token)),
            $user->xero_tenant_id
        );

$user->xero_access_token != $user->xero_oauth_2_access_token.

And when you try to access the data, AccessToken takes an array like so:

$xero = new XeroApp(
            new AccessToken(['access_token' => new AccessToken(['access_token' => json_decode($user->xero_access_token)->access_token]),
            $user->xero_tenant_id
        );

Also note that the token gets saved as an object, so you need to pass the access_token param or cast $user->xero_access_token to an array I guess.

Anyway, thanks! Hopefully the rest is plain sailing :)

@DivineOmega
Copy link
Contributor

Thanks @williamthowe.

You need to add accounting.settings.read to the default provided scopes in config/xero-laravel-lf.php, because you get 0 tenants back from the initial set up call example you provide which had me floundering for a bit.

I didn't realise this. We'll have to double check this and add it the default list of scopes.

Some documentation (or link to) about setting up the token in Xero would be helpful

Some basic docs on setting up the actually Xero app from their control panel is probably a good idea. 👍

Clear step to add the fields you need in the DB

I want to ensure people can store the Xero access token and tenant ID however they wish, so I don't want to necessarily provide precise instructions. However, more details here might be useful.

Your example code for accessing data is incorrect and has inconsistent data:

You are right. Good catch.

  • Field names need to be made consistent.
  • Need to use json_decode with the second parameter set to true in order to ensure this is an array.
  • Perhaps we should mention that Eloquent's casting to array can be used here.

Thanks for your feedback.

@dextermb
Copy link
Contributor

dextermb commented May 1, 2020

@DivineOmega does anything need to be actioned here? Or has everything already been resolved?

@DivineOmega
Copy link
Contributor

@dextermb These changes haven't been actioned yet.

We'll need to test if the accounting.settings.read scope is required to retreive tenants and if so, add it as a default scope.

It would be a good idea to include docs on setting up the Xero app.

And also we need to make the fixes to the documentation described by the bullet pointed items in my 1 April comment above.

@hjbdev
Copy link

hjbdev commented Aug 18, 2020

It definitely seems to be the case that accounting.settings.read is required to retrieve tenants. Was not able to use the example code to authorize without that scope in my config.

@azam1
Copy link

azam1 commented Oct 17, 2020

Also when you renew the token and save it you will need to add json_encode, Update your example code if possible.
$user->xero_access_token = json_encode($accessToken);

@zimonline
Copy link
Contributor

It definitely seems to be the case that accounting.settings.read is required to retrieve tenants. Was not able to use the example code to authorize without that scope in my config.

I can confirm this too. Glad I came across this as made me scratch my head a little too.

@jaredkove jaredkove linked a pull request Nov 5, 2020 that will close this issue
@DivineOmega DivineOmega removed a link to a pull request Dec 4, 2020
@DivineOmega
Copy link
Contributor

The missing accounting.settings.read scope is now added to the default config file as part of v3.1.1.

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

No branches or pull requests

6 participants