[19.0][MIG] rma: Migration from 18.0 to 19.0

[eugenios73]

Additional changes included during migration

The migration preserves the existing functional behavior of the module. However,
during the adaptation, some pre-existing issues were identified and fixed, and
test coverage was added for behaviors that could suffer regressions in Odoo 19.

Fix in multiple receptions preparation

The method that prepares reception procurements iterates over each RMA in the
recordset, but some values were obtained from self instead of using the
rma record being processed in each iteration.

This behavior did not usually affect single RMA confirmations, but it could trigger
singleton errors or use data from an incorrect RMA when confirming multiple
returns with different products or operations together.

The preparation now consistently uses the product, return product configuration,
and alternative product from the RMA being processed in each iteration. This fixes
the existing confirmation and batch grouping flow without introducing new
functionality.

Explicit account assignment on refund lines

Credit note lines created directly from an RMA now explicitly receive the income
account configured for the product in the RMA's company.

The module creates these lines programmatically without going through the invoice
form view. Resolving the account during value preparation avoids relying on
implicit environment defaults and ensures that the generated line has a valid,
company-consistent accounting configuration.

The test configuration also explicitly creates a sales journal and an income
account. This avoids relying on demo data, which is no longer installed by
default in Odoo 19. The refund flow test additionally verifies that the expected
account is assigned to the resulting line.

Fix in reception grouping test coverage

The test verifying reception grouping was accidentally defined inside another test
method. As a result, the framework did not discover or execute it.

The test has been converted into an independent method to ensure that two
compatible RMAs are grouped into the same picking.

An assertion that compared the reference of the first RMA against itself (and thus
always returned a positive result) has also been fixed. Now, it verifies that RMAs
that should be grouped share the same reference, and that an RMA belonging to
another customer receives a different reference.

These changes do not modify the grouping rules: they ensure that the existing
behavior is properly covered against regressions.

Suggested recipient coverage in chatter

A test has been added to verify that the RMA customer continues to appear among
the suggested recipients when composing a message.

This coverage protects the existing functional behavior after adapting the
customization to Odoo 19's new suggested recipients API. The goal is not to change
which contacts are proposed, but to guarantee that the customer remains available
as a recipient after migration.

Picking dates coverage in portal

A dedicated HTTP test has been added for the RMA portal. The test creates an RMA
with automatic reception and delivery, initially accesses its portal page, validates
the related pickings, and accesses the page again.

This test covers portal rendering for both pending and completed pickings. Its
purpose is to catch template errors when rendering the scheduled date before
transfer and the effective date after validation.

This coverage accompanies the replacement of the old stock.picking.date field:
pending pickings show scheduled_date and completed ones show date_done.
Dates are displayed only when available and without including the time component,
keeping portal information aligned with the actual transfer state.

Functional scope

Beyond the described fixes, no new business functionality is intended to be
introduced. The remaining changes respond to framework, model, and API updates
required to run the module on Odoo 19.

Coverage Note

Functional tests pass successfully with both Odoo and OCB.

The failure of codecov/project does not represent an actual regression in coverage. The base branch 19.0 did not yet contain the rma module, and its report only measured 71 lines—mostly belonging to product_warranty—with a coverage of 98.59%.

With the addition of rma, Codecov now measures 2,360 lines. The resulting overall coverage is 89.74%, which appears as an 8.85% drop even though the compared sets have completely different scopes. In practice, the coverage of an almost empty branch is being compared to the full coverage of the migrated module.

The codecov/patch check is independent of this scope difference and currently shows 64%. Targeted functional tests are being added for the lines modified during the migration to improve this coverage, without lowering thresholds or excluding code from analysis.

Therefore:

• codecov/project mainly reflects the addition of a complete module that did not yet exist in the base branch.
• codecov/patch reflects the coverage of the changes in this migration and is the indicator currently being addressed with additional tests.
• Test suites with Odoo and OCB execute successfully.

[victoralmau]

Sorry, but I don't understand the suggestions to remove filtered to achieve the same result conceptually.
I think it's simply a matter of code design preferences and isn't related to the migration.

[Reyes4711-S73]

@victoralmau
Both options are O(n), but a for loop with continue is usually slightly more efficient because it does not create an intermediate recordset or execute a lambda:

for line in self.line_ids:
    if not line.is_valid:
        continue
    # Process line

Using filtered() can be more expressive:

for line in self.line_ids.filtered("is_valid"):
    # Process line

My rule of thumb:

• Use continue if you only need the filtering for that loop or the condition is complex.
• Use filtered() if it clearly improves readability or you will reuse the filtered recordset.
• If you can avoid loading unnecessary lines by using a search() with a domain, that will have a much greater impact than choosing between these two options.

In practice, the difference is negligible for small recordsets, so prioritize readability. For large loops, continue has a slight performance advantage.

Also note that filtered() filters the recordset in Python; it does not execute a SQL query.

(thanks codex)

[pablo-cort-s73]

LGTM

[victoralmau]

@victoralmau Both options are O(n), but a for loop with continue is usually slightly more efficient because it does not create an intermediate recordset or execute a lambda:

for line in self.line_ids:
    if not line.is_valid:
        continue
    # Process line

Using filtered() can be more expressive:

for line in self.line_ids.filtered("is_valid"):
    # Process line

My rule of thumb:

* Use `continue` if you only need the filtering for that loop or the condition is complex.

* Use `filtered()` if it clearly improves readability or you will reuse the filtered recordset.

* If you can avoid loading unnecessary lines by using a `search()` with a domain, that will have a much greater impact than choosing between these two options.

In practice, the difference is negligible for small recordsets, so prioritize readability. For large loops, continue has a slight performance advantage.

Also note that filtered() filters the recordset in Python; it does not execute a SQL query.

(thanks codex)

What do you think @pedrobaeza ?

[pedrobaeza]

@Reyes4711-S73 @victoralmau there's still a more important thumb rule: on a migration, you should reduce the diff at maximum, so it's better to keep the previous code style even if it doesn't match your preferences unless there's an important reason for changing it (for example, an important performance draining). And even though, it can go into a separate commit [REF] or [PERF] one for proper diff reading.

Due to the scope of the filtered, that doesn't happen, and even more important, your thumb rule with the new ORM cache system is not so important, as data is fetched in batch only once, but using filtered or just the loop over self. And accessing the fields for the condition of the filter or the if takes the same time. In previous versions, there was some kind of double processing, but this is no longer true.

Conclusion: please revert those changes and keep migration commit minimal.

And another thing: the commit message shouldn't contain the prefix [19.0].

[eugenios73]

@pedrobaeza @victoralmau @Reyes4711-S73 The filtered changes have been reverted and the [19.0] prefix has been removed from the commit message and increased test coverage. Please, can you review?

[victoralmau]

Please, cherry-pick #624 to commit history before migration commit.

[eugenios73]

@victoralmau I've already added the cherry-pick