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

Add account_tax_balance #190

Merged
merged 9 commits into from Nov 4, 2016
Merged

Conversation

@eLBati eLBati force-pushed the oca-sorrento:add_account_tax_balance branch from e50e15e to a9d8584 Apr 29, 2016
@coveralls
Copy link

coveralls commented Apr 29, 2016

Coverage Status

Coverage remained the same at 77.143% when pulling a9d8584 on oca-sorrento:add_account_tax_balance into be32b21 on OCA:9.0.


class TestAccountTaxBalance(TransactionCase):

def setUp(self):

This comment has been minimized.

Copy link
@lepistone

lepistone Apr 29, 2016

Member

not needed

'invoice_line_tax_ids': [(6, 0, [tax.id])],
})
invoice._onchange_invoice_line_ids()
# save new() values to DB

This comment has been minimized.

Copy link
@lepistone

This comment has been minimized.

Copy link
@eLBati

eLBati May 3, 2016

Author Member

@lepistone see account.invoice._onchange_invoice_line_ids
Do you know a better way to save to DB recordset created with new() ?

This comment has been minimized.

Copy link
@lepistone

lepistone May 20, 2016

Member

I don't know a better way. Then OK thanks!

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza May 20, 2016

Member

Please use invoice._convert_to_write(invoice._cache). This way, the onchange will be inheritable and will add here also the added values. Example: https://github.com/OCA/commission/pull/67/files#diff-73f037396680f271cd1a6b014c9935ecR83

}
invoice.tax_line_ids = None
self.env['account.invoice.tax'].create(tax_vals)
self.assertTrue((invoice.state == 'draft'))

This comment has been minimized.

Copy link
@lepistone

lepistone Apr 29, 2016

Member

self.assertEqual('draft', invoice.state)

# change the state of invoice to open by clicking Validate button
invoice.signal_workflow('invoice_open')

self.assertEquals(tax.base_balance, -100)

This comment has been minimized.

Copy link
@lepistone

lepistone Apr 29, 2016

Member

for comparing floats assertAlmostEqual makes probably more sense.

This comment has been minimized.

Copy link
@eLBati

eLBati May 3, 2016

Author Member

In this case I want it to be -100

This comment has been minimized.

Copy link
@lepistone

lepistone May 20, 2016

Member

I understand, but normally with floats "equal" comparison is not relevant. It might work here with =, especially because integers can be represented exactly, but it is not always the case. That is because a base-10 decimal number like 0.5 is represented in a float only by approximation IFRC.

I am suggesting to use assertAlmostEqual with the default setting (which is something like 7 decimal places) just to prevent low-level problems with floats, not with something like 2 decimal places because that could hide real bugs.

Thanks!

This comment has been minimized.

Copy link
@eLBati

eLBati May 22, 2016

Author Member

I understand assertAlmostEqual is useful when approximation risks are present but, in this case, I can't see those risks. (What does IFRC stand for?)

This comment has been minimized.

Copy link
@lepistone

lepistone May 23, 2016

Member

If I Remember Correctly.

As I said, floats are represented exactly only in few cases (including integers) otherwise they are approximations. "Approximation risks" are almost everywhere and an exact comparison of floats doesn't make sense in general.

I can live with that tough since -100 is an integer.

Thanks!

@lepistone
Copy link
Member

lepistone commented Apr 29, 2016

minor remarks, otherwise looks good, thanks! 👍

@jgrandguillaume
Copy link
Member

jgrandguillaume commented May 3, 2016

Hi,

Thanks, functional tests ok on my side.

👍

Regards,

Joël

@eLBati eLBati force-pushed the oca-sorrento:add_account_tax_balance branch from a9d8584 to 391bbfa May 3, 2016
company_id = self.env.user.company_id.id
else:
company_id = self.env.context['company_id']
return from_date, to_date, company_id, target_move

This comment has been minimized.

Copy link
@lepistone

lepistone May 20, 2016

Member

What do you think about:

context = self.env.context
return (
    context.get('from_date') or fields.Date.context_today(self),
    context.get('to_date') or fields.Date.context_today(self),
    context.get('target_move') or 'posted',
    context.get('company_id') or self.env.user.company_id.id,
)

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza May 20, 2016

