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

Currency field not always required #62

Merged
merged 2 commits into from Jun 26, 2018

Conversation

nelz9999
Copy link
Contributor

When using the existing Adjustment object for use against the Purchases API, was getting an error with the following text: The provided XML was invalid. Adjustments cannot contain a child currency tag.

This just sets the currency element as omitempty, and tidies up tests that hard-coded the element.

Copy link
Contributor

@frankienicoletti frankienicoletti left a comment

Choose a reason for hiding this comment

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

I left one small comment but otherwise LGTM!

adjustments.go Outdated
@@ -49,7 +49,7 @@ func (a Adjustment) MarshalXML(e *xml.Encoder, start xml.StartElement) error {
ProductCode string `xml:"product_code,omitempty"`
UnitAmountInCents int `xml:"unit_amount_in_cents"`
Quantity int `xml:"quantity,omitempty"`
Currency string `xml:"currency"`
Currency string `xml:"currency,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an in-line comment here that says something like // Should only be omitted in purchase calls? This field is still required to create an adjustment and I don't want it to look optional.

@nelz9999
Copy link
Contributor Author

@katenicoletti Comment added. (Tho, somewhat generic. I hope that's okay.)

@cristiangraz cristiangraz merged commit 774af5a into blacklightcms:master Jun 26, 2018
@nelz9999 nelz9999 deleted the adjustment-currency branch June 27, 2018 18:09
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.

None yet

3 participants