Skip to content

[10.0][FIX+IMP] contract: Improve usability and don't fail on wrong data #130

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

Merged
lasley merged 2 commits into from
Dec 7, 2017
Merged

[10.0][FIX+IMP] contract: Improve usability and don't fail on wrong data #130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dd6fc41
[FIX+IMP] contract: Improve usability and don't fail on wrong data
pedrobaeza Dec 2, 2017
bb2730d
[FIX] contract_sale_generation: Adapt tests to upstream contract
pedrobaeza Dec 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion contract/__manifest__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

{
'name': 'Contracts Management - Recurring',
'version': '10.0.3.2.0',
'version': '10.0.3.3.0',
'category': 'Contract Management',
'license': 'AGPL-3',
'author': "OpenERP SA, "
Expand Down
151 changes: 93 additions & 58 deletions contract/i18n/es.po
View file
Open in desktop

Large diffs are not rendered by default.

77 changes: 68 additions & 9 deletions contract/models/account_analytic_account.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,17 @@ class AccountAnalyticAccount(models.Model):
index=True,
default=lambda self: self.env.user,
)
create_invoice_visibility = fields.Boolean(
compute='_compute_create_invoice_visibility',
)

@api.depends('recurring_next_date', 'date_end')
def _compute_create_invoice_visibility(self):
for contract in self:
contract.create_invoice_visibility = (
not contract.date_end or
contract.recurring_next_date <= contract.date_end
)

@api.onchange('contract_template_id')
def _onchange_contract_template_id(self):
Expand All @@ -75,15 +86,60 @@ def _onchange_contract_template_id(self):
)):
self[field_name] = self.contract_template_id[field_name]

@api.onchange('recurring_invoices')
def _onchange_recurring_invoices(self):
if self.date_start and self.recurring_invoices:
@api.onchange('date_start')
def _onchange_date_start(self):

@NL66278 NL66278 Dec 2, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question, what is the reasoning for not testing on recurring_invoices?

@pedrobaeza pedrobaeza Dec 2, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

With this, now you don't need to care about that field: the recurring_next_invoice will always be synchronized.

@lasley lasley Dec 2, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like we're also implicitly assuming that it's a recurring contract in the event of a start_date (IMO a great simplification). With that though, could we then just outright remove the recurring_next_invoice field?

@lasley lasley Dec 2, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry recurring_invoices field, not recurring_next_invoice

@pedrobaeza pedrobaeza Dec 3, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Previous code assumes the same: it took date_start when you mark recurring_invoices field. What you are talking about is to take today when marking recurring_invoices. It can be another thing to add indeed.

What I don't see correct is to remove recurring_invoices field, as it serves a lot for UI usability and user experience. Having that check, you clean the interface out of fields that aren't important if you don't use recurring invoicing. Note that in this PR, I have improved also the experience automarking by default the field when clicking on create when you are in the "Contracts" menu.

@lasley lasley Dec 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the elaboration Pedro. Does it make sense to actually store recurring_invoices, given that it is really just a functional field for triggering of specific workflows?

Just spitballing other upgrades here seeing as we have these nice improvements in similar areas. Not blocking

@pedrobaeza pedrobaeza Dec 6, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well, I prefer to keep it right now, or we would need to change a lot of filters, context values...

if self.date_start:
self.recurring_next_date = self.date_start

@api.onchange('partner_id')
def _onchange_partner_id(self):
self.pricelist_id = self.partner_id.property_product_pricelist.id

@api.constrains('partner_id', 'recurring_invoices')
def _check_partner_id_recurring_invoices(self):
for contract in self.filtered('recurring_invoices'):
if not contract.partner_id:
raise ValidationError(
_("You must supply a customer for the contract '%s'") %
contract.name
)

@api.constrains('recurring_next_date', 'date_start')
def _check_recurring_next_date_start_date(self):
for contract in self.filtered('recurring_next_date'):
if contract.date_start > contract.recurring_next_date:
raise ValidationError(
_("You can't have a next invoicing date before the start "
"of the contract '%s'") % contract.name
)

@api.constrains('recurring_next_date', 'recurring_invoices')
def _check_recurring_next_date_recurring_invoices(self):
for contract in self.filtered('recurring_invoices'):
if not contract.recurring_next_date:
raise ValidationError(
_("You must supply a next invoicing date for contract "
"'%s'") % contract.name
)

@api.constrains('date_start', 'recurring_invoices')
def _check_date_start_recurring_invoices(self):
for contract in self.filtered('recurring_invoices'):
if not contract.date_start:
raise ValidationError(
_("You must supply a start date for contract '%s'") %
contract.name
)

@api.constrains('date_start', 'date_end')
def _check_start_end_dates(self):
for contract in self.filtered('date_end'):
if contract.date_start > contract.date_end:
raise ValidationError(
_("Contract '%s' start date can't be later than end date")
% contract.name
)

@api.multi
def _convert_contract_lines(self, contract):
self.ensure_one()
Expand Down Expand Up @@ -203,39 +259,42 @@ def _create_invoice(self):

@api.multi
def recurring_create_invoice(self):
"""
Create invoices from contracts
"""Create invoices from contracts

