Skip to content

[FIX] base: add ir_config_parameter with default value - #105991

Closed
thle-odoo wants to merge 1 commit into
odoo:saas-15.2from
odoo-dev:saas-15.2-opw-3057306-invoice_not_sent_automatically_eshop-thle
Closed

[FIX] base: add ir_config_parameter with default value#105991
thle-odoo wants to merge 1 commit into
odoo:saas-15.2from
odoo-dev:saas-15.2-opw-3057306-invoice_not_sent_automatically_eshop-thle

Conversation

@thle-odoo

Copy link
Copy Markdown
Contributor

Steps to reproduce:
- install sale_management module;
- go to settings of sale_management and check "Automatic invoice" parameter;
- in debug mode, we can see the template selected (do not change it);
- save the settings;
(This is just one example)

Issue:
The parameter is not added to the "ir_config_parameter" table.

Cause:
The improvement which consists in comparing the settings which one wishes to record with those which already exist in order to set the parameters which are different creates a problem.
Indeed, the current settings are retrieved directly from the model with their default value if there is one.
These are the values that will be used for the comparison.
Because of this, when we want to save a parameter which is set to its default value (on first save), the logic will not notice a difference.
Therefore, the parameter will not be saved in the "ir_config_parameter" table.

Consequence:
In the code, when we wanted to look up the value of a parameter, if it is not saved, we use its default value instead of saving it with its default value.
Example:
field_example = fields.Integer(string='Example', default=1000, config_parameter='model.field_example') value_field_example = params.get_param('model.field_example', default=1000)
The default value is repeated.

Solution:
To know whether or not we add a new parameter in the "ir_config_parameter" table, we must look at what already exists in the table and not in the settings defined in the model.

opw-3057306

@robodoo

robodoo commented Nov 18, 2022

Copy link
Copy Markdown
Contributor

Pull request status dashboard

@thle-odoo
thle-odoo force-pushed the saas-15.2-opw-3057306-invoice_not_sent_automatically_eshop-thle branch from 8fd15c3 to 11ab6c8 Compare November 18, 2022 07:59
@C3POdoo C3POdoo added the OE the report is linked to a support ticket (opw-...) label Nov 18, 2022
@thle-odoo
thle-odoo requested a review from adwid November 18, 2022 08:01

@adwid adwid left a comment

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.

lgtm
It may worth adding a test ?

As explained by the description, since [1], we only apply the settings that have changed. To do so, we compare the old and new res.config.settings. Moreover, in the code, we use the settings classified as config through the model ir.config.parameter, we don't read such settings field directly. For instance:

invoice_mail_template_id = fields.Many2one(
comodel_name='mail.template',
string='Invoice Email Template',
domain="[('model', '=', 'account.move')]",
config_parameter='sale.default_invoice_email_template',
default=lambda self: self.env.ref('account.email_template_edi_invoice', False)
)

default_template = self.env['ir.config_parameter'].sudo().get_param('sale.default_invoice_email_template')

Therefore, to know if a config setting has changed, we should not use the field on res.config.settings because its value could come from its default attribute and because, as shown above, this is not how a config setting is read/used. We should rather read the associated ir.config.parameter

[1] 76982c0

@thle-odoo
thle-odoo marked this pull request as ready for review November 18, 2022 09:37
@C3POdoo
C3POdoo requested review from a team November 18, 2022 09:38

@nle-odoo nle-odoo left a comment

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.

seems to make sense

side note, there is several cases where this would not be an issue:

  • if when we did get_param in the code using the ir.config_parameter for real, we used the same default value

  • if we set the ir.config_parameter to the same default value at module installation in data.

it's possible that this is the current intention of how things are done.

But with the difference between 15.0 (where we would write the ir.config_parameter in any case) and saas-15.2 this change make sense to me for the continuity of the behavior.

Comment thread odoo/addons/base/models/res_config.py Outdated
@thle-odoo
thle-odoo force-pushed the saas-15.2-opw-3057306-invoice_not_sent_automatically_eshop-thle branch from 11ab6c8 to cba0350 Compare November 18, 2022 13:19
@thle-odoo
thle-odoo requested a review from Feyensv November 18, 2022 14:58

@Feyensv Feyensv left a comment

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 seems that we are losing part of my performance improvement with this change, but this seems to be the right fix considering the problem 👍.

