-
Notifications
You must be signed in to change notification settings - Fork 302
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
Add support for Minor Version 1 which adds TaxInclusiveAmt attribute … #440
base: master
Are you sure you want to change the base?
Conversation
…Line.SalesItemLineDetail, Line.ItemBasedExpenseLineDetail, and Line.AccountBasedExpenseLineDetail line types
Missed some code and updates to 0.6.2. This update has now been reverted as I had assumed that the changes in the 0.6.2 gem were updates, whereas they were really the old level of code. |
@@ -289,7 +289,7 @@ def check_response(response, options = {}) | |||
when 429 | |||
message = parse_intuit_error[:message] | |||
raise Quickbooks::TooManyRequests, message | |||
when 502, 503, 504 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the removal of the 502 code here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These updates came from the 0.6.2 version of the gem. I merged them in from the gem as I was surprised that they were not in the github repository. Are you the only owner of the gem, as it seems to have been updated independently of the github repository?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha. Yep, my bad. After cutting 0.6.2
back in May I forgot to push my change to version.rb
; which I just did. However, it was only version.rb
and HISTORY.md
that didnt get pushed. All other files (such as lib/quickbooks/service/base_service.rb
and the addition of the 502
status check have been in the repo since May)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These have now been reverted and can be ignored. I was confused by the status of the gem as it didn't match the repo. Do I need to do anything more for this pull request?
lib/quickbooks/version.rb
Outdated
@@ -1,5 +1,5 @@ | |||
module Quickbooks | |||
|
|||
VERSION = "0.6.1" | |||
VERSION = "0.6.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to bump this yourself. I will do it pre-release
Thanks for the PR. The specs are failing. We'll need a green build for a merge. Thanks again! |
Preparing 0.6.2 release
Do you plan on fixing those changes? If not, I can take those changes and create a new PR out of them? |
There's nothing wrong with the changes. The problem in the Travis CI build seems to have nothing to do with the updates, and is reporting that there are no environment variables set. This is not something I can fix. Any advice? |
Ok, I can see that the specs need updating to support the additional minorversion parameter, but at the moment, I don't have much of an understanding on how the specs have been written and how they interact with QB. I know that the actual code updates are good, but I may need some help and time to get the spec code updated to match the code update. |
@ruckus any idea on when can be this SalesItemLineDetail will have TaxInclusiveAmt attribute? |
…Line.SalesItemLineDetail, Line.ItemBasedExpenseLineDetail, and Line.AccountBasedExpenseLineDetail line types.
This adds support for additional attributes used outside of the US that allow prices to be displayed as tax inclusive amounts.