thallium205 / xero

Xero Library for Private Applications in Node
48 stars 55 forks source link

Why singularize(root)? #14

Open jasonfmo opened 9 years ago

jasonfmo commented 9 years ago

Why are we singularizing the root element name? This appears to be a bug?

For example if I try and update a contact number I'd make the request to..

https://api.xero.com/api.xro/2.0/contacts/##contactID##

Payload:

<contacts>
    <Contact>
        <ContactNumber>123456</ContactNumber>
    </Contact>
</contacts>

Notice the root element is contacts and not contact? It won't work if it's singularized.

andrew-wharton commented 8 years ago

Hey! Just a quick question. Are you saying that this is a bug with this wrapper library or with Xero's API or API docs. Where exactly are we singularising the root element name? Is this a documentation problem or a library problem?

I know it's been a while since you opened this issue, so if it's not a problem anymore, just let me know and I'll close this issue. If I don't hear from you in the next few weeks, I'll assume that all is OK and close this issue as well. Feel free to re-open if need be.

jasonfmo commented 8 years ago

Hi Andrew! A bug with the wrapper.

Line 33 of index.js

"inflect.singularize(root)"

I don't believe we should be doing this. At least definitely not for contacts.

andrew-wharton commented 8 years ago

Absolutely, I see what you're referring to now. I'd like to hear from @jesucarr (https://github.com/thallium205/xero/commit/b755166fcc131fb59c9ea0dcdfcdca7bdb50415e) what the reason for this was in the first place, as he went to some effort (bringing in the 'inflect' library) to do this.

But you're right that at least in the case of contacts and accounts this is broken and in other cases it's likely unnecessary, unless I'm missing something.

jesucarr commented 8 years ago

I think that change for this was that originally all the answers where wrapped by Response instead of the singularized name as per documentation, and to make possible to send arrays in the calls, useful for example to post multiple elements in one call.

But anyway I could not reproduce the issue. I just did:

request = {
  Name: 'Test UpdateContact'
};
xero.call('POST', '/Contacts/##contactId##', request, function(err, json) {
  ...
});

And the response is:

{ Response: 
   { Id: 'xxx',
     Status: 'OK',
     ProviderName: 'xxx',
     DateTimeUTC: '2016-01-18T08:33:08.0898339Z',
     Contacts: 
      { Contact: 
         { ContactID: 'xxx',
           ContactStatus: 'ACTIVE',
           Name: 'Test Contact2',
           Addresses: { Address: [ { AddressType: 'POBOX' }, { AddressType: 'STREET' } ] },
           Phones: 
            { Phone: 
               [ { PhoneType: 'DEFAULT' },
                 { PhoneType: 'DDI' },
                 { PhoneType: 'FAX' },
                 { PhoneType: 'MOBILE' } ] },
           UpdatedDateUTC: '2016-01-18T08:33:08.027',
           IsSupplier: 'false',
           IsCustomer: 'false' } } } }

As expected. Am I missing something?

jasonfmo commented 8 years ago

Ahhh... Looks like I'm not constructing my request correctly. (The readme clearly tells me how I SHOULD be doing it)

I've got something like ...

request = { Contact: { Name: 'Test UpdateContact' } }

Which gives you a response but it doesn't actually update the contact.

I'm guessing this changed with the wrapper update and I never updated my calls. My bad!

andrew-wharton commented 8 years ago

Right, then if the passed request data is an Array, EasyXML will wrap it with a root node using the pluralized version of the name which we have extracted from the path, in this case "Contacts".

There's a little too much magic going on here for my liking, but as long as Xero follows this pattern consistently across all their endpoints, I don't see the harm in it.

I think we could add something to the README that we use EasyXML for JS <-> XML conversion, because you also need to know the conventions that they use when doing the conversion, eg. Arrays, singularizing names etc.

I think this is resolved for now, with a small task just to update the README if you guys also think it's warranted.