[FIX] portal_backend: assert the parent menus group on registry load - #436
Closed
rov-adhoc wants to merge 1 commit into
Closed
[FIX] portal_backend: assert the parent menus group on registry load#436rov-adhoc wants to merge 1 commit into
rov-adhoc wants to merge 1 commit into
Conversation
Contributor
|
|
There was a problem hiding this comment.
Pull request overview
Este PR corrige un efecto colateral en portal_backend donde se escribía en ir.ui.menu desde load_menus (ruta readonly), provocando errores de transacción de solo lectura en logs y fallos en CI/tours. La solución mueve la autocorrección de grupos a _register_hook, que corre con cursor de lectura/escritura durante la carga del registry.
Changes:
- Mueve la asignación de
base.group_usera menús raíz sin grupos desdeload_menusa_register_hook. - Elimina el
ormcachey el override deload_menus, evitando escrituras desde requests web readonly. - Evita la escritura cuando no hay menús que corregir.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Granting base.group_user to the parent menus without group was done while loading the menus, but /web/webclient/load_menus is a readonly route, so the write breaks its cursor with "cannot execute INSERT in a read-only transaction". The request itself survives -it is retried with a read/write cursor- but the failed query is already logged as an error, which is enough to turn a test run red. On a real database it happened once, since the retry commits the group; on tests it happens on every tour that loads the webclient, because each test rolls that write back. Doing it on _register_hook keeps the case covered -another module installing a parent menu without internal group after portal_backend- and runs on a read/write cursor: the hook is called when the registry is loaded, which is what installing or updating a module does.
rov-adhoc
force-pushed
the
19.0-t-64440-rov
branch
from
August 19, 2026 16:04
f087943 to
64b57b1
Compare
Contributor
|
@roboadhoc r+ bump |
roboadhoc
pushed a commit
to ingadhoc/product
that referenced
this pull request
Aug 26, 2026
…he value adjustment Task 64440, functional feedback. Changing the accounting cost of a product with perpetual valuation posts the revaluation entry here, but the ``product.value`` the standard records for that same price change was left with no reference to it. ``stock_account_ux`` reads "no entry" as "still part of the difference to adjust", so the adjustment kept showing up as pending in the inventory valuation report and the closing booked it a second time — telling booked from pending is the whole point of that field. ``_link_cost_revaluation_entry`` points the record at the entry right after posting it: the most recent price change of the product in the company still without an entry, which is the one ``_change_standard_price`` just created. Lot price changes are left alone —they record their own ``product.value`` and this entry values the product's on-hand stock, not a lot's. When no entry is posted (no revaluation account, no stock, FIFO, periodic category) there is nothing to link and the adjustment stays pending, as it should. ``stock_account_ux`` joins ``depends``: it is where ``account_move_id`` comes from. It is ``auto_install`` over ``stock_account``, which this module already depends on, so it is installed in every database that has this one anyway. Test plan: 2 new tests, 9 green in the module on a fresh database. The linking one is red without the fix. closes #937 Related: ingadhoc/stock#1004 Related: ingadhoc/account-financial-tools#984 Related: ingadhoc/miscellaneous#436 Signed-off-by: Filoquin adhoc <maq@adhoc.com.ar>
roboadhoc
pushed a commit
to ingadhoc/stock
that referenced
this pull request
Aug 26, 2026
…CO report Housekeeping found while reviewing this module against the v19 valuation flow. None of it depends on that work. Dead v18 block removed: ``wizard/stock_valuation_layer_revaluation.py``, its view, ``wizard/__init__.py`` and ``views/stock_valuation_layer.xml``. All four hang from ``stock.valuation.layer``, a model Odoo removed in v19, and none was reachable — the ``wizard`` package was never imported and neither XML was declared in the manifest. That double disconnection is why the module still installed. The revaluation wizard was the v18 home of the secondary-currency journal entry, so leaving it in place made it read as current code instead of a leftover. ``_set_value`` filled a ``lots_to_recompute`` set on every lot-valuated incoming move whose only consumer was a commented-out line, so the work was done for nothing on every valuation. Bug — the AVCO audit report lost the core's UoM conversion. This module's ``stock_avco_report`` view is a copy of the standard one with the secondary-currency columns added, and the copy had dropped ``sm.quantity * (um.factor / up.factor)`` along with the two ``uom_uom`` joins it needs. So quantities were added up in each move's own unit instead of the product's reference one: a receipt of 1 dozen reported 1 instead of 12. And since the report derives the unit cost as ``total_value / total_quantity``, the AVCO came out twelve times too expensive. It only shows when the move's unit differs from the product's, which is why it went unnoticed. The view is now byte-identical to the standard one except for the two ``value_in_currency`` columns, and both copied methods carry a maintenance note saying they cannot call ``super()`` so drift is silent — the view builds fine and the numbers come out wrong. Docs and tests: FUNCIONALIDADES.md listed as "implemented but not active" four things of which two were the v18 code above and two are ACTIVE, so landed costs in secondary currency gets the section it never had. TESTING.md documented two test files that do not exist. And ``test_replenishment_cost_average_in_currency.py`` was on disk but imported by no one, so the only coverage of ``replenishment_cost_type = 'average_in_currency'`` never ran. Verified: 18 tests green on a fresh database, and reverting only the UoM fix makes the two new tests fail with 1.0 != 12.0. Part-of: #1004 Related: ingadhoc/account-financial-tools#984 Related: ingadhoc/product#937 Related: ingadhoc/miscellaneous#436 Signed-off-by: Camila Vives <cav@adhoc.inc>
roboadhoc
pushed a commit
to ingadhoc/stock
that referenced
this pull request
Aug 26, 2026
…cord company Contract of the seam ``account-multicompany-ux`` <-> ``stock-currency-valuation``: the valuation currency lives on the product CATEGORY and is company-dependent, so reading it off the ambient company answers for the wrong one as soon as the record belongs to another — routine with multi-company UX, where a user can have company A selected and operate on a record of company B. ``stock.quant._compute_secondary_value`` read the guard off the ambient company and the price with ``with_company(quant.company_id)``. When the active company has no valuation currency and the quant's does, the guard answers "none" —true for the active one— and the quant is skipped: its secondary value is not wrong, it is missing. The product is now resolved once with the quant's company and reused for both. ``stock.move._set_value`` had the same read on its conversion branch. The three violations this was expected to fix in ``product.product`` do not exist, and were checked one by one before touching anything: that model has no company of its own, so for its company-dependent fields the CONTEXT company is the semantic one. ``_get_last_product_value`` filters by ``self.env.company`` exactly as the core does, and ``write`` / ``_change_standard_price`` read in the very company they are writing to. ``stock.picking`` and ``stock.landed.cost`` already comply. Left out on purpose: ``valuation_currency_id`` is a ``related`` on ``stock.move``, ``stock.quant`` and the AVCO report, and a ``related`` always resolves in the ambient company with no way to apply ``with_company``, so two companies valuing the same category in different currencies can see a right amount under the wrong symbol. The fix is a compute with the record's company, which has consequences for views and searches. Verified in both directions: with the fix the suite is green, and without it the test fails 0.0 != 3500.0. The test uses a category whose valuation currency exists ONLY in company B and re-browses the quant from an env with company A — reading it on the ``with_company(company_b)`` recordset that created it proves nothing, since a company-dependent field resolves off the RECORDSET's context. Part-of: #1004 Related: ingadhoc/account-financial-tools#984 Related: ingadhoc/product#937 Related: ingadhoc/miscellaneous#436 Signed-off-by: Camila Vives <cav@adhoc.inc>
roboadhoc
pushed a commit
to ingadhoc/stock
that referenced
this pull request
Aug 26, 2026
…on flow Extends the valuation flow ``stock_account_ux`` introduces —value adjustments, manual valuation of selected moves, periodic closing— so a company valuing its stock in a second currency sees and books that valuation too. Declares the dependency on that module: it is ``auto_install`` so it was always there, but without the declaration Odoo guarantees no load order, and an override whose ``super()`` is not yet there fails. ``product.value`` gains ``previous_value_in_currency`` and ``delta_in_currency``, twins of the pair kept in company currency, with the same semantics: the previous value is captured when the adjustment is created and never recomputed, because by then the adjustment has already been applied to the move and to the product's cost. They show on the list, the search and the form next to their counterparts. Two bugs of this module surfaced on the way. The ``create`` filled a missing ``value`` with the product's ``standard_price``, which on an adjustment recorded ON A MOVE is wrong by a whole dimension —there ``value`` is the move's TOTAL— so adjusting only the secondary amount dropped a receipt of 4 units at 25 from 100 to 25. And the secondary amount never reached the move at all: nothing overrode the core's ``_get_manual_value``, so the "New Value in Currency" field of the "Adjust Valuation" dialog wrote a record no computation read. It now has its mirror, with priority over the AVCO and rate branches, returning ``None`` rather than ``0`` for "no adjustment" so an explicit correction to zero is honoured. With that in place an adjustment that moved only the secondary amount does change what the move is worth, so it counts as a revaluation and leaves the Stock Moves component — the criterion agreed for the valuation flow (task 64440, clarification Q2), reached by overriding ``_is_revaluation``. While the amount did not reach the move, that override would have reclassified something that moved nothing. Both valuation entries carry ``currency_id`` and ``amount_currency`` and add up to zero PER currency. The manual wizard accumulates the secondary balance under the same key as the company one and shows it on the draft, with the wizard-level total set only when every line shares a currency — a draft can gather products valued in different ones, and then a single total states nothing. The periodic closing takes the variation as inventory value minus what is booked, through the twins ``stock_value_in_currency`` and ``stock_accounting_value_in_currency``, and prorates it when the entry is split per product. Two limits, deliberate: an account gathering products valued in DIFFERENT secondary currencies is left in company currency, since a journal item carries one currency and picking one would be wrong; and the location-reclassification ``extra_balance`` is not netted from the secondary variation, having no twin in that currency. The second is an accounting call rather than a technical one. The column is not retroactive: entries posted before this carry no secondary amount and cannot be rebuilt. Verified: 49 tests green on a fresh database, each behaviour change checked in both directions. Four of these tests passed without proving anything until the reverted run exposed them — a move adjustment where the recomputed average happens to equal the manual value, a multi-company read taken on a recordset that still carried the other company, and a per-product split over a product with no cost, where there is nothing to prorate. Balance is asserted per currency: summing ``amount_currency`` across currencies comes out at zero even when one of them is wrong. Inert without a secondary currency, which is the majority scenario: ``stock_account_ux`` is ``auto_install`` and runs in plenty of databases where nobody values stock in a second currency. With no category using one, the two twins return nothing, the closing vals are handed back untouched, both entries stay whole in company currency with ``amount_currency`` equal to the balance, the draft offers no secondary total, and the ``_is_revaluation`` override answers exactly what the base criterion answers. Of those five tests only one is independently discriminating —breaking the twin makes it fail— and the other four are absence assertions: a product with no secondary currency has ``total_value_in_currency`` at zero, so even a broken twin leaves the balance at zero and the annotation never fires. Their value comes from being paired with the presence tests in the closing and wizard classes, which prove the machinery DOES fire when a currency is configured. Together they bracket the behaviour; on their own they do not. Worth recording, because it corrects what the code looks like it does: inertness is guaranteed by the twins discarding products with no currency, NOT by the currency filter in ``_get_valuation_currency_by_account``. Simplifying those twins in a later refactor on the assumption that the filter covers it would break the majority case silently. The limitations are documented in FUNCIONALIDADES.md, this module's equivalent of a README, including one the criterion did not ask for and this merge needs: the data is stored but the valuation report does not show the secondary currency yet. The fixture clears the valuation currency of every OTHER category. This module's own demo data puts one on a demo category, and that category shares the company's default valuation account with the fixture's one; an account gathering two secondary currencies is deliberately left in company currency, so on a database WITH demo data —the runbot build, among others— the closing carried no secondary amount and three tests measured nothing. Verified both ways on a fresh database with demo: 3 failed without this, 0 failed of 54 with it. A valuation account is only stated in a secondary currency when EVERY product on it is valued in that same one. Products with no secondary currency disqualify it just like two different currencies do, and that is the frequent case, not the exotic one: one category valued in dollars and the whole rest of the catalogue, with no currency, on the default valuation account. The closing line is split per product and the secondary amount was shared among all of them, so products valued in no second currency took a slice that was not theirs and the one that is kept a fraction of its own value — measured on a database with demo data, of 100 belonging to a single product that product kept 14,49 and the rest went to a dozen furniture products. Practical consequence, documented in FUNCIONALIDADES.md: a category valued in a secondary currency needs its own valuation account, which is what the test fixture now sets up. The shares are also rounded as a whole through the new ``_balance_valuation_extra_vals`` seam, with the leftover landing on the largest one. Rounding each share on its own left the entry off by a cent in the secondary currency —three products sharing 100 take 33,33 each— and it posted anyway, because in company currency it balances. closes #1004 Related: ingadhoc/account-financial-tools#984 Related: ingadhoc/product#937 Related: ingadhoc/miscellaneous#436 Signed-off-by: Camila Vives <cav@adhoc.inc>
roboadhoc
added a commit
that referenced
this pull request
Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
portal_backendgrantedbase.group_userto the parent menus without group from inside itsload_menusoverride. That method serves/web/webclient/load_menus, a route declaredreadonly=True, so the write hits a read-only cursor:The request survives (
http._serve_dbretries it with a read/write cursor), butodoo.sql_dbalready logged the failed query as an error, which is enough to turn a build red. On a real database it happens once, because the retry commits the group. On tests it happens on every tour that loads the webclient, since each test rolls that write back — so any module adding anHttpCasetour gets these errors in its log on a full OBA database that has a parent menu without group.What changes
The assignment moves to
_register_hook, which runs on a read/write cursor and is called every time the registry is loaded — which is exactly what installing or updating a module does. That keeps the case the original note cared about covered (another module installing a parent menu without internal group afterportal_backend), without writing from a web request.The
ormcacheon the override is dropped along with it: the coreload_menusis already cached, and the override existed only for the write.Also skips the write when there is nothing to fix, so the common case is a single search per registry load.
Test plan
On a 19.0 database with
portal_backendinstalled and three active parent menus stripped of their group (Calendar, WhatsApp, Dashboards), running the threestock_account_uxtours of ingadhoc/account-financial-tools#984:read-only transactionerrorsERRORlinesAfter the run the three menus have
base.group_useragain, written during the registry load.Branch name matches ingadhoc/account-financial-tools#984 so runbot builds both in a single bundle.