[10.0][FIX+IMP] contract: Improve usability and don't fail on wrong data by pedrobaeza · Pull Request #130 · OCA/contract

pedrobaeza · 2017-12-02T15:13:55Z

Cron create invoices masked for avoiding silent errors
New constraints for assuring data consistency
UI helps for entering consistent data
Spanish translation
Remove double company_id field on form

pedrobaeza · 2017-12-02T15:14:24Z

Let's see if this one is the definitive one for total satisfaction, hehe

NL66278 · 2017-12-02T18:41:56Z

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


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

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

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?

Sorry recurring_invoices field, not recurring_next_invoice

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.

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

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

NL66278

I have one question, but this seems a whole set of very welcome improvements to me!

lasley

Thanks @pedrobaeza. Few minor verbiage changes, but otherwise solid. I do have the same question as @NL66278 though.

lasley · 2017-12-02T19:46:13Z

+        for contract in self.filtered('recurring_invoices'):
+            if not contract.recurring_next_date:
+                raise ValidationError(
+                    _("You must supply an invoicing next date for contract "


s/an invoicing next date/a next invoicing date/

lasley · 2017-12-02T19:46:29Z

+        for contract in self.filtered('recurring_invoices'):
+            if not contract.date_start:
+                raise ValidationError(
+                    _("You must supply an start date for contract '%s'") %


lasley · 2017-12-02T19:50:51Z

Oh and I guess the most important part - Travis is 🔴

pedrobaeza · 2017-12-02T20:13:14Z

Done!

rafaelbn

Tested functionally in runbot 👍

Just check before merging if you want to improve codecov/patch — 89.65% of diff hit (target 94.61%)

pedrobaeza · 2017-12-03T21:44:19Z

@rafaelbn coverage improved

lasley · 2017-12-04T00:24:31Z

            if (contract.date_start > ref_date or
                    contract.date_end and contract.date_end < ref_date):
-                if self.env.context.get('cron'):
+                if self.env.context.get('cron'):  # pragma: no cover


IMO this is cheating & should be removed. We should either take the coverage hit if we don't want to test it, or add the test (I'm good with either)

No, this is not cheating in this case. It's a case that doesn't modify anything and I don't want to be reported as a coverage failure.

There is still logic here, that if untested should yield a lack of coverage. Whether it modifies anything or not, the logic is still in the program and still does something different from the other things.

I'm not arguing against the lack of testing for this, as I agree it's a minimal point and probably doesn't need it. I am arguing against the false coverage report though, because this is not covered and is a unique logic path that should be tested per TDD standards.

Dave, not for this case. There's no possibility of leading to that situation right now with current constraints, so it's only a safeguard for already installed DBs, but no sense of testing that specific case (forcing through SQL) or to report as not covered.

Untouchable code is still untested IMO. What's the harm in having it show what it is? Hiding the fact that the block of code wasn't executed during testing is just sweeping the fact it can't be touched under the rug. There's always going to be scenarios like this, which is why 100% test coverage is an unrealistic goal in the practical sense.

I won't block for it - but I really feel that pragma: no cover should align explicitly with the example case provided in Coverage.py docs, which is to exclude debug code. Any production code that's not actually tested should show as untested.

OK, I give up due to your insistence...

* Cron create invoices masked for avoiding silent errors * New constraints for assuring data consistency * UI helps for entering consistent data * Spanish translation * Remove double company_id field on form

lasley · 2017-12-07T16:29:55Z

Thanks for removing the pragma Pedro! Runbot red is the full disk and unrelated to this, so I merge

…ata (#130) * [FIX+IMP] contract: Improve usability and don't fail on wrong data * Cron create invoices masked for avoiding silent errors * New constraints for assuring data consistency * UI helps for entering consistent data * Spanish translation * Remove double company_id field on form

pedrobaeza · 2017-12-11T09:22:25Z

Cherry-picked to v11 at 8d10f68

…ata (OCA#130) * [FIX+IMP] contract: Improve usability and don't fail on wrong data * Cron create invoices masked for avoiding silent errors * New constraints for assuring data consistency * UI helps for entering consistent data * Spanish translation * Remove double company_id field on form * [FIX] contract_sale_generation: Adapt tests to upstream contract