[FIX] Surround setting view added fields by more standard markup

[fcayre]

A div tag with the o_setting_box class is added around the fields and their label.

This seems to be a more standard markup among the OCA. At least the remove_odoo_enterprise module expects it, see issue OCA/server-brand#116

[fcayre]

This is how it looks like (Configuration > General settings > Invoicing > Contract section) with proposed markup.

[Honeyxilia]

Code review + visual test OK

[remi-filament]

It works, though I think you do not need to create 2 separate o_settings_container div, since these are related.
You probably could use the following instead :

        <xpath expr="//div[div[div[field[@name='create_new_line_at_contract_line_renew']]]]" position="after">
                <div class="row mt16 o_settings_container">
                    <div class="col-12 col-lg-6 o_setting_box">
                        <div class="o_setting_left_pane">
                            <field name="enable_contract_forecast"/>
                        </div>
                        <div class="o_setting_right_pane">
                            <label for="enable_contract_forecast"/>
                            <div class="row mt16" attrs="{'invisible': [('enable_contract_forecast', '=', False)]}">
                                <label for="contract_forecast_interval" string="Forecast Interval" class="oe_inline"/>
                                <div>
                                    <field name="contract_forecast_interval" class="oe_inline"/>
                                    <field name="contract_forecast_rule_type" class="oe_inline"/>
                                </div>
                            </div>
                        </div>
                    </div>
                </div>
            </xpath>

(also I find it easier to read xpath using div[xxx] rather than xxx/parent::div so I proposed that change as well in code snippet above)

[fcayre]

@remi-filament thanks for the suggestion. While we're at improving the xpath I made a different suggestion though, please review.

cc @Honeyxilia for a new review also please.

[remi-filament]

Hi @fcayre thanks for this improvement, my comments below (none are blocking) :

1. Good catch with /ancestor which is more readable than multiple parents, though IMO it is harder to understand than imbricated div (div[div[div[field[]]]]), but this is my personal pov
2. I still think that you could have the second part with interval in the same
   than the enable_contract_forecast boolean (which also makes sense since these parameters are related and could therefore be grouped in the same o_setting_box div)

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[etobella]

Use a proper commit name please

[marcos-mendez]

Automated Review -- Tests Failed

1. Root Cause

The test failure is caused by a missing or unavailable module required during the installation of contract_forecast. The error indicates that Odoo failed to load the registry due to an issue with module installation, likely because of a dependency not being present in the test environment.

---

2. Suggested Fix

The root issue is not in the XML view change itself but in the test setup or missing dependencies. Ensure all required modules (including those listed in depends in __manifest__.py) are installed in the test environment before running the tests.

File to check: contract_forecast/__manifest__.py
Action: Verify that all dependencies listed under depends are available and properly installed in the test environment.

Also, consider adding a post_load hook or ensuring the module is loaded after its dependencies in test setup.

---

3. Additional Code Issues

No real bugs found in the provided diff. The XPath change is valid and follows Odoo's view inheritance patterns. The use of ancestor::div[hasclass('o_settings_container')] is a correct way to target the container div, and the layout reorganization is consistent with Odoo's settings UI practices.

---

4. Test Improvements

To better cover the changed code, consider adding or enhancing tests in the following areas:

• Test res_config_settings view rendering:
  Use TransactionCase or SavepointCase to ensure the settings form is correctly rendered and fields are visible/invisible based on enable_contract_forecast.

  def test_contract_forecast_settings_visibility(self):
      # Ensure fields are visible when enable_contract_forecast = True
      # and hidden when False
      config = self.env['res.config.settings'].create({
          'enable_contract_forecast': True,
      })
      # Test that contract_forecast_interval and contract_forecast_rule_type are visible

• Test configuration save and load:
  Ensure that saving and loading settings works correctly, especially for the new field layout.

  def test_contract_forecast_settings_save(self):
      config = self.env['res.config.settings'].create({
          'enable_contract_forecast': True,
          'contract_forecast_interval': 30,
          'contract_forecast_rule_type': 'daily',
      })
      config.execute()
      # Verify settings are saved and loaded correctly

• Use tagged tests for settings UI:
  Tag tests with @tag('post_install') or @tag('settings') to ensure they run in the correct context.

---

Summary

• The failure is due to a missing module, not the code change.
• Ensure all dependencies are present in the test environment.
• No bugs in the diff itself.
• Add tests covering visibility and save/load behavior of new settings fields.

---

⏰ This PR has been open for 87 days.
💤 Last activity was 81 days ago.

Every ignored PR is a contributor who might not come back. Review time matters. (OCA Aging Report)

---

Reciprocal Review Request

Hi everyone! I found some test failures on this PR and left detailed feedback above. I am happy to discuss or help debug. In the meantime, if any of you get a chance, I would appreciate a look at my open PR(s):

My open PRs across OCA:

• server-tools#3548 [18.0][MIG] base_kanban_stage: Migration to 18.0
• hr-attendance#262 [16.0][ADD] Hr_attendance_idsecure: iDSecure (ControliD) attendance integration
• stock-logistics-workflow#2276 [16.0][ADD] stock_move_line_devaluation
• stock-logistics-workflow#2275 [16.0][ADD] Stock move line analytic account
• stock-logistics-workflow#2268 [16.0][ADD] stock_move_line_picking_partner
• purchase-workflow#2694 [16.0][IMP]Purchase workflow added to review state & exception fix

Reviewing each other's work helps the whole community move forward. Thank you!

---

Environment via OCA Neural Reviewer: Minikube + K8s Job + oca-ci/py3.10-odoo16.0 | Odoo 16.0
Automated review by OCA Neural Reviewer + qwen3-coder:30b