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

[12.0][MIG] account_payment_show_invoice #242

Merged
merged 9 commits into from
Apr 20, 2019

Conversation

kittiu
Copy link
Member

@kittiu kittiu commented Mar 29, 2019

No description provided.

<field name="inherit_id" ref="account.view_account_payment_tree"/>
<field name="arch" type="xml">
<field name="partner_id" position="after">
<field name="invoice_vendor_references"/>
Copy link
Member Author

Choose a reason for hiding this comment

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

@lreficent , I change from invoice_ids to invoice_vendor_references. (not sure invoice_ids is your intention?)

class AccountPayment(models.Model):
_inherit = 'account.payment'

@api.multi
Copy link
Member Author

Choose a reason for hiding this comment

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

@lreficent I change from api.one -> api.multi. In the past, I heard that we should always use api.multi since api.one have performance issue.
But I also see many code using api.one, not sure the performance was fixed. Please suggest.

@kittiu kittiu mentioned this pull request Mar 29, 2019
16 tasks
@pedrobaeza pedrobaeza added this to the 12.0 milestone Mar 29, 2019
@kittiu kittiu force-pushed the 12.0-mig-account_payment_show_invoice branch 2 times, most recently from 77f9707 to 8861a83 Compare April 2, 2019 04:21
_inherit = 'account.payment'

@api.multi
def _compute_invoice_vendor_references(self):
Copy link
Member

Choose a reason for hiding this comment

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

i think that it is a good practice to include ensure_one() if you change the api from one to multi.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here, I think the reason to use multi is oppose of using ensure_one() here isn't it? We use multi because we want to compute once for all records.
Please correct me if I am wrong.

Copy link
Member

Choose a reason for hiding this comment

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

When I double think, should that not be @api.depends('invoice_ids')?
Because the field should be changed in case the 'invoice_ids' are changed.

Copy link
Member

Choose a reason for hiding this comment

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

ok, you are right, after we discussed it I agree that @api.multi without ensure_one is fine. : ))

@@ -0,0 +1 @@
from . import test_payment_ref_invoice
Copy link
Member

Choose a reason for hiding this comment

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

dont't we need a license text here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add it.

<odoo>

<record id="view_account_supplier_payment_tree" model="ir.ui.view">
<field name="name">account.supplier.payment.tree - show invoice</field>
Copy link
Member

Choose a reason for hiding this comment

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

Is changing the name required?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you suggest please? I am not sure what you means (I just migrate from original)

Copy link
Member

Choose a reason for hiding this comment

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

In the original is "account.supplier.payment.tree" and here it is "account.supplier.payment.tree - show invoice", I am just not sure if it should be changed or not : )

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I just check from v11 also. And it also has the - show invoice

Copy link
Member

Choose a reason for hiding this comment

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

True, I am sorry. I have overseen it. That here all is fine.

</record>

<record id="view_account_payment_tree" model="ir.ui.view">
<field name="name">account.supplier.payment.tree - show invoice</field>
Copy link
Member

Choose a reason for hiding this comment

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

same as above.

@@ -0,0 +1,431 @@
<?xml version="1.0" encoding="utf-8" ?>
Copy link
Member

Choose a reason for hiding this comment

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

Is this file changed, for me it looks like the one in v11?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one, I did follow the instruction by using oca-gen-addon-readme

for payment in self:
ref = payment.invoice_ids.mapped(lambda x: x.reference or x.number)
ref.sort()
payment.invoice_vendor_references = ', '.join(ref)
Copy link
Member

Choose a reason for hiding this comment

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

I like this function implementation much more than in v11, good job!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you :)

@ntodorova
Copy link
Member

It looks good, even better in v11. I just have couple of questions before I approve it.

@kittiu kittiu force-pushed the 12.0-mig-account_payment_show_invoice branch from 8861a83 to a451bb6 Compare April 10, 2019 14:27
Copy link
Member

@ntodorova ntodorova left a comment

Choose a reason for hiding this comment

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

ok, so only the api question is left.

_inherit = 'account.payment'

@api.multi
def _compute_invoice_vendor_references(self):
Copy link
Member

Choose a reason for hiding this comment

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

When I double think, should that not be @api.depends('invoice_ids')?
Because the field should be changed in case the 'invoice_ids' are changed.

<odoo>

<record id="view_account_supplier_payment_tree" model="ir.ui.view">
<field name="name">account.supplier.payment.tree - show invoice</field>
Copy link
Member

Choose a reason for hiding this comment

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

True, I am sorry. I have overseen it. That here all is fine.

@kittiu
Copy link
Member Author

kittiu commented Apr 10, 2019

ok, so only the api question is left.

Note really, because this is not store=True field. So, it is not necessary (it will be recomputed every time it is called)

To use this module, you need to:

#. Go to 'Invoicing > Vendors > Payments' or to 'Invoicing > Customers >
Payments'
Copy link
Member

Choose a reason for hiding this comment

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

I did a function test and did not found the menus mentioned here. Can you please check this too?

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Copy link
Member

@ntodorova ntodorova left a comment

Choose a reason for hiding this comment

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

LGTM

@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). 🤖

Copy link
Member

@tbaden tbaden left a comment

Choose a reason for hiding this comment

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

"website": "https://odoo-community.org/",

please change the website in the manifest to: https://github.com/OCA/account-payment

@kittiu kittiu force-pushed the 12.0-mig-account_payment_show_invoice branch from a451bb6 to eab500d Compare April 20, 2019 05:32
@kittiu
Copy link
Member Author

kittiu commented Apr 20, 2019

account-payment/account_payment_show_invoice/manifest.py

Line 9 in a451bb6

"website": "https://odoo-community.org/",
please change the website in the manifest to: https://github.com/OCA/account-payment

Done.

Copy link
Member

@tbaden tbaden left a comment

Choose a reason for hiding this comment

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

Code Review

@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 pedrobaeza merged commit 94c2853 into OCA:12.0 Apr 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants