[19.0][MIG] dms: Migration to 19.0 - #475
Conversation
|
Thanks for the contribution. Please preserve commit history following technical method explained in https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-19.0. If the jump is between several versions, you have to modify the source branch in the main command to accommodate it to this circumstance. |
43752f4 to
660b3c9
Compare
|
Thanks for the review @pedrobaeza — sorry I missed the technical method on the first round. I've now:
CI is re-running now. |
4affd39 to
724ff8b
Compare
|
Friendly ping @pedrobaeza @victoralmau @eLBati @etobella |
etobella
left a comment
There was a problem hiding this comment.
Using AI for coding is fine (I personally think it is a nice tool to improve your results), but you need to understand the code you're submitting.
In this PR specifically:
- Some functionalities were removed
- A lot of unnecessary diff was introduced
These are basic mistakes that would be caught by simply reviewing the changes your AI assistant supplied before opening a PR.
That is also a license risk: AI tools are trained on code with various licenses (GPL, MIT, Apache…). If you can't read the output, you can't detect contamination, missing attribution, or patent-encumbered patterns. Evenmore, you cannot claim ownership and copyright (completely necessary in an OpenSource community). "The AI wrote it" is not a legal defense.
Review what you ship, always.
24e9535 to
7128196
Compare
Follow-up to OCA#475 phase 1 (the minimal-viable 19.0 [MIG]). This PR layers the OWL/UX modernisations that were held back to keep the base MIG reviewer-friendly. Opens on the ledoent fork against the phase 1 branch so reviewers can preview the delta on top of OCA#475 before it lands. - **Kanban buttons**: drop the classic `dms.KanbanButtons` template (mobile-Scan + desktop-Upload + hidden file input) and rewire `buttonTemplate` to the modern `dms.FileKanbanView.Buttons` (single Upload button, `t-ref="uploadFileInput"` hook pattern, modern `onFileInputChange` handler). Mobile-Scan UX is intentionally dropped — the file drop-zone (already wired via `createFileDropZoneExtension()`) covers the mobile-upload flow. - **Breadcrumb modernisation**: replace `path_owl.xml`'s inline-style `<a oe_form_uri>` + `<span style="display: inline">` pattern with Odoo 19's `<ol class="o_breadcrumb breadcrumb"><li class="breadcrumb-item">` Bootstrap idiom. Final segment uses `class="breadcrumb-item active"` with `aria-current="page"` per Bootstrap a11y guidance. - **Restore `filter_domain` on `dms_category.xml`** search view — `filter_domain="['|', ('name', 'ilike', self), ('parent_id', 'child_of', raw_value)]"` was removed in OCA#475 and re-introduces the 18.0 UX of filtering by parent-category subtree when typing a parent's name. The bare 19.0 default `ilike` on name lost that behaviour. (Tag search left as default; 18.0's `filter_domain` was equivalent to 19.0's default for that field.) Deliberately held for follow-up rounds (not in this PR): - **JS dead-code audit** of `attachment_image.esm.js` / `attachment_viewer_viewable.esm.js`: grep across odoo/odoo:19.0 + OCA org confirmed only our own files reference the patches, and core 19.0 `LinkPreview` has no native `imageUrl` getter — verification inconclusive on whether the patches are still load-bearing. Keeping both files until a runtime regression test or a maintainer's explicit "core handles it now" signal. - **Coverage closure**: add `coverage.py` integration + tests for the new 19.0 code paths (`_search()` override on `dms_security_mixin`; `_search_starred` operator normalisation in `directory`). Separable. - **Code-style sweep** (whitespace + indentation): not bundled here to keep the diff focused on user-visible UX changes. Signed-off-by: Daniel Kendall <dkendall@ledoweb.com>
|
Thanks for the partial response, but several inline comments were left unanswered... More importantly, your own Phase 2 PR restores things that were removed in this PR (kanban buttons, breadcrumb, filter_domain). That is the clearest possible confirmation that functionality was silently dropped and went unnoticed — exactly the problem I feared in the review. Please address all inline comments before requesting another review, and make sure this PR is complete and self-consistent before opening follow-ups to fix what it broke. Vibecoding without knowing what you are doing is a problem for the code quality, for licensing, and for the reviewers who have to clean up after it. So, can you justify all the actions done? |
|
@etobella certainly, thanks for the review! Apologies for the AI-review failure on the first round — I do try to keep it reined in. Your feedback is the best teacher; sometimes between all the notes and skill updates I wonder, but I believe next year the careful conventions will pay back in speed without losing quality. this PR feedback is delays as I fix my internal runboat. for the reasoning on the button change I wanted to get the phase 2 IMP up. will try and get that later today. Force-pushed
Final scope: 31 files / +190-122 (down from 36 / +430-431, ~64% churn reduction). All 7 CI checks green. A follow-up PR is opened as a draft preview on the ledoent fork: ledoent#2 — base = this PR's branch, so the diff visible there is only the phase-2 delta. Contents:
Deferred to a later round in that PR: JS dead-code audit (verification was inconclusive; core 19.0 Will promote ledoent#2 to an OCA upstream draft PR once this one lands |
Follow-up to OCA#475 phase 1 (the minimal-viable 19.0 [MIG]). Opens on the ledoent fork against the phase 1 branch so reviewers can preview the delta on top of OCA#475 before it lands. - **Breadcrumb modernisation**: replace `path_owl.xml`'s inline-style `<a oe_form_uri>` + `<span style="display: inline">` pattern with Odoo 19's `<ol class="o_breadcrumb breadcrumb"><li class="breadcrumb-item">` Bootstrap idiom. Final segment uses `class="breadcrumb-item active"` with `aria-current="page"` per Bootstrap a11y guidance. - **Restore `filter_domain` on `dms_category.xml`** search view — `filter_domain="['|', ('name', 'ilike', self), ('parent_id', 'child_of', raw_value)]"` was removed in OCA#475 and re-introduces the 18.0 UX of filtering by parent category subtree when typing a parent's name. The bare 19.0 default `ilike` on name lost that behaviour. (Tag search left as default — 18.0's filter was equivalent to 19.0's default for that field.) Originally this PR also rewired the kanban `buttonTemplate` from `dms.KanbanButtons` to the orphan `dms.FileKanbanView.Buttons` template, but that template uses invalid OWL inheritance syntax (`<div role="toolbar" position="inside">` instead of `<xpath ...>`) AND references controller hooks (`uploadFileInputRef`, `onFileInputChange`) that don't exist on the controller. Reverted; phase 1's classic `dms.KanbanButtons` template stays in place (it's now self-contained without `t-inherit` since 19.0's `web.KanbanView.Buttons` is empty). Deferred to a later round: JS dead-code audit (inconclusive — core 19.0 `LinkPreview` has no native `imageUrl` getter, so our patches may still be load-bearing), coverage closure, and a proper kanban UX modernisation once `dms.FileKanbanView.Buttons` is fixed or replaced. Signed-off-by: Daniel Kendall <dkendall@ledoweb.com>
|
Even this response seems AI-generated. Please, stop doing this, it is only making things worse @dnplkndll We don't have yet an AI Policy, but I hope we will have one soon. meanwhile, you can see one example of what should be and shouldn't be allowed to be done with AI by checking other community policies... |
|
@etobella looking over the policy that matched up with the pycore team podcast from maintainers I just mentioned to @JordiBForgeFlow . I get it. the social part of this think is important and maintainers are not here to fix slop. what I actually do is cli 6 sessions and always try my best to review and update. clarify fix everything I do. I understand I own it and I do understand the value of being the human. but I make mistakes. miss things, like I have in the past in many contributions I made before llms. but I can assure you I am present trying to be an enguaged policy following participant in the community. I continue to have much more grandiuos goals than my time and budget and am included to learn and grow with all the feedback I can get. I will comment on your inlines and to be honest that last one I was like wth, but I did not then have time / remember to open that file to verify. what is hard for me at this point is going through. then working on something else and a change happend in the wrong window I miss. I get it this is your time and will try to do better at making sure nothing hits the upstream repo intill I am ready to own it. |
|
Please, cherry-pick #492 to commit history before pre-commit commit. |
When an incoming email is processed for a dms.directory alias configured
with "Unpack Emails as: Subdirectory" (alias_process='directory'),
message_new() builds the new subdirectory name from the email subject by
calling ir.http._slug() on a plain string. _slug() expects a record or a
(id, name) tuple, so it raises:
ValueError: too many values to unpack (expected 2)
and the incoming email cannot be ingested. The intent is simply to
slugify the subject string, which is what ir.http._slugify() does.
Use it instead.
|
I prepared a focused follow-up on top of the current PR head: ledoent#29 It fixes Odoo 19 contextual directory/file defaults and computed search Domains, and adds regressions for read-only access, restricted direct binary URLs, and real upload/download. Validation passed: all pre-commit hooks, fresh install, 68 test methods / 82 checks with 0 failures/errors, and module upgrade. |
|
@Erp4759 I see the issue on the commit. seems like a lot of lines for the fix. ledoent#30 suggesting this but also was looking for a more experienced look from someone link @victoralmau to confirm. |
XtremXpert
left a comment
There was a problem hiding this comment.
Functional review — tested this PR locally on a fresh Odoo 19.0 (Community) database:
odoo-bin -d <fresh_db> -i dms --test-tags=/dms --stop-after-init→ 0 failed, 0 error(s) of 65 tests- Module installs cleanly (no demo data required), registry loads without module-related warnings.
Environment: Odoo 19.0 Community, Python 3.12, PostgreSQL local.
Thanks for the migration — we plan to use dms 19.0 for Quebec non-profit deployments and are happy to test follow-ups.
🤖 Generated with Claude Code
victoralmau
left a comment
There was a problem hiding this comment.
I don't understand the point of 171bd86.
In my opinion, it shouldn't be necessary, right?
agreed. it was just an attempt to adapt to the most concise modern access methods. unneeded in this mig. |
|
Please, cherry-pick #503 to commit history before migration commit. |
| ) | ||
| # search | ||
| if search and search_in == "name": | ||
| domain = Domain.AND([domain, Domain.OR([[], [("name", "ilike", search)]])]) |
There was a problem hiding this comment.
| domain = Domain.AND([domain, Domain.OR([[], [("name", "ilike", search)]])]) | |
| domain += Domain.OR([[], [("name", "ilike", search)]]) |
There was a problem hiding this comment.
@victoralmau I applied this but was looking at core, eq https://github.com/odoo/odoo/blob/57ffa1abb6e1ed481b8bccc5db5534ea0ad2c6b5/addons/hr_timesheet/controllers/portal.py#L113 and you can see the explaination on the last comit. I will drop it if you do not agree but debugging the non matching term did seem to prove it out.
| def _process_attachments_for_post(self, attachments, attachment_ids, message_data): | ||
| """Indicate to DMS that we're attaching a message to a record.""" | ||
| _self = self.with_context(attaching_to_record=True) | ||
| return super(MailThread, _self)._process_attachments_for_post( | ||
| attachments, attachment_ids, message_data | ||
| ) |
There was a problem hiding this comment.
| def _process_attachments_for_post(self, attachments, attachment_ids, message_data): | |
| """Indicate to DMS that we're attaching a message to a record.""" | |
| _self = self.with_context(attaching_to_record=True) | |
| return super(MailThread, _self)._process_attachments_for_post( | |
| attachments, attachment_ids, message_data | |
| ) | |
| def _process_attachments_for_post(self, attachments, attachment_ids, message_values): | |
| """Indicate to DMS that we're attaching a message to a record.""" | |
| self = self.with_context(attaching_to_record=True) | |
| return super()._process_attachments_for_post( | |
| attachments, attachment_ids, message_values | |
| ) |
| """ | ||
| Open the form to create a new storage from the onboarding panel. | ||
| """ | ||
| return self.env.ref("dms.action_dms_storage_new").read()[0] |
There was a problem hiding this comment.
| return self.env.ref("dms.action_dms_storage_new").read()[0] | |
| return self.env["ir.actions.actions"]._for_xml_id( | |
| "dms.action_dms_storage_new" | |
| ) |
| Open the form to create a new directory from the onboarding panel. | ||
| """ | ||
| storage = self.env["dms.storage"].search([], order="create_date desc", limit=1) | ||
| action = self.env.ref("dms.action_dms_directory_new").read()[0] |
There was a problem hiding this comment.
| action = self.env.ref("dms.action_dms_directory_new").read()[0] | |
| action = self.env["ir.actions.actions"]._for_xml_id( | |
| "dms.action_dms_directory_new" | |
| ) |
| directory = self.env["dms.directory"].search( | ||
| [], order="create_date desc", limit=1 | ||
| ) | ||
| action = self.env.ref("dms.action_dms_file_new").read()[0] |
There was a problem hiding this comment.
| action = self.env.ref("dms.action_dms_file_new").read()[0] | |
| action self.env["ir.actions.actions"]._for_xml_id( | |
| "dms.action_dms_file_new" | |
| ) |
There was a problem hiding this comment.
probably you mean
action = self.env["ir.actions.actions"]._for_xml_id(
"dms.action_dms_file_new"
)
I see you miss "="
| def _find_record_check_access(self, record, access_token, field): | ||
| if record._name in ("dms.file", "dms.directory"): | ||
| if record.sudo().check_access_token(access_token): | ||
| # sudo because the user might not usually have access to the record but | ||
| # now the token is valid. | ||
| # Used to display the icon in the portal. | ||
| return record.sudo() | ||
|
|
||
| return super()._find_record_check_access(record, access_token, field) |
There was a problem hiding this comment.
This method no longer exists in v19; you'll likely need to override the _can_return_content() method in the dms.file and dms.directory models to achieve similar behavior.
There was a problem hiding this comment.
you are correct, replaced with_can_return_content()
Assisted-by: Claude Fable 5
Assisted-by: Claude Fable 5
OR([[], ('name', 'ilike', search) leaf]) contains the empty domain, which
normalizes to TRUE — the absorbing element of OR — so the search term was
discarded: 18.0 expression.combine returns its zero, 19.0 Domain.OR
optimizes to [(1, '=', 1)]. Verified live in both forms; a search for a
non-existent term returned every directory.
The empty seed came from the core portal multi-field accumulator idiom
(38d8463, 2020), where core appends per-field domains to a plain list and
ORs once at the end; inlined to the current shape by the 17.0 migration.
Assisted-by: Claude Fable 5
98f4088 to
4fb9072
Compare
Port of
dmsfrom 18.0 to 19.0 (migration guide).Non-mechanical adaptations worth flagging
osv.expression→fields.Domainthroughout the security mixin, andread_group→_read_group(keywordaggregates=, tuple unpacking insteadof dicts).
_search_permission_*methods and the computed-permission
ir.rules are kept as-is. An earlierrevision of this PR carried an extra
[IMP]that moved this to_search/_check_accessoverrides; that commit has been dropped —@victoralmau was right that it was not necessary for the migration, and the
same access tests pass without it. It now lives on a fork branch for
separate consideration after this lands.
with_demo=False,so fixtures are built in
setUpClasswithnew_test_user()andcreate()rather than skipping when a demo xmlid is absent — a skip would have meant
those tests silently never ran in CI.
AI-assisted (Claude Code); every change reviewed, tested, and owned by the author.