-
-
Notifications
You must be signed in to change notification settings - Fork 401
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 German date format #222
Conversation
…e with wrong sign
…ith-wrong-sign [IMP] account_bank_statement_import_paypal: skip mapping currency lin…
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.
Just tested and it works fine! How about merging this one?
This PR has the |
@pedrobaeza is there a reason, why not merging this one? Any Problems with the code in other countries? |
I'm not anymore OCA's community manager and I'm not monitoring PRs. This should corresponds to the PSC (as in the past, but I served as wildcard). I can do it one last time, but now this PR is contaminated with 4 commits more than when I approved it. |
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.
Remove extra commits, that also performs other things.
Hi and thanks, I understand! Wanted to address @StefanRijnhart first, but thought you are on the PSC and on the topic. Couldn't find a maintainer in the readme. Will remember next time... |
@wildi1 would be great if you could fix this soon! :-) |
@@ -156,7 +156,7 @@ def _post_process_statement_line(self, raw_lines): | |||
# transaction of currency change if yes merge the transaction | |||
# as for odoo it's only one line | |||
cline = currency_change_lines.get(line['origin_transaction_id']) | |||
if cline: | |||
if cline and (cline['amount'] * line['amount'] > 0): |
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.
I'm slightly puzzled by this line. It looks like you encountered a statement line with the currency amount having the wrong sign. Can you clarify this a bit, or even better, add a test for this situation?
@pedrobaeza The first PR was made in Aug 19. I thought it will be merged soon after you approved it and so I forgot that it is not merged yet. Therfore my other commits have contaminated this PR. @StefanRijnhart This schould be part of an other Pull Request I already made. I have split them now in two separate PRs. Here is the new PR for this request. |
@StefanRijnhart Here is the other PR and the explenation of the behaviour with the wrong sign. |
No description provided.