xero-laravel icon indicating copy to clipboard operation
xero-laravel copied to clipboard

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

Open williamthowe opened this issue 5 years ago • 7 comments

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 :)

williamthowe avatar Apr 01 '20 02:04 williamthowe

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. :+1:

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.

DivineOmega avatar Apr 01 '20 08:04 DivineOmega

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

dextermb avatar May 01 '20 07:05 dextermb

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

DivineOmega avatar May 01 '20 08:05 DivineOmega

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.

hjbdev avatar Aug 18 '20 14:08 hjbdev

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);

azam1 avatar Oct 17 '20 03:10 azam1

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.

zimonline avatar Nov 04 '20 16:11 zimonline

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

DivineOmega avatar Dec 04 '20 09:12 DivineOmega