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

Handle the new pagination element #917

Merged
merged 1 commit into from
Jul 1, 2024
Merged

Conversation

bretto36
Copy link
Contributor

@bretto36 bretto36 commented Jul 1, 2024

So any requests that are paginated are currently failing. Can this be merged in ASAP please @calcinai

@bretto36
Copy link
Contributor Author

bretto36 commented Jul 1, 2024

#916 #915

This handles the changes Xero just released

@calcinai
Copy link
Owner

calcinai commented Jul 1, 2024

LGTM. Thanks @bretto36

@calcinai calcinai merged commit 7fbe9a0 into calcinai:master Jul 1, 2024
@bretto36
Copy link
Contributor Author

bretto36 commented Jul 1, 2024

@calcinai I can't run the tests locally as my version of php doesn't seem to like it. Are you able to run them. I've tested the XML stuff locally, but not sure how to test the parseJSON() method

Thanks for actioning so fast. Love when Xero releases something and doesn't give you a way to test beforehand

@bretto36
Copy link
Contributor Author

bretto36 commented Jul 1, 2024

@calcinai Sorry for hounding you, can you also submit a new tag for release when you have time. Thanks so much!

@p4veI
Copy link

p4veI commented Jul 17, 2024

Hello @bretto36 and @calcinai came across this MR while debugging an empty invoice element parsed via the sdk, I believe the handling of pagination here is not completely correct, from what I'm seeing the child_index/key of the pagination is pagination i don't see any PageInfo really, see debug step attached. Can we resolve this somehow, the pagination element is being parsed as an empty invoice without any data.

Screenshot 2024-07-17 at 11 23 15

@bretto36
Copy link
Contributor Author

Yes I have a new PR up because Xero have changed the value again.

Just waiting for @calcinai to merge. Hopefully he's online at some stage.

Or you can use my fork in the meantime

@p4veI
Copy link

p4veI commented Jul 17, 2024

Cool, missed that one, thanks!

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.

3 participants