Member

It's even easier:

context = self.env.context
return (
    context.get('from_date', fields.Date.context_today(self)),
    context.get('to_date', fields.Date.context_today(self)),
    context.get('target_move', 'posted'),
    context.get('company_id', self.env.user.company_id.id),
)

and you also allow False values in the context.


def _compute_balance(self):
from_date, to_date, company_id, target_move = self.get_context_values()
for tax in self:

This comment has been minimized.

Copy link
@lepistone

lepistone May 20, 2016

Member

By splitting the two methods compute_balance and compute_base_balance you introduced duplication. If you did everything together you could just write the balance and base_balance in-place in the same method instead of returning the value and writing it here. What do you think?

@lepistone
Copy link
Member

lepistone commented May 20, 2016

Thanks for your work @eLBati @gfcapalbo ! please give a look to the suggestions (which are minor). Then I'd like to merge this.

@eLBati eLBati force-pushed the oca-sorrento:add_account_tax_balance branch from 45da448 to bb2e77b May 22, 2016
@eLBati
Copy link
Member Author

eLBati commented May 22, 2016

@lepistone @pedrobaeza thanks for your suggestions, I made the changes

@eLBati eLBati force-pushed the oca-sorrento:add_account_tax_balance branch from 8857e4c to caf20b5 May 22, 2016
@coveralls
Copy link

coveralls commented May 22, 2016

Coverage Status

Coverage remained the same at 79.661% when pulling caf20b5 on oca-sorrento:add_account_tax_balance into f237a52 on OCA:9.0.

"version": "9.0.1.0.0",
"category": "Accounting & Finance",
"website": "https://www.agilebg.com/",
"author": "Agile Business Group, Odoo Community Association (OCA)",

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza May 23, 2016

Member

Therp is not credited here?

Usage
=====

Accounting --> Reporting --> Open Tax Balances

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza May 23, 2016

Member

Why the "Open" word? Isn't better to just be "Taxes balance"?

@pedrobaeza
Copy link
Member

pedrobaeza commented May 23, 2016

For improving the functionality, you should save in a many2many the reference to all the account.move.line that compose the balance and the base balance, and provide buttons at line level that allows to open these lines for tracing the source of the data.

@lepistone
Copy link
Member

lepistone commented May 23, 2016

@pedrobaeza I'd like this to be merged quickly if it works, is it OK for you if we merge without the last thing you suggest? Thanks a lot!

@pedrobaeza
Copy link
Member

pedrobaeza commented May 23, 2016

That functionality is basic, because the report is not useful without that, so I consider it a blocking point.

@fclementic2c
Copy link
Member

fclementic2c commented May 24, 2016

First thank you for this very important module existing in enterprise version but not in community version.

My comments :

  • I notices a sign issue on this view : it should be - Debit + Credit (and not Debit - Credit as usual in accounting journal entries) since you want to see purchases as negative value for ex. Otherwise total VAT to be paid is wrong !
  • I totally agree with Predro's comment. Without this, this module is not helpful. You must be able to dig into lines to find where are errors !

thanks

@eLBati eLBati force-pushed the oca-sorrento:add_account_tax_balance branch from caf20b5 to 469f3d7 May 26, 2016
@coveralls
Copy link

coveralls commented May 30, 2016

Coverage Status

Coverage decreased (-0.4%) to 77.286% when pulling dcc68e6 on oca-sorrento:add_account_tax_balance into eb5e437 on OCA:9.0.

@eLBati
Copy link
Member Author

eLBati commented May 30, 2016

@pedrobaeza thanks, I made the changes.
I added 2 buttons ('View tax lines' and 'View base lines') for every tax.

@fclementic2c I just used the balance field of account.move.line
see https://github.com/oca-sorrento/account-financial-reporting/blob/dcc68e6932fd2cee8f4cae94417a10bc741a0e93/account_tax_balance/models/account_tax.py#L48
Should I use its opposite?

Thanks

@fclementic2c
Copy link
Member

fclementic2c commented Aug 30, 2016

@eLBati Yes, on the VAT return you should use the opposite of the balance field.