As Adrien suggested, a test would indeed be nice to make sure the behavior is still supported in the future ;).

side note, there is several cases where this would not be an issue:

  • if when we did get_param in the code using the ir.config_parameter for real, we used the same default value
  • if we set the ir.config_parameter to the same default value at module installation in data.

it's possible that this is the current intention of how things are done.

More like we didn't think of those cases that are wrongly implemented :D.
Those default values in the settings (but not in the code) are strange, as it means you could open the settings, save without modifying anything, and some parameters would have been modified (when you thought you didn't change anything).

Comment thread odoo/addons/base/models/res_config.py Outdated
@thle-odoo
thle-odoo force-pushed the saas-15.2-opw-3057306-invoice_not_sent_automatically_eshop-thle branch 3 times, most recently from 0f8f493 to 7479b96 Compare November 21, 2022 12:15
@thle-odoo
thle-odoo requested a review from Feyensv November 21, 2022 12:56
Steps to reproduce:
    - install sale_management module;
    - go to settings of sale_management and check "Automatic invoice" parameter;
    - in debug mode, we can see the template selected (do not change it);
    - save the settings;
    (This is just one example)

Issue:
    The parameter is not added to the "ir_config_parameter" table.

Cause:
    The improvement which consists in comparing the settings which one wishes to record with those which already exist in order to set the parameters which are different creates a problem.
    Indeed, the current settings are retrieved directly from the model with their default value if there is one.
    These are the values that will be used for the comparison.
    Because of this, when we want to save a parameter which is set to its default value (on first save), the logic will not notice a difference.
    Therefore, the parameter will not be saved in the "ir_config_parameter" table.

Consequence:
    In the code, when we wanted to look up the value of a parameter, if it is not saved, we use its default value instead of saving it with its default value.
    Example:
    ```
    field_example = fields.Integer(string='Example', default=1000, config_parameter='model.field_example')
    value_field_example = params.get_param('model.field_example', default=1000)

    ```
    The default value is repeated.

Solution:
    To know whether or not we add a new parameter in the "ir_config_parameter" table, we must look at what already exists in the table and not in the settings defined in the model.

opw-3057306
@Feyensv
Feyensv force-pushed the saas-15.2-opw-3057306-invoice_not_sent_automatically_eshop-thle branch from 7479b96 to a804076 Compare November 21, 2022 13:34

@Feyensv Feyensv left a comment

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.

Fixed the check and improved the test to cover the case when value would be False, making the check current_value == str(value) fail when it should have been truthy.

Thanks for the test and the fix 👍

@robodoo r+

robodoo pushed a commit that referenced this pull request Nov 21, 2022
Steps to reproduce:
    - install sale_management module;
    - go to settings of sale_management and check "Automatic invoice" parameter;
    - in debug mode, we can see the template selected (do not change it);
    - save the settings;
    (This is just one example)

Issue:
    The parameter is not added to the "ir_config_parameter" table.

Cause:
    The improvement which consists in comparing the settings which one wishes to record with those which already exist in order to set the parameters which are different creates a problem.
    Indeed, the current settings are retrieved directly from the model with their default value if there is one.
    These are the values that will be used for the comparison.
    Because of this, when we want to save a parameter which is set to its default value (on first save), the logic will not notice a difference.
    Therefore, the parameter will not be saved in the "ir_config_parameter" table.

Consequence:
    In the code, when we wanted to look up the value of a parameter, if it is not saved, we use its default value instead of saving it with its default value.
    Example:
    ```
    field_example = fields.Integer(string='Example', default=1000, config_parameter='model.field_example')
    value_field_example = params.get_param('model.field_example', default=1000)

    ```
    The default value is repeated.

Solution:
    To know whether or not we add a new parameter in the "ir_config_parameter" table, we must look at what already exists in the table and not in the settings defined in the model.

opw-3057306

closes #105991

Signed-off-by: Victor Feyens (vfe) <vfe@odoo.com>
@robodoo
robodoo temporarily deployed to merge November 21, 2022 15:00 Inactive
@robodoo robodoo closed this Nov 21, 2022
@fw-bot
fw-bot deleted the saas-15.2-opw-3057306-invoice_not_sent_automatically_eshop-thle branch December 5, 2022 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

OE the report is linked to a support ticket (opw-...)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants