Problem/Motivation

XeroItemManager::doLoadItem() adds a query condition to query an item by its guid.

According to https://community.xero.com/developer/question/43161

You would be better off not using a 'WHERE' if you are retrieving a contact and know the GUID already:
GET https://…/Contacts/{identifier}

Steps to reproduce

\Drupal::service('xero.item_manager')->loadItem('xero_account', '12633dc2-
6fa3-4995-b96f-4215e88dc8f9');

leads to a logged error from Xero, a QueryParseException:
Operator '==' incompatible with operand types 'Guid' and 'String'

Proposed resolution

Load by guid directly, without a WHERE.

Remaining tasks

I'll prepare a patch.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#2 3179866-2.patch29.51 KBjonathanshaw

Issue fork xero-3179866

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jonathanshaw created an issue. See original summary.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
StatusFileSize
new 3179866-2.patch29.51 KB

So I did the following:

1) In xeroQuery use Contacts/{$this->uuid} as the endpoint rather than just Contacts, any time the id is set on the query.

2) Adapt XeroItemManager::loadItem to set the id on the query rather than specifiy a condition.

3) In which case XeroItemManager::doLoadItem is uneeded, reloadItem() can just wrap loadItem() now.

4) Added a XeroQueryExecuteTest kernel test. It's not much now, but it's a step in a good direction. Unit tests are masochistic when woring with typed data.

5) I also tidied up the createGuid helper method. It was being duplicated a dozen times, sometimes with the optional $braces argument ignored, and sometimes tests were even depending on it being ignored. Given that history of confusion, I thought it best to split into 2 methods, createGuid() and (more rarely used) createGuidWithBraces().

6) As this helper is needed by both kernel and unit tests, I created a Traits directory and put a XeroGuidTrait there.

7) I removed Unit/XeroTestHelperTrait and replaced with with XeroGuidTrait and XeroTokenTrait

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Assigned: jonathanshaw » Unassigned
Status: Active » Needs review

mradcliffe made their first commit to this issue’s fork.

mradcliffe opened merge request !1

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented

Re-rolled patch in #2 and then added an example to xero_example module mainly for manual testing.

I think all the changes should be there.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Status: Needs review » Needs work

Issue status ping-pong to see if merge request will run tests after I forgot to enable test on issues.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Status: Needs work » Needs review

Back to needs review.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented

Reroll looks good to me, thanks for looking at this.

johnpitcairn’s picture

johnpitcairn commented

Aha. Was wondering why I couldn't query for a bank transaction by its guid.

[edit: removed irrelevant problem]

johnpitcairn’s picture

johnpitcairn commented
Status: Needs review » Reviewed & tested by the community

This appears to be working as expected, and fixes a major annoyance.

  • mradcliffe committed 886026b on 8.x-2.x 
    Issue #3179866 by mradcliffe, jonathanshaw, John Pitcairn:...
mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Thank you for the review, @jonathanshaw. Thank you for the review and testing, @John Pitcairn. I'm sorry for my tardiness.

Merging in now.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.