balance is debit - credit whereas on tax return you want to see what vat has to be paid so : VAT on sales (credit) - VAT on purchases (debit).

@eLBati eLBati force-pushed the oca-sorrento:add_account_tax_balance branch from dcc68e6 to 7789d75 Sep 8, 2016
@eLBati
Copy link
Member Author

eLBati commented Sep 8, 2016

@fclementic2c thanks, done

@eLBati eLBati force-pushed the oca-sorrento:add_account_tax_balance branch from 7ef3f06 to 7dd3d2c Oct 4, 2016
@eLBati eLBati added the needs review label Oct 17, 2016
Copy link
Member

archetipo left a comment

@eLBati thanks, only one little remark

<field name="search_view_id" ref="view_tax_search_balance"/>
</record>
</data>
</openerp>

This comment has been minimized.

Copy link
@archetipo

archetipo Oct 20, 2016

Member

no new line

This comment has been minimized.

Copy link
@eLBati

eLBati Oct 20, 2016

Author Member

@archetipo thanks, done

@eLBati eLBati force-pushed the oca-sorrento:add_account_tax_balance branch from 9c94be6 to 2057ae7 Oct 20, 2016
@antespi
Copy link
Member

antespi commented Oct 21, 2016

In the Spanish localization (and in other ones I guess) we need to separate taxes from normal sale/purchase and refunds (sale or purchase), so we want to propose these changes:

  • Make 4 taxes columns:
    • Tax base
    • Tax amount
    • Tax refund base
    • Tax refund amount

In the other hand, It would be great to show only lines with amount in any column, because in some localization there are many taxes

We can do these changes, if you don't mind, and PR them to 'oca-sorrento' repo

Copy link
Member

gurneyalex left a comment

code review only, no tests done.

@antespi
Copy link
Member

antespi commented Oct 21, 2016

@eLBati I've already done our propose, please review it in oca-sorrento repo: oca-sorrento#11

@flotho
Copy link
Member

flotho commented Oct 21, 2016

Hi, I've tested it on runbot successfully.
Regards

@eLBati
Copy link
Member Author

eLBati commented Nov 4, 2016

@pedrobaeza
Copy link
Member

pedrobaeza commented Nov 4, 2016

One last task before merging, please: you know my preference for squashing commits grouped by author and logic 😉

@antespi
Copy link
Member

antespi commented Nov 4, 2016

@eLBati For mi is ok, you can squash my commits in only one

@eLBati eLBati force-pushed the oca-sorrento:add_account_tax_balance branch 2 times, most recently from fee3821 to b198e45 Nov 4, 2016
eLBati and others added 4 commits Apr 28, 2016
IMP filters and views

Avoid create and delete taxes

ADD base balance
FIX PEP8

FIX README image
FIX PEP8

use invoice._convert_to_write(invoice._cache). This way, the onchange will be inheritable and will add here also the added values
@eLBati eLBati force-pushed the oca-sorrento:add_account_tax_balance branch from b198e45 to 253a871 Nov 4, 2016
eLBati and others added 5 commits May 22, 2016
unify method compute_balance

ADD open move lines linked to balance
balance is debit - credit whereas on tax return you want to see what vat has to be paid so : VAT on sales (credit) - VAT on purchases (debit).
ADD account_tax_balance: testing buttons

IMP account_tax_balance: covering specific methods with tests
Allow to explore all move lines

Spanish translation

Bugfixes

Show negative lines too

Show move type in account.move views

Tests include new fields
@eLBati eLBati force-pushed the oca-sorrento:add_account_tax_balance branch from 253a871 to b79358a Nov 4, 2016
@eLBati
Copy link
Member Author

eLBati commented Nov 4, 2016

@pedrobaeza squashed!

@pedrobaeza
Copy link
Member

pedrobaeza commented Nov 4, 2016

Thanks!

@pedrobaeza pedrobaeza merged commit 2e35b01 into OCA:9.0 Nov 4, 2016
5 checks passed
5 checks passed
ci/runbot runbot build 3271523-190-b79358 (runtime 66s)
Details
codecov/patch 84.56% of diff hit (target 82.25%)
Details
codecov/project 82.38% (+0.12%) compared to ae39fa1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 82.384%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.