[FIX] models: do no allow voiding a required fields #164712
Conversation
|
|
C3POdoo
requested review from
a team,
rco-odoo and
HydrionBurst
and removed request for
a team
| @@ -310,23 +310,6 @@ def test_mail_mail_send_exceptions_origin(self): | |||
| self.assertEqual(notification.failure_type, 'mail_from_missing') | |||
| self.assertEqual(notification.notification_status, 'exception') | |||
|
|
|||
| # MailServer.send_email(): _prepare_email_message: unexpected ASCII / Malformed 'Return-Path' or 'From' address | |||
There was a problem hiding this comment.
Don't remove it, the purpose is to check malformed email_from, it should still be tested.
There was a problem hiding this comment.
What would the expected behavior be?
Right now, it will fail on self.mail_alias_domain.bounce_alias = False
But: before, if you did a self.env.flush_all(), it would go 💥
The test is not realistic, it only works mid-transaction, so not when receiving emails.
There was a problem hiding this comment.
The purpose is not to test bounce_alias = False, it is to test malformed email_from. I think the alias reset was just a trick during the test, don't have much time to dive into it but maybe you can try simply removing alias domains completely, should not try to use them.
There was a problem hiding this comment.
Got it: I fixed it by forcing the bounce to be the same value as email_from (using with_context(domain_bounce_address=email_from))
|
As it seems to be only a nice to have improvement, why do you target stable ? |
There was a problem hiding this comment.
Changing exception types is changing the API, so this is clearly not acceptable on a stable branch. The proof being that you broke existing tests.
That being said, the error message is indeed much more informative about what's wrong with the values. Moreover, without this change, the error is triggered when the update is flushed, instead of when the value is assigned, which makes debugging more complex.
I suggest to:
- make a branch for master;
- add some specific tests on the feature itself;
- cover the case of
create(),write()and stored field assignment (in compute).
william-andre
force-pushed
the
17.0-require-required-fields-wan
branch
from
e9831df
to
cc82929
Compare
william-andre
force-pushed
the
17.0-require-required-fields-wan
branch
4 times, most recently
from
41a6f2e
to
d2217ba
Compare
Display a nice error instead of a SQL error It could make sense in some cases to be able to void a field in order to recompute the the default value, but that should only happen in `onchange`
william-andre
force-pushed
the
17.0-require-required-fields-wan
branch
from
d2217ba
to
3a12711
Compare
Display a nice error instead of a SQL error
It could make sense in some cases to be able to void a field in order to recompute the the default value, but that should only happen in
onchange