-
-
Notifications
You must be signed in to change notification settings - Fork 171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[17.0][MIG] DMS : Migration to 17.0 #323
Conversation
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? |
Yes, they are both under |
Great, then other contributors may improve it in the future. For now seems enough. |
Please check red CIs. |
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). |
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. |
Yes, it seems good. Don't underestimate your design skills 😉 About the copyright in headers, you should only add it on the files you touch. |
In the directory model, the 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. |
About the copy, there are 2 aspects:
|
Well, I set |
Am I allowed to change the |
Same for ruff error : |
I would say that the official specification for v17 is still ECMA 2016. @ged-odoo can you please tell us? |
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you unify all change commits into 1 (v17 migration) for easier review?
Please, cherry-pick #327 to commit history before migration v17 commit. |
3bbb33e
to
4cf0da4
Compare
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job, some things:
-
It would be missing the pre-commit commit before the v17 migration commit ([IMP] dms: pre-commit auto fixes) https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-17.0 I think that would help to "reduce" the changes in the migration commit.
-
Can you indicate all the changes you have done in the migration commit?
-
On the other hand, other improvements (adding the
has_all_access
field fromdms.access.group
oris_first_creation
field in dms.directory) should go in another commit [IMP] after the migration commit OR do it in another PR after merging this (to make it easier to review this PR).
Please, cherry-pick #318 to commit history (before migration v17 commit). |
e839217
to
cb30f22
Compare
Done. Also, I edited the comment of this PR to list what's still to be done, and what's already done. |
Okay, I fixed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code and functional review OK.
Ping @CarlosRoca13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code and functional review 👍 Thanks 😄
This PR has the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have started to review the migration commit, but it's barely impossible due to all the "style changes". Please refrain from doing them on the migration commit. You may heard that squashing commits should be done, but not at this level. The commit should be the unit to ease the review, so you must squash when you do a progressive migration refinement through several commits, not when you are doing a total move of every line of code for placing according a specific order. I agree that is good to have the code sorted according certain guidelines, but the first rule is to minimize the diff on any existing code. Please check the rest of my inline comments where I expand these thoughts.
dms/models/dms_directory.py
Outdated
auto_join=False, | ||
copy=False, | ||
copy=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is changing the behavior. AFAIK, the problem here is that we can't assure the security consistency on the copied files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? Everything should be copied just fine. The copy method in dms_file.py was not doing much than what Odoo now already does.
domain="[('permission_create', '=', True)]", | ||
context="{'dms_directory_show_path': True}", | ||
context={"dms_directory_show_path": True}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to change this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To change what? If you compare with like the v16 or v15 I only removed the outer quotes according to the official doc for the context param to be a dict and not a string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link doesn't work to the documentation you mention doesn't work. Can you correct it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah you're right, sorry. Here is the new new link. If you search for other places where they use the context in fields, you'll see that they don't use quotes round the dict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the link. Well, the option to provide strings is also supported, and it's useful when you need to put a dynamic expression depending on a field value for example, but in this case, it's the same. It was only for reducing the diff of the migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean that I can leave as is?
e33eb16
to
80d8b67
Compare
@pedrobaeza it should be all good now. I think I've reversed all the reordering I did, and you can know more easily do the code review. |
f428007
to
cfe838f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the efforts and your patience. I think we are almost there. Just some final comments, and I don't nickpit more.
domain="[('permission_create', '=', True)]", | ||
context="{'dms_directory_show_path': True}", | ||
context={"dms_directory_show_path": True}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link doesn't work to the documentation you mention doesn't work. Can you correct it?
cfe838f
to
0ffaf96
Compare
@pedrobaeza I fixed what you asked, and sent the right url this time (I hope). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, there are still some stylistic things (like empty lines inside methods, as I didn't mention each of them), but let's merge it for not disturbing you more. Thanks for the work.
/ocabot merge nobump
What a great day to merge this nice PR. Let's do it! |
Congratulations, your PR was merged at 6dbce74. Thanks a lot for contributing to OCA. ❤️ |
Migration of the DMS module to v17.
Code review is in progress, so changes might still occur.
Closes #269
Done:
mail.assets_messaging
onboarding.onboarding.step
andonboarding.onboarding
has been added. Corresponding data has been added in thedata
folder. Onboarding actions per model has been added. Theaction_close
has been added to theonboarding.onboarding
model as well as different open actions inonboarding.onboarding.step
. Also updatedres_company
to reflect those changesportal_my_dms_directory
andportal_my_dms
controller methodsdirectoy
andaccess_group
filename to fit the model nameunlink
method ofBase
model causing infinite loop. See the comment for more infocopy
method of access group, as well as the ones indirectory
andfile
in order to display (copy) or (1) when duplicated_name_get
being replace by_compute_display_name
,attrs="{'invisible...
replaced byinvisible="...
_compute_permissions
removed theif self.env.su
since odoo already does it in the_filter_access_rules
called a few line belowsecurity.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.useFileViewer
hookres_settings
view according to the Migration guide v16->v17