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

Credit Note Allocations (plus minor tweaks) #125

Open
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

sensadrome
Copy link

@sensadrome sensadrome commented Dec 19, 2017

Added updated_at accessor to Account
Added :status filter for Invoice requests
Added :page option for paging credit notes
Added Allocation record (based on base_record) with tests
Fixed BaseRecord nested structure (so that to_xml works as expected)
Added allocate_credit_note action to gateway which takes a credit note id, invoice_id and amount to allocate part of the credit note against an invoice
Changed the method for creating credit_notes to post as per Xero docs

@sensadrome
Copy link
Author

Hi there! Thanks very much for the gem!

Sorry about the large PR.... mostly I wanted to be able to allocate credit notes to invoices and see which invoices credit notes had been applied to when pulling down from Xero. Along the way I discovered a few little niggles which I also fixed. I thought some people might find them useful, and some were broken anyway (e.g. paging for credit notes) If you're ok with all this then great, but let me know if you'd rather I split it up into smaller PRs....

Best,

Simon (aka sensadrome)

@@ -791,7 +808,7 @@ def parse_response(raw_response, request = {}, options = {})
end if response_element

# If a single result is returned don't put it in an array
if response.response_item.is_a?(Array) && response.response_item.size == 1
if response.response_item.is_a?(Array) && response.response_item.size == 1 && !parse_options[:dont_flatten]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good solution for now, but we should really build up a list of the entities that are "flattenable" and those that just happen to be returning a single value :)

Copy link
Contributor

@nikz nikz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good, thank you so much for the changes! I've left a couple notes, and I've given @armstrjare a 🔔 to check it out to since he's using TrackingCategories pretty heavily I think.

Thanks again!

@@ -212,7 +213,7 @@ def self.from_xml(invoice_element, gateway = nil, options = {})
when "Contact" then invoice.contact = Contact.from_xml(element)
when "Date" then invoice.date = parse_date(element.text)
when "DueDate" then invoice.due_date = parse_date(element.text)
when "UpdatedDateUTC" then invoice.updated_date_utc = parse_date(element.text)
when "UpdatedDateUTC" then invoice.updated_at = parse_date_time(element.text)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this probably makes the PR a breaking change unfortunately. Do you think it would be worth maintaining the updated_date_utc API (i.e aliasing it?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well the reason for the change was simply that the attr_accessor is for updated_at and not updated_at_utc.

# This means rather than going category.to_xml we need to call the special category.to_xml_for_invoice_messages
(tracking.is_a?(TrackingCategory) ? [tracking] : tracking).each do |category|
category.to_xml_for_invoice_messages(b)
b.Tracking do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@armstrjare want to check this one?

end

def test_create_allocation
example_allocation = dummy_allocation.dup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% following why you're #dup-ing these sorry?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.. actualyl nor am I now ;)

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.

2 participants