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

9.0 [ADD] account_invoice_check_total #162

Merged
merged 12 commits into from
Oct 11, 2016

Conversation

ThomasBinsfeld
Copy link
Contributor

No description provided.

@ThomasBinsfeld
Copy link
Contributor Author

@sbidoul

===========

Bugs are tracked on `GitHub Issues
<https://github.com/OCA/{project_repo}/issues>`_. In case of trouble, please
Copy link
Member

Choose a reason for hiding this comment

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

replace {project_repo}

@sbidoul
Copy link
Member

sbidoul commented Jul 12, 2016

@ThomasBinsfeld runbot is red because existing tests do set the check_total field when creating invoices.

Perhaps we should make check_total required on the supplier invoice/refund form and have the check trigger only if check_total is set.

What do you think?

@sbidoul
Copy link
Member

sbidoul commented Jul 13, 2016

@ThomasBinsfeld it seems this is not finished, you need to

<data noupdate="0">

<record id="group_supplier_inv_check_total" model="res.groups">
<field name="name">Check Total on supplier bills</field>
Copy link
Member

Choose a reason for hiding this comment

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

Check Total on Vendor Bills

@sbidoul
Copy link
Member

sbidoul commented Jul 15, 2016

@ThomasBinsfeld one last small remark. It now works fine.

👍 (code review and functional test on runbot)

@adrienpeiffer
Copy link
Contributor

@ThomasBinsfeld I just add group_supplier_inv_check_total on account.config.settings. So, it's now possible to add this group for all users in one click.

Copy link
Member

@atchuthan atchuthan left a comment

Choose a reason for hiding this comment

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

few minor corrections

Runbot test 👍


Add a Verification Total field on vendor bills.
The user enters the taxes included invoice total as printed on the vendor bill, then enters the invoice lines and taxes. The system then checks the total computed by Odoo is the same as the verification total.

Copy link
Member

Choose a reason for hiding this comment

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

Add runbot link in Usage section
Use template from maintainers repo (https://raw.githubusercontent.com/OCA/maintainer-tools/master/template/module/README.rst)

'security/account_invoice_security.xml',
'views/account_invoice.xml',
],
'demo': [
Copy link
Member

Choose a reason for hiding this comment

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

Remove empty keys

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

Choose a reason for hiding this comment

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

use odoo instead of openerp for 9.0

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

Choose a reason for hiding this comment

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

make it as noupdate="1" or remove the tag itself

License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). -->

<openerp>
<data>
Copy link
Member

Choose a reason for hiding this comment

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

replace openerp and data with odoo xml tag

@Cedric-Pigeon
Copy link

👍 code review

@api.multi
def action_move_create(self):
for inv in self:
gr = 'account_invoice_check_total.group_supplier_inv_check_total'
Copy link
Member

Choose a reason for hiding this comment

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

You can import the group from the other file to have only one reference

@api.model
def _prepare_refund(self, invoice, date_invoice=None,
date=None, description=None, journal_id=None):
vals = super(AccountInvoice, self)._prepare_refund(invoice,
Copy link
Member

Choose a reason for hiding this comment

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

Use this for shorten the number of lines:

vals = super(AccountInvoice, self)._prepare_refund(
    invoice, date_invoice, date, description, journal_id)

description,
journal_id)

if invoice and invoice.type in ['in_invoice', 'in_refund']:
Copy link
Member

Choose a reason for hiding this comment

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

No need to check previously if invoice. Only second condition is needed.

@@ -0,0 +1,79 @@
<?xml version="1.0" encoding="UTF-8" standalone="no"?>
Copy link
Member

Choose a reason for hiding this comment

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

Put the PNG icon, not the SVG (unless it's an specific one)

self.account_invoice_line = self.env['account.invoice.line']
self.current_user = self.env.user
# Add current user to group: group_supplier_inv_check_total
group_id = 'account_invoice_check_total.group_supplier_inv_check_total'
Copy link
Member

Choose a reason for hiding this comment

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

Import the group variable

@pedrobaeza
Copy link
Member

Please attend last comments and squash all the commits together by author, and I'll merge

@@ -4,6 +4,8 @@

from openerp.tests.common import TransactionCase
from openerp.exceptions import UserError
from openerp.addons.account_invoice_check_total.models.account_invoice\
Copy link
Member

Choose a reason for hiding this comment

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

Use from ..models.account_invoice import GROUP_AICT


from openerp import fields, models

GROUP_XML_ID = 'account_invoice_check_total.group_supplier_inv_check_total'
Copy link
Member

Choose a reason for hiding this comment

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

Import here also the group

@pedrobaeza
Copy link
Member

Please remember to squash commits

@ThomasBinsfeld ThomasBinsfeld force-pushed the 9.0-add_account_invoice_check_total branch from fe88110 to 332c4dd Compare October 11, 2016 08:50
@ThomasBinsfeld
Copy link
Contributor Author

@pedrobaeza Thanks for the review. I squashed cosmetic commits to preserve a meaningful history.

@pedrobaeza
Copy link
Member

Please use relative import as requested, or the linter fails.

@pedrobaeza
Copy link
Member

I see still a lot of irrelevant commits at the beginning. Please squash them.

@ThomasBinsfeld ThomasBinsfeld force-pushed the 9.0-add_account_invoice_check_total branch from 332c4dd to 3229b65 Compare October 11, 2016 09:45
@adrienpeiffer
Copy link
Contributor

@pedrobaeza this one is now ok for you ?

@pedrobaeza
Copy link
Member

Yeah, this is better, thanks.

@pedrobaeza pedrobaeza merged commit 962ad26 into OCA:9.0 Oct 11, 2016
@pedrobaeza pedrobaeza mentioned this pull request Oct 11, 2016
26 tasks
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.

6 participants