:return: invoices created
"""
invoices = self.env['account.invoice']
for contract in self:
ref_date = contract.recurring_next_date or fields.Date.today()
if (contract.date_start > ref_date or
contract.date_end and contract.date_end < ref_date):
if self.env.context.get('cron'):
continue # Don't fail on cron jobs
raise ValidationError(
_("You must review start and end dates!\n%s") %
contract.name)
contract.name
)
old_date = fields.Date.from_string(ref_date)
new_date = old_date + self.get_relative_delta(
contract.recurring_rule_type, contract.recurring_interval)
ctx = self.env.context.copy()
ctx.update({
'old_date': old_date,
'next_date': new_date,
# Force company for correct evaluate domain access rules
# Force company for correct evaluation of domain access rules
'force_company': contract.company_id.id,
})
# Re-read contract with correct company
invoices |= contract.with_context(ctx)._create_invoice()
contract.write({
'recurring_next_date': new_date.strftime('%Y-%m-%d')
'recurring_next_date': fields.Date.to_string(new_date)
})
return invoices

@api.model
def cron_recurring_create_invoice(self):
today = fields.Date.today()
contracts = self.search([
contracts = self.with_context(cron=True).search([
('recurring_invoices', '=', True),
('recurring_next_date', '<=', today),
'|',
Expand Down
73 changes: 48 additions & 25 deletions contract/tests/test_contract.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,18 +65,14 @@ def test_contract(self):
res = self.acct_line._onchange_product_id()
self.assertIn('uom_id', res['domain'])
self.acct_line.price_unit = 100.0

self.contract.partner_id = False
with self.assertRaises(ValidationError):
self.contract.recurring_create_invoice()
self.contract.partner_id = False
self.contract.partner_id = self.partner.id

self.contract.recurring_create_invoice()
self.invoice_monthly = self.env['account.invoice'].search(
[('contract_id', '=', self.contract.id)])
self.assertTrue(self.invoice_monthly)
self.assertEqual(self.contract.recurring_next_date, '2016-03-29')

self.inv_line = self.invoice_monthly.invoice_line_ids[0]
self.assertTrue(self.inv_line.invoice_line_tax_ids)
self.assertAlmostEqual(self.inv_line.price_subtotal, 50.0)
Expand Down Expand Up @@ -129,11 +125,11 @@ def test_onchange_partner_id(self):
self.assertEqual(self.contract.pricelist_id,
self.contract.partner_id.property_product_pricelist)

def test_onchange_recurring_invoices(self):
self.contract.recurring_next_date = False
self.contract._onchange_recurring_invoices()
self.assertEqual(self.contract.recurring_next_date,
self.contract.date_start)
def test_onchange_date_start(self):
date = '2016-01-01'
self.contract.date_start = date
self.contract._onchange_date_start()
self.assertEqual(self.contract.recurring_next_date, date)

def test_uom(self):
uom_litre = self.env.ref('product.product_uom_litre')
Expand All @@ -160,6 +156,31 @@ def test_check_journal(self):
with self.assertRaises(ValidationError):
contract_no_journal.recurring_create_invoice()

def test_check_date_end(self):
with self.assertRaises(ValidationError):
self.contract.date_end = '2015-12-31'

def test_check_recurring_next_date_start_date(self):
with self.assertRaises(ValidationError):
self.contract.write({
'date_start': '2017-01-01',
'recurring_next_date': '2016-01-01',
})

def test_check_recurring_next_date_recurring_invoices(self):
with self.assertRaises(ValidationError):
self.contract.write({
'recurring_invoices': True,
'recurring_next_date': False,
})

def test_check_date_start_recurring_invoices(self):
with self.assertRaises(ValidationError):
self.contract.write({
'recurring_invoices': True,
'date_start': False,
})

def test_onchange_contract_template_id(self):
"""It should change the contract values to match the template."""
self.contract.contract_template_id = self.template
Expand Down Expand Up @@ -241,24 +262,14 @@ def test_contract_count(self):
self.contract.copy()
self.assertEqual(self.partner.contract_count, count)

def test_date_end(self):
"""It should don't create invoices from finished contract."""
AccountInvoice = self.env['account.invoice']
self.contract.date_end = '2015-12-31'
with self.assertRaises(ValidationError):
self.contract.recurring_create_invoice()
init_count = AccountInvoice.search_count(
[('contract_id', '=', self.contract.id)])
self.contract.cron_recurring_create_invoice()
last_count = AccountInvoice.search_count(
[('contract_id', '=', self.contract.id)])
self.assertEqual(last_count, init_count)

def test_same_date_start_and_date_end(self):
"""It should create one invoice with same start and end date."""
AccountInvoice = self.env['account.invoice']
self.contract.date_start = self.contract.date_end = fields.Date.today()
self.contract.recurring_next_date = self.contract.date_start
self.contract.write({
'date_start': fields.Date.today(),
'date_end': fields.Date.today(),
'recurring_next_date': fields.Date.today(),
})
init_count = AccountInvoice.search_count(
[('contract_id', '=', self.contract.id)])
self.contract.cron_recurring_create_invoice()
Expand All @@ -267,3 +278,15 @@ def test_same_date_start_and_date_end(self):
self.assertEqual(last_count, init_count + 1)
with self.assertRaises(ValidationError):
self.contract.recurring_create_invoice()

