[17.0][MIG] DMS : Migration to 17.0

[tva-subteno-it]

Migration of the DMS module to v17.

Currently a WIP. Code review is in progress, si changes might still occur.

TO DO:

• [ ] Translations
• [ ] Correct code according to the code review
• [ ] Add documentation, docstrings, comments
• [ ] Refactor code, re-order fields by alphabetical order for more readability, follow the contributing guide for file naming. Follow the attribute model order stated here. Done : abstract_dms_mixin, base, dms_access_group, dms_directory
• [ ] Fixing tests

Done:

• [x] Reorganized manifest into security, actions, templates, data, views and wizard. Same for the assets, organized by styles, JS and XML
• [x] Removed inexistant assets mail.assets_messaging
• [x] Changed the onbaording flow, for since v17, new models onboarding.onboarding.step and onboarding.onboarding has been added. Corresponding data has been added in the data folder. Onboarding actions per model has been added. The action_close has been added to the onboarding.onboarding model as well as different open actions in onboarding.onboarding.step. Also updated res_company to reflect those changes
• [x] Refactoring of portal_my_dms_directory and portal_my_dms controller methods
• [x] Rename of the directoy and access_group filename to fit the model name
• [x] Fix context parsing in fields, by changing the context from a string to a dictionnary (cf this line)
• [x] Fixed unlink method of Base model causing infinite loop. See the comment for more info
• [x] Fixed copy method of access group, as well as the ones in directory and file in order to display (copy) or (1) when duplicated
• [x] Fixed _name_get being replace by _compute_display_name, attrs="{'invisible... replaced by invisible="...
• [x] In _compute_permissions removed the if self.env.su since odoo already does it in the _filter_access_rules called a few line below
• [x] Changed security.xml to better handle what is the user is allowed to see/write/create/unlink by calling _get_allowed_directory_ids doing the job of checking what the user can do according to its access groups.
• [x] Created two new icons to fit the new design of Odoo V17
• [x] Removed useless Javascript file (not in the manifest anymore) as well as restructured the files into a more consistant structure
• [x] Changed Javascript files displaying file's preview to use the new approach, with the useFileViewer hook
• [x] Fixed the patch of multiple class using the solution in the Odoo documentation
• [x] Fix of the kanban view for directory and files to the new architecture. This led to some refactoring in the SCSS, to remove useless class names, and clean up some duplicated code between file/directory styles sheets
• [x] Changed the res_settings view according to the Migration guide v16->v17

[pedrobaeza]

Let's put here further conversation about this migration instead on the general migration issue.

That icon may serve. Do you have it on SVG?

[tva-subteno-it]

Yes, they are both under static/description.

[pedrobaeza]

Great, then other contributors may improve it in the future. For now seems enough.

[pedrobaeza]

Please check red CIs.

[tva-subteno-it]

Thanks for taking the time of doing a little code review, but I don't have finished the migration yet. Things might change over time (I'm not quite used of contributing to projects, so I don't know if I'm doing things right). About the CIs, I think I will correct them all at once once the migration is finished, as well as documenting the code, translations etc.

[tva-subteno-it]

What should I do about Copyrights ? I'm migrating this module because a customer needs it. So at my job we copyright every file with this :

