Navigation Menu

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

[IMP] account: add tax lock date #29272

Closed
wants to merge 1 commit into from

Conversation

william-andre
Copy link
Contributor

Description of the issue/feature this PR addresses:
Journal entry can be changed the way we want while the lock date is before then entry date

Current behavior before PR:
One can only change draft entries

Desired behavior after PR is merged:
One can change all entries provided the date is after the lock date

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@C3POdoo C3POdoo added the RD research & development, internal work label Dec 5, 2018
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Dec 5, 2018
Copy link
Contributor

@qdp-odoo qdp-odoo left a comment

Choose a reason for hiding this comment

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

some required changes, some less important stuff that can be discussed

addons/account/views/account_view.xml Outdated Show resolved Hide resolved
addons/account/models/account_move.py Outdated Show resolved Hide resolved
addons/account/models/account_move.py Outdated Show resolved Hide resolved
addons/account/tests/test_account_move_tax_block_date.py Outdated Show resolved Hide resolved
addons/account/tests/test_account_move_tax_block_date.py Outdated Show resolved Hide resolved

def test_create_before_block_date(self):
self.user_id.company_id.tax_lock_date = self.last_day_month_str
with self.assertRaises(ValidationError):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather have an invoice created with a single product line with a tax on it

Copy link
Contributor

Choose a reason for hiding this comment

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

or both, but then, the with self.assertRaises(ValidationError): must be only on the move.post() line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it make any difference since the constraint is
@api.constrains('credit', 'debit', 'tax_ids', 'tax_line_id', 'date')

No difference is made.

Also, the create should raise an error since the date of the move is before the lock date. Shouldn't it ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking that the constraint might be checked only on posted moves (so, upon posting, deleting/editing/creating a posted move) but I'm not sure anymore....

keep it as it, for now

addons/account/tests/test_account_move_tax_block_date.py Outdated Show resolved Hide resolved
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Jan 4, 2019
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Jan 24, 2019
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Feb 26, 2019
Copy link
Contributor

@qdp-odoo qdp-odoo left a comment

Choose a reason for hiding this comment

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

the tests are dubious

@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Feb 27, 2019
@@ -698,6 +698,14 @@ def default_get(self, fields):
rec.update({'credit': balance})
return rec

@api.multi
@api.constrains('credit', 'debit', 'tax_ids', 'tax_line_id', 'date')
Copy link
Contributor

Choose a reason for hiding this comment

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

that ain't working...

the implementation with a constraint will fail in the following use case:

  • make an invoice with a tax
  • set the tax lock date to include the invoice
  • go on the invoice journal entry and remove the tax_ids or tax_line_id values (with account_cancel, or via the studio)
    => an error should be thrown but there isn't any since the test at line 705 is falsy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't get how it is different from what is done in the tests? And the tests are green.

Copy link
Contributor

Choose a reason for hiding this comment

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

because in my use case i'm modifying one of the field that composes the condition of L705, making it False where it was True before.

please, read again my use case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right... I remember having thought about this though.
Updated, with the tests too.

@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Mar 5, 2019
Purpose:
Journal entry can be changed the way we want while the lock date is before then entry date..
Entry : Draft -> Validated (or posted)

Specs:

    add a date field in the accounting settings: tax lock date
    implement a constraint ensuring that no one can modify the information related to tax on account.move.line prior to this lock date. That means the debit, credit, tax_ids, tax_line_id fields. Other fields can be modified. This also means we cannot create/delete an account.move.line with tax information prior to this lock date.

Task 1914762
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Mar 6, 2019
@qdp-odoo
Copy link
Contributor

qdp-odoo commented Mar 6, 2019

@robodoo r+

@robodoo robodoo added the r+ 👌 label Mar 6, 2019
robodoo pushed a commit that referenced this pull request Mar 6, 2019
Purpose:
Journal entry can be changed the way we want while the lock date is before then entry date..
Entry : Draft -> Validated (or posted)

Specs:

    add a date field in the accounting settings: tax lock date
    implement a constraint ensuring that no one can modify the information related to tax on account.move.line prior to this lock date. That means the debit, credit, tax_ids, tax_line_id fields. Other fields can be modified. This also means we cannot create/delete an account.move.line with tax information prior to this lock date.

Task 1914762

closes #29272

Signed-off-by: Quentin De Paoli (qdp) <qdp@openerp.com>
@robodoo
Copy link
Contributor

robodoo commented Mar 6, 2019

Merged, thanks!

@robodoo robodoo closed this Mar 6, 2019
@fw-bot fw-bot deleted the master-lock-date-wan branch October 20, 2019 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI 🤖 Robodoo has seen passing statuses RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants