[16.0][FIX] partner_industry_secondary: Fix industry populate

[vincent-hatakeyama]

Fix industry populate to take to handle required name.

The fix needs to be ported to 17.0 and 18.0 too, as it affects those versions too.
I have not looked in older versions.

[vincent-hatakeyama]

Populate tests is not done by OCA so it does not appear on codecov/path.

Odoo core puts populate methods in a separate directory and pylint-odoo complains about it. Is it expected to put the populate methods in the models directory?

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[StefanRijnhart]

Thanks! Populate seems to be a bit of a blind corner in the OCA. I was hardly aware myself.
You are right that Odoo keeps population code separately, but its still model code that is loaded during runtime, so for our purposes merging it into models is fine.

[vincent-hatakeyama]

I moved the populate fix in the models/<model>.py file.

[marcos-mendez]

Automated Review -- Tests Failed

1. Root cause of the test failure

The test failure is caused by a database connection error during the Odoo startup, likely due to an invalid or inaccessible database configuration in the test environment. This is not directly caused by the code changes in the PR but indicates an infrastructure issue.

2. Suggested fix

The test failure itself is not due to code changes in partner_industry_secondary, but to the test environment. However, to ensure correctness of the new _populate_factories method, make sure it's only called during data population, not during normal runtime.

File: partner_industry_secondary/models/res_partner_industry.py
Method: _populate_factories
Suggestion: Add a guard to ensure that _populate_factories is only used in a data population context (e.g., via self.env.context.get("populate")), or ensure that it's not called during normal Odoo operations.

3. Additional code issues

• Potential performance issue in _check_uniq_name: The method loops over self and performs a database query for each record, which is inefficient. It should be optimized to check uniqueness in bulk.

  Fix suggestion:
  Replace the loop with a single domain-based search and validate uniqueness across all records in self at once.

  def _check_uniq_name(self):
      if not self:
          return
      # Group records by name and parent_id
      groups = {}
      for rec in self:
          key = (rec.name, rec.parent_id.id)
          groups.setdefault(key, []).append(rec)
      # Check for duplicates
      for (name, parent_id), records in groups.items():
          if len(records) > 1:
              raise exceptions.ValidationError(
                  _("Error! Industry with same name and parent already exists.")
              )

4. Test improvements

Add tests to ensure:

• The _populate_factories method works correctly with populate.cartesian.
• The name field is properly populated with fallback values.
• The _check_uniq_name method properly detects and raises errors for duplicate industries.

Suggested test cases:

• TransactionCase: Test copy() and name field population.
• SavepointCase: Test _check_uniq_name with duplicate records.
• Tagged test: Add a test tagged with @tag('populate') to validate data factory behavior.

Example test snippet (TransactionCase):

def test_populate_factories(self):
    factories = self.env["res.partner.industry"]._populate_factories()
    name_factory = next((f for f in factories if f[0] == "name"), None)
    self.assertIsNotNone(name_factory)
    # Validate that it uses cartesian with expected values

Example test snippet (SavepointCase):

def test_check_uniq_name_duplicate(self):
    industry1 = self.env["res.partner.industry"].create({"name": "Test"})
    with self.assertRaises(exceptions.ValidationError):
        self.env["res.partner.industry"].create({
            "name": "Test",
            "parent_id": industry1.parent_id.id,
        })

These tests align with OCA patterns and ensure correctness of both new logic and existing constraints.

---

⚠️ PR Aging Alert: CRITICAL

This PR by @vincent-hatakeyama has been waiting for 186 days — that is over 6 months without being merged or closed.

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#3554 [MIG] datetime_formatter: Migration to 18.0
• 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

[BhaveshHeliconia]

• Here, multiple records with name=Industry name might be generated with parent_id=False.
• Due to the same name and parent, _check_uniq_name() might give error.
• Possible solution: Use ["Industry name {counter}"], [1.0]

[vincent-hatakeyama]

I applied your suggested changes and rebased on latest 16.0 branch.

[BhaveshHeliconia]

@vincent-hatakeyama could you please check? The pre-commit and test/ci checks are failing.

[vincent-hatakeyama]

@vincent-hatakeyama could you please check? The pre-commit and test/ci checks are failing.

pre-commit fails on all PR due to incorrect translation. I’ve added a fix for that. The rest works as expected now.

For the coverage, as populate is not run in OCA, it might be needed to ignore the unused method.

[BhaveshHeliconia]

LGTM!