[16.0][account_invoice_constraint_chronology] better error message #2035
Conversation
rvalyi
force-pushed
the
16.0-fix-account_invoice_constraint_chronology-ak
branch
from
b32c505 to
e19db35
Compare
|
hum it seems #2032 (review) is actually a nicer fix than my 1st commit. Putting it in draft and I'll probably remove my 1st commit, stay tuned. |
There was a problem hiding this comment.
Following the line I mentioned in #2032, I think it's also applicable here. I'm sharing the comments to see what you all think.
| @@ -95,6 +100,8 @@ def _conflicting_inv_after_sequence_before_inv_date_domain(self): | |||
| return expression.AND( | |||
| [ | |||
| ( | |||
| ("journal_id", "=", self.journal_id.id), | |||
| ("move_type", ">", self.move_type), | |||
There was a problem hiding this comment.
I don't understand the ">" operator in the move_type comparisons... ¿?
There was a problem hiding this comment.
wow that was a typo indeed.
There was a problem hiding this comment.
Ok, we must take into account that if the journal has the "different sequence" checkbox marked for refunds (refund_sequence), then it's true that the move_type should be checked. However, if not, the situation changes... there could be sequence conflicts among different types (https://github.com/OCA/account-financial-tools/blob/16.0/account_invoice_constraint_chronology/model/account_move.py#L14).
In my opinion, it would be best to unify the domain methods to avoid confusion and duplicate code.
What do you all think?
rvalyi
force-pushed
the
16.0-fix-account_invoice_constraint_chronology-ak
branch
from
e19db35 to
ca57806
Compare
rvalyi
changed the title
|
@DarioLodeiros I just removed my 1st commit as it is covered by your PR #2032 |
The error message in case of an older conflicting invoice was not really helpful. Now it will show the conflicting invoice name if any and also the customer name (because a draft invoice will likely be named '/' so just the name might not be enough). This will help the user to find the offending invoice. It also suggest to removed the invoice date as a possible work around.
rvalyi
force-pushed
the
16.0-fix-account_invoice_constraint_chronology-ak
branch
from
ca57806 to
2969978
Compare
|
This PR has the |
| "Chronology conflict: A conflicting draft invoice {name} for " | ||
| "{partner} dated before {date_invoice} exists, please validate it " | ||
| "first or remove its invoice date." | ||
| ).format( |
There was a problem hiding this comment.
You should use translation function arguments instead of format()
There was a problem hiding this comment.
Shouldn't the translation functions be automatically validated with precommit? I thought that if the precommit was passing, it was because it was a valid format (in my personal experience, I get an error if I don't use the translation function).
There was a problem hiding this comment.
Yes, but this is new from >= v16. So, maybe you can propose a change in pylint-odoo.
| "Chronology conflict: A conflicting draft invoice {name} for " | ||
| "{partner} dated before {date_invoice} exists, please validate it " | ||
| "first or remove its invoice date." | ||
| ).format( |
There was a problem hiding this comment.
| "Chronology conflict: A conflicting draft invoice {name} for " | |
| "{partner} dated before {date_invoice} exists, please validate it " | |
| "first or remove its invoice date." | |
| ).format( | |
| "Chronology conflict: A conflicting draft invoice %(name)s for " | |
| "%(partner)s dated before %(date_invoice)s exists, please validate it " | |
| "first or remove its invoice date." |
@rvalyi would you this before merging?
There was a problem hiding this comment.
Will try to find the time, carnival here :-)
The error message in case of an older conflicting invoice was not really
helpful. Now it will show the conflicting invoice name if any and also
the customer name (because a draft invoice will likely be named '/' so
just the name might not be enough). This will help the user to find
the offending invoice. It also suggest to remove the invoice date
as a possible work around.
cc @DarioLodeiros @sbidoul @alexis-via @sebastienbeau @florian-dacosta @pedrobaeza @etobella @DarioLodeiros