-
Notifications
You must be signed in to change notification settings - Fork 3
Introduce invoice_type #8
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
Conversation
7b88e50 to
27170b7
Compare
27170b7 to
750ae96
Compare
650a47f to
25f4433
Compare
25f4433 to
c003d7a
Compare
lib/secretariat/invoice.rb
Outdated
| xml['ram'].TypeCode payment_code | ||
| xml['ram'].Information payment_text | ||
| if payment_iban | ||
| xml['ram'].Information payment_text if payment_text && payment_text != '' |
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.
| xml['ram'].Information payment_text if payment_text && payment_text != '' | |
| xml['ram'].Information payment_text if payment_text.present? |
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.
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.
it's pretty easy to just copy over those methods from rails, please check my PR: https://github.com/fortytools/ruby-secretariat/pull/9/files
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.
@bigsolom Ahh okay thanks
introduce blank? and present? method to make checking for blank values easier
| trade_settlement = by_version(version, 'ApplicableSupplyChainTradeSettlement', 'ApplicableHeaderTradeSettlement') | ||
| xml['ram'].send(trade_settlement) do | ||
| if payment_reference && payment_reference != '' | ||
| if payment_reference.present? |
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.
present? is a method from Rails and the gem does not depend on Rails, so shouldnt introduce that dependency here. In our context it probably works but we should keep this upstream compatable.
See also comment on object extensions below.
| @@ -0,0 +1,14 @@ | |||
| module Secretariat | |||
| module ObjectExtensions | |||
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.
Not sure if we should do this because the upstream maintainer of the gem seems to like to stick to != ''
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.
IMHO it's a better approach than !='', I create a RP for the gem to adapt this approach.
In worst case if the PR were rejected we can keep this approach for our fork and mind that when merging upstream changes to our fork
No description provided.