[18.0][MIG] document_page_portal - #615
Conversation
OCA-git-bot
added
series:18.0
mod:document_page_portal
|
@marcelsavegnago — thanks in advance for reviewing 🙏 A heads-up on the red These are unreaped Chrome child processes in the CI container during HttpCase cleanup ( Is this a known issue with browser tours in the current image (possibly Chrome-version related, cf. OCA/oca-ci#122)? Happy to adjust the tests if you'd prefer a different approach (e.g. HTTP-level checks instead of full browser tours). Thanks! |
|
@kaynnan @pedrobaeza this should run clean now on a rebase / rerun ? |
|
@dnplkndll From what I see, yes, the rebase would already solve the tests that failed |
caa396d to
832a07b
Compare
|
@dnplkndll @kaynnan thanks for taking a look 🙏 Rebased onto the latest |
|
ping @pedrobaeza |
|
/ocabot migration document_page_portal |
|
@kwoychesko Can you improve the testing percentage? |
Cover the portal controller branches the browser tours do not exercise: the access-denied / missing-record redirect in document_pages_followup, and the date_begin/date_end filter of the document list. These use url_open (no browser), so coverage rises without adding Chrome tours.
|
@kaynnan Added HTTP-level controller tests ( |
|
@kwoychesko thanks for your efforts on this.
2. Since v17 all sections have icons. I'd recommend you use the same icon from the homescreen of document_knowledge? IMO it looks odd not to have one for Knowledge.
https://github.com/OCA/knowledge/blob/18.0/document_knowledge/static/description/icon.svg
|
tonygalmiche
left a comment
tonygalmiche
left a comment
There was a problem hiding this comment.
Functional review
Tested on a local Odoo 18.0 instance:
- Public pages correctly appear on the portal ("Knowledge Documents"), and private pages correctly follow the follower-based visibility.
- Category badges display correctly.
Found and confirmed 2 bugs in controllers/portal.py:
- Search bar doesn't filter anything — typing any search term still returns all pages. Root cause:
search_domainstarts as[](matches everything) and gets OR-ed with the real condition, which always matches everything regardless of the search term:
search_domain = []
if search_in in ("content", "all"):
search_domain = OR(
[
search_domain,
["|", ("name", "ilike", search), ("content", "ilike", search)],
]
)
domain += search_domain
- The search bar label displays raw HTML as literal text instead of rendering it, because the string is not marked safe:
"label": _('Search (in Content)'),
Confirmed fix for both (tested locally, works correctly):
from markupsafe import Markup
...
searchbar_inputs = {
"content": {
"input": "content",
"label": Markup(_('Search (in Content)')),
},
"all": {"input": "all", "label": _("Search in All")},
}
...
search
if search and search_in in ("content", "all"):
domain += ["|", ("name", "ilike", search), ("content", "ilike", search)]
Request changes with the fix above.
`OR([[], <leaves>])` evaluates to `[(1, '=', 1)]` on 18.0, so the search term
was silently discarded and the portal list came back unfiltered.
`normalize_domain([])` returns `[TRUE_LEAF]`, and `[TRUE_LEAF]` is the
absorbing element for `OR`, so `expression.combine()` returns it before the
`name`/`content` leaves are ever appended (odoo/osv/expression.py:222-224,
290-296, 308-310). The resulting domain was
`[('type', '=', 'content'), (1, '=', 1)]`.
The idiom was correct up to 16.0, where `combine()` skipped falsy operands
before normalizing them. Odoo 450f5c9307a changed that and rewrote its own
copy of this pattern in hr_timesheet's portal controller; this module carried
the pre-fix shape forward. Build the domain directly instead.
Also add `search` and `search_in` to the pager `url_args`: page links are
built from `url_args` alone (addons/portal/controllers/portal.py:41-46), so
without them page 2 of a filtered list silently reverts to the full list.
The existing search tour cannot catch this - it asserts only that the matching
page is present, and its fixture holds a single record, so the rendered list is
identical whether the domain filters or not. Add a browser-free controller test
with a second, non-matching page and an `assertNotIn`.
The "(in Content)" searchbar entry rendered as literal text - users saw `Search <span class="nolabel"> (in Content)</span>` in the dropdown, and the same string in the search box placeholder. `_()` with no format arguments returns a plain `str` (odoo/tools/translate.py:483-484), and `portal.portal_searchbar` renders the label with `t-out`, which compiles to `yield str(escape(content))` (odoo/addons/base/models/ir_qweb.py:2079). Only `Markup` survives `escape()`. The placeholder follows from the same cause: `_adaptSearchLabel` (addons/portal/static/src/js/portal.js:136-139) clones the label, removes `span.nolabel` and uses the remaining text. With the markup escaped there is no element to remove, so the raw string lands in the placeholder. The module was correct under 14.0, where that template still used `t-raw`. Odoo c7002c7be7e converted it to `t-out` and wrapped its own copy of this string in `Markup()` at the same time; migrating 14.0 -> 18.0 skips the release where that adaptation was owed.
Every other section of the portal home carries a pictogram since 17.0; the Knowledge Documents card had neither icon nor caption, so it read as unfinished next to the core cards. `portal.portal_docs_entry` accepts both (addons/portal/views/portal_templates.xml:256-266), and the pictogram layout that un-hides the icon is active by default (:273-277). Ship the artwork inside this module rather than pointing at `document_knowledge/static/description/icon.svg`. `o_portal_icon` has no CSS rule anywhere in 18.0 - it appears only twice, both in portal_templates.xml - so the `<img>` is sized purely by the SVG's intrinsic dimensions. Core pictograms declare them (addons/sale/static/src/img/bag.svg is `<svg width="64" height="64" ...>`), while the app-store icon carries only a `viewBox` and would render at the browser default of roughly 150px square. The copy adds the two attributes and drops the Inkscape bookkeeping. Reported by @chrisandrewmann.
I asked Claude to provide a long-form response in case you were interested (ignore at your leisure):
One precision on the effect: it doesn't match every record outright — the module's ir.rule is still ANDed in, so the user gets back every content page they were already entitled to read. A filter no-op rather than an ACL bypass. Worth noting for the changelog: the same upstream commit rewrote addons/hr_timesheet/controllers/portal.py::_get_search_domain away from this exact idiom. This controller is a copy of Odoo's pre-fix pattern — the block is byte-identical to 14.0 — so the framework moved underneath it. I've kept it out of the [MIG] commit and put it in a separate [FIX]. Applied as you proposed, dropping OR entirely (and the now-unused import). Addition your patch doesn't cover: portal_pager was called with url_args={"date_begin", "date_end", "sortby"}, and page links are built from url_args alone (addons/portal/controllers/portal.py:41-46). With search fixed, clicking page 2 of a filtered list would drop the term and revert to the full list, while the pager's own page count still reflected the filtered total. Core includes both keys (addons/project/controllers/portal.py:474), so I've added search and search_in in the same commit.
Applied as you wrote it. I deliberately did not switch to the 18.0 core form (_("Search%(left)s …", left=Markup(…), …), addons/project/controllers/portal.py:349-354). It's better hygiene, but it changes the msgid and orphans the Italian translation at i18n/it.po:86-87, whose msgstr already carries correct markup and renders correctly the moment the Markup() wrap lands. Happy to switch if you'd rather have the hardening. Regression guard. The existing search tour couldn't have caught this — it asserts only that the matching page is present, and its fixture holds a single record, so the rendered list is identical whether the domain filters or not. Added test_06_document_list_search_filter to the browser-free url_open class, with a second non-matching page and an assertNotIn. Verified it fails on the old code and passes on the new. Separately, I noticed two pre-existing issues that aren't migration fallout — the followup route errors instead of redirecting when given a bogus token, and a category page renders its children's titles without re-checking access. Both behave identically on 14.0, so they don't belong in a MIG PR; I'll open a separate [FIX] once this lands and the module exists on the 18.0 branch. |
@chrisandrewmann Both points confirmed and fixed. Point 1 turned out to be two unrelated bugs that happened to land on the same widget. Again, here is the long-form response for those interested, otherwise skip: The Search <span class in the box is a rendering bug, not a search bug. The label is a plain translated str containing markup, and portal.portal_searchbar renders it with t-out, which HTML-escapes anything that isn't Markup (ir_qweb.py:2079). The placeholder is derived from that label by _adaptSearchLabel (addons/portal/static/src/js/portal.js:136-139), which strips span.nolabel before using the text — with the markup escaped there's no element to strip, so the whole raw string ends up in the box. Fixed by wrapping the label in markupsafe.Markup. The term being ignored is unrelated: expression.OR uses [TRUE_LEAF] as its absorbing element and normalize_domain([]) returns [TRUE_LEAF], so OR([[], ]) collapses to [(1,'=',1)] and the term is discarded (odoo/osv/expression.py:222-224, 290-296, 308-310). The domain is now built directly, with a regression test that uses a non-matching fixture. I also added the search to the pager links, otherwise page 2 would have dropped it. Point 2, the icon. Agreed — portal.portal_docs_entry takes both icon and text (addons/portal/views/portal_templates.xml:256-266) and this card set neither. Both added. One caveat on the icon source, since it's a trap worth recording: o_portal_icon has no CSS rule anywhere in Odoo 18 CE — it appears exactly twice, both in portal_templates.xml (:256, :274). The So it's the same artwork, shipped inside this module at static/src/img/knowledge.svg with width="64" height="64" added. That matches the core convention of keeping portal pictograms under static/src/img/, and avoids an asset path into a module this one doesn't declare in depends. |
Thanks, looks good to me. |
9 participants
Migration of
document_page_portalto 18.0. Part of #499.Changes during migration (beyond the mechanical pass)
<data>node;t-raw→t-out; Bootstrap 4 → 5 classes.views/assets.xmlto the manifestassetskey.web_tour.toursregistry API; the Python tests now usestart_tour.knowledge.group_document_user→document_knowledge.group_document_user.placeholder_countpattern (it was rendering hidden).portal.portal_record_layoutwith an inline card./web#…to/odoo/document.page/<id>.A separate
[IMP]commit improves the README (clearer DESCRIPTION/USAGE, fixed CREDITS link).Tested locally on 18.0: the module installs cleanly and both portal tours pass in a headless browser.