Copyright 2024 Subteno (https://www.subteno.com).
License LGPL-3.0 or later (https://www.gnu.org/licenses/lgpl).

which matches the current license used. Should I add the Copyright 2024 Subteno (https://www.subteno.com). line on every file ? And the whole thing if the file doesn't already have one ?

[tva-subteno-it]

I added another icon for the portal. Similarly to the previous one, I edited it in order to fit the others portal's icon.

It was a bit hard to find the correct one, and edited it order to stay "minimalist" and follow the color scheme of the portal, but it doesn't look too odd I think. Let me know your thoughts.

[pedrobaeza]

Yes, it seems good. Don't underestimate your design skills :wink:

About the copyright in headers, you should only add it on the files you touch.

[tva-subteno-it]

In the directory model, the file_ids field's copy attribute is set to False, but in the copy method of the directory, the files are manually duplicated :

def copy(self, default=None):
    # ...
    for record in self.file_ids:
        record.copy({"directory_id": new.id})

Should the files be duplicated with the directory or not ? I think it can be quite dangerous if you want to duplicate a folder, but you forgot there are many files, or big files in it.

[pedrobaeza]

About the copy, there are 2 aspects:

• Functional / business logic one: When you copy a folder in a file explorer, the contents are copied, so I think it's OK to copy contents as well when duplicating. If the storage is attachment, no real space duplication will happen.
• Implementation aspect: not sure why it's copy=False and done manually. Maybe when this was done (v12), copy didn't work correctly, or it was a thing of permissions due to the overrides there. As there are no code comments for knowing the reasons, we can only speculate. You can try switching to copy=True to see if you find any issue.

[tva-subteno-it]

Well, I set copy=True, as well as removing the manual copy, and it worked juste fine. There was the same process for child_directories_ids, so I did the same and it worked. Maybe indeed it was some leftovers from the V12

[tva-subteno-it]

Am I allowed to change the ecmaVersion in .eslintrc.yml ? I would like to change it to 2020, because when running pre-commit, I'm getting an error on props.record.data?.path_json, simply because it doesn't recognize ?.. (even though it works).

[tva-subteno-it]

Same for ruff error : B023 Function definition does not bind loop variable inside the lambda of a filtered. I could just ignore this specific error, because these errors are juste in the case of .filtered, which is unavoidable. It would be much more difficult and less readable trying to remove this error, knowing that it's done on purpose.

[pedrobaeza]

Am I allowed to change the ecmaVersion in .eslintrc.yml ?

I would say that the official specification for v17 is still ECMA 2016. @ged-odoo can you please tell us?

[pedrobaeza]

B023 Function definition does not bind loop variable

This linter in fact is correct. We are not doing something totally correct, and should be changed. How it should be done? From the existing code:

Before:

tags = record.tag_ids.filtered(lambda rec: not rec.category_id or rec.category_id == record.category_id)

After:

tags = record.tag_ids.filtered(lambda rec, record=record: not rec.category_id or rec.category_id == record.category_id)

[victoralmau]

Please, cherry-pick https://github.com/OCA/dms/pull/327 to commit history before migration v17 commit.

[tva-subteno-it]

@victoralmau Everything should be done. I rebased all my commits and cherry-picked the last change. I'm currently working on the tests. Let me know if you have any question/suggestion, or anything else.

[victoralmau]

Please, cherry-pick https://github.com/OCA/dms/pull/318 to commit history (before migration v17 commit).

[tva-subteno-it]

Please, cherry-pick #318 to commit history (before migration v17 commit).

Done. Also, I edited the comment of this PR to list what's still to be done, and what's already done.

[victoralmau]

Please, cherry-pick #318 to commit history (before migration v17 commit).

Done. Also, I edited the comment of this PR to list what's still to be done, and what's already done.

Ok, all my comments are pending.

[tva-subteno-it]

Please, cherry-pick #318 to commit history (before migration v17 commit).

Done. Also, I edited the comment of this PR to list what's still to be done, and what's already done.

Ok, all my comments are pending.

Hi, I've just returned from vacation. I've already replied to some of your comments, but I'm working on the rest.

[victoralmau]

Please, cherry-pick https://github.com/OCA/dms/pull/332 to commit history (before migration v17 commit).

[victoralmau]

Please, cherry-pick https://github.com/OCA/dms/pull/334 to commit history (before migration v17 commit).

[tva-subteno-it]

I've corrected the unit tests. I also removed the code I added about the has_all_access and is_first_creation. I managed to find more Odoo vanilla approach. But I have a question about the translations. When will the module be translated ? After this PR is merged ? For the moment the only translations are the ones coming from the v16.

[tva-subteno-it]

For the moment, I edited the fr.po file myself since I'm french. It shouldn't cause any problem with Weblate (I think) because the project is currently not available for this version on Weblate.

[pedrobaeza]

OK, no problem, as indeed there won't be conflict. Thanks for the migration work.

/ocabot migration dms

[tva-subteno-it]

Hi @victoralmau, I resolved all the comments. I also fixed a bug raised in the issue of this PR. About the perm_read field, you will see that I finally found a way to remove it. You left a comment about the indentation in the menu.xml file. The problem is that I'm not sure what you meant, so I changed it a bit, but pre-commit edit this file anyway, so I didn't manage to remove all the indents.

Feel free to check again, and tell me if you see anything left to improve/fix.

[victoralmau]

Please, cherry-pick https://github.com/OCA/dms/pull/338 to commit history (before migration v17 commit).

[tva-subteno-it]

Please, cherry-pick #338 to commit history (before migration v17 commit).

Done

[tva-subteno-it]

How does the process works now? Will someone check the code, or something like that? When do you think this can be merged?