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.

Sign up for GitHub

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

Jump to bottom

[IMP] account_payment_partner: Add options to show partner bank accou… #458

Merged
merged 2 commits into from
Dec 4, 2018
Merged

[IMP] account_payment_partner: Add options to show partner bank accou… #458

merged 2 commits into from
Dec 4, 2018

Conversation

carlosdauden
Copy link
Contributor

…nt in invoice report

It is possible that this isn't the most appropriate module, but this is the module that adds the bank account to the invoice report.

@Tecnativa

@carlosdauden carlosdauden force-pushed the 10.0-IMP-account_payment_partner-show_bank_account branch 2 times, most recently from 6db036e to 702438f Compare March 15, 2018 13:51
pedrobaeza
pedrobaeza requested changes Mar 16, 2018
View reviewed changes
account_payment_partner/README.rst Show resolved Hide resolved
account_payment_partner/models/account_payment_method.py Outdated Show resolved Hide resolved
account_payment_partner/models/account_payment_method.py Outdated Show resolved Hide resolved
account_payment_partner/models/account_payment_method.py Outdated
default='full',
help="Show in invoices partial or full bank account number")
show_bank_account_chars = fields.Integer(
string="Number of chars",
Copy link
Member

Choose a reason for hiding this comment

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

For the sake of helping to know which field is each one when using advanced search, I would put here a more descriptive string: Nº of digits for customer bank account, and a label with only # of digits

@carlosdauden carlosdauden force-pushed the 10.0-IMP-account_payment_partner-show_bank_account branch 2 times, most recently from bf3138b to c944710 Compare March 22, 2018 10:53
rafaelbn
rafaelbn approved these changes Apr 17, 2018
View reviewed changes
Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

Tested functionally only 👍

alexis-via
alexis-via reviewed Apr 17, 2018
View reviewed changes
Copy link
Contributor

@alexis-via alexis-via left a comment

Choose a reason for hiding this comment

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

To sum up my position: I am OK to modify the partner_id_change, I'm OK to modify the invoice report in this module, but I think this PR needs to be simplified and add most of the fields that you propose to add to account.payment.mode should NOT be added... it's just too much complexity for something that 99% of users don't need.

account_payment_partner/models/account_invoice.py Show resolved Hide resolved
account_payment_partner/models/account_payment_mode.py Show resolved Hide resolved
rafaelbn
rafaelbn approved these changes Apr 26, 2018
View reviewed changes
Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

Runbot is 🔴

In my side this is functionally 👍

@pedrobaeza
Copy link
Member

@carlosdauden please improve help or README and check Runbot status.

@pedrobaeza
Copy link
Member

@carlosdauden please finish this

@carlosdauden carlosdauden force-pushed the 10.0-IMP-account_payment_partner-show_bank_account branch from c944710 to d052933 Compare May 24, 2018 11:28
@sbidoul
Copy link
Member

sbidoul commented May 24, 2018

@carlosdauden since you are touching the readme, would you try with the new readme/*.rst fragments?

@carlosdauden carlosdauden force-pushed the 10.0-IMP-account_payment_partner-show_bank_account branch 2 times, most recently from 3c0f19c to a577b4a Compare May 24, 2018 16:19
@carlosdauden carlosdauden force-pushed the 10.0-IMP-account_payment_partner-show_bank_account branch from a577b4a to 84199e8 Compare July 23, 2018 16:56
@cubells cubells force-pushed the 10.0-IMP-account_payment_partner-show_bank_account branch 2 times, most recently from 81fcb5a to 7931636 Compare December 3, 2018 13:03
@cubells
Copy link
Member

cubells commented Dec 3, 2018

@alexis-via @pedrobaeza

can you recheck this?

@cubells cubells mentioned this pull request Dec 4, 2018
Merged
@cubells cubells force-pushed the 10.0-IMP-account_payment_partner-show_bank_account branch from 7931636 to f97d48e Compare December 4, 2018 08:33
carlosdauden and others added 2 commits December 4, 2018 17:51
@carlosdauden @pedrobaeza
[FIX+IMP] account_payment_partner: Fix invoice bank account assignati…
fbb15b4 
…on + show it on invoice report

* IMP: Add options to show partner bank account on invoice report
* FIX: Don't auto-assign partner bank account due to problems when you have a direct debit payment
  method with bank_account_link = 'fixed'. Bank account is shown through new mechanism.
@cubells @pedrobaeza
[IMP] account_payment_partner: Add several tests
4d68b74
@pedrobaeza pedrobaeza force-pushed the 10.0-IMP-account_payment_partner-show_bank_account branch from f97d48e to 4d68b74 Compare December 4, 2018 16:52
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@pedrobaeza
Copy link
Member

This has been long time here and all comments have been honored, so I'm merging and forward-porting to newer versions. Thanks to all.

@pedrobaeza pedrobaeza merged commit 6338837 into OCA:10.0 Dec 4, 2018
@pedrobaeza pedrobaeza deleted the 10.0-IMP-account_payment_partner-show_bank_account branch December 4, 2018 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexis-via alexis-via alexis-via left review comments

@rafaelbn rafaelbn rafaelbn approved these changes

@pedrobaeza pedrobaeza pedrobaeza approved these changes

Assignees
No one assigned
Labels
approved ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@carlosdauden @pedrobaeza @sbidoul @cubells @OCA-git-bot @alexis-via @rafaelbn