def test_compute_create_invoice_visibility(self):
self.contract.write({
'recurring_next_date': '2017-01-01',
'date_start': '2016-01-01',
'date_end': False,
})
self.assertTrue(self.contract.create_invoice_visibility)
self.contract.date_end = '2017-01-01'
self.assertTrue(self.contract.create_invoice_visibility)
self.contract.date_end = '2016-01-01'
self.assertFalse(self.contract.create_invoice_visibility)
23 changes: 17 additions & 6 deletions contract/views/account_analytic_account_view.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
<field name="mode">primary</field>
<field name="priority" eval="9999"/>
<field name="arch" type="xml">
<field name="partner_id" position="attributes">
<attribute name="attrs">{'required': [('recurring_invoices', '=', True)]}</attribute>
</field>
<xpath expr="//div[@name='button_box']/.." position="before">
<header>
<button name="action_contract_send" type="object" string="Send by Email" groups="base.group_user"/>
Expand All @@ -19,10 +22,11 @@
/>
<div>
<field name="recurring_invoices" class="oe_inline"/>
<field name="create_invoice_visibility" invisible="1"/>
<label for="recurring_invoices" />
<button name="recurring_create_invoice"
type="object"
attrs="{'invisible': [('recurring_invoices','!=',True)]}"
attrs="{'invisible': ['|', ('recurring_invoices', '!=', True), ('create_invoice_visibility', '=', False)]}"
string="Create invoices"
class="oe_link"
groups="base.group_no_one"
Expand All @@ -36,9 +40,10 @@
</div>
<group col="4" attrs="{'invisible': [('recurring_invoices','!=',True)]}">
<field name="contract_template_id" colspan="4"/>
<field name="journal_id"/>
<field name="journal_id"
attrs="{'required': [('recurring_invoices', '=', True)]}"
/>
<field name="pricelist_id"/>
<field name="company_id" groups="base.group_multi_company"/>
<label for="recurring_interval"/>
<div>
<field name="recurring_interval"
Expand All @@ -50,10 +55,16 @@
attrs="{'required': [('recurring_invoices', '=', True)]}"
/>
</div>
<field name="recurring_invoicing_type"/>
<field name="date_start"/>
<field name="recurring_invoicing_type"
attrs="{'required': [('recurring_invoices', '=', True)]}"
/>
<field name="date_start"
attrs="{'required': [('recurring_invoices', '=', True)]}"
/>
<field name="date_end"/>
<field name="recurring_next_date"/>
<field name="recurring_next_date"
attrs="{'required': [('recurring_invoices', '=', True)]}"
/>
</group>
<label for="recurring_invoice_line_ids"
attrs="{'invisible': [('recurring_invoices','=',False)]}"
Expand Down
2 changes: 1 addition & 1 deletion contract/views/account_analytic_contract_view.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@
<filter string="Recurrence"
context="{'group_by': 'recurring_rule_type'}"
/>
<filter string="Invoicing Type"
<filter string="Invoicing type"
context="{'group_by': 'recurring_invoicing_type'}"
/>
<filter string="Pricelist"
Expand Down
2 changes: 1 addition & 1 deletion contract/views/res_partner_view.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
<xpath expr="//div[@name='button_box']" position="inside">
<button name="act_show_contract" type="object" class="oe_stat_button"
icon="fa-book"
help="show the contracts for this partner">
help="Show the contracts for this partner">
<field name="contract_count" widget="statinfo" string="Contracts"/>
</button>
</xpath>
Expand Down
13 changes: 0 additions & 13 deletions contract_sale_generation/tests/test_contract_sale.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,19 +57,12 @@ def test_contract(self):
res = self.contract_line._onchange_product_id()
self.assertIn('uom_id', res['domain'])
self.contract_line.price_unit = 100.0

self.contract.partner_id = False
with self.assertRaises(ValidationError):
self.contract.recurring_create_sale()
self.contract.partner_id = self.partner.id

self.contract.recurring_create_sale()
self.sale_monthly = self.env['sale.order'].search(
[('project_id', '=', self.contract.id),
('state', '=', 'draft')])
self.assertTrue(self.sale_monthly)
self.assertEqual(self.contract.recurring_next_date, '2017-02-28')

self.sale_line = self.sale_monthly.order_line[0]
self.assertAlmostEqual(self.sale_line.price_subtotal, 50.0)
self.assertEqual(self.contract.partner_id.user_id,
Expand All @@ -81,12 +74,6 @@ def test_contract_autoconfirm(self):
res = self.contract_line._onchange_product_id()
self.assertIn('uom_id', res['domain'])
self.contract_line.price_unit = 100.0

self.contract.partner_id = False
with self.assertRaises(ValidationError):
self.contract.recurring_create_sale()
self.contract.partner_id = self.partner.id

self.contract.recurring_create_sale()
self.sale_monthly = self.env['sale.order'].search(
[('project_id', '=', self.contract.id),
Expand Down