[MIG] Migrate module attachment_base_synchronize to v10#763
Conversation
florian-dacosta
force-pushed
the
10-mig-attachment_base_synchronize
branch
from
950915e to
eddca74
Compare
lasley
left a comment
lasley
left a comment
There was a problem hiding this comment.
Minor comments, otherwise LGTM
| @@ -1,5 +1,5 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <openerp> | |||
| <odoo> | |||
There was a problem hiding this comment.
odoo noupdate="1" & can remove data tag
| @@ -1,5 +1,5 @@ | |||
| <?xml version="1.0"?> | |||
| <openerp> | |||
| <odoo> | |||
There was a problem hiding this comment.
odoo noupdate="1" & can remove data tag
florian-dacosta
force-pushed
the
10-mig-attachment_base_synchronize
branch
from
eddca74 to
090b3e7
Compare
|
Thanks for the review @lasley |
| if domain is None: | ||
| domain = [] | ||
| domain.append(('state', '=', 'pending')) | ||
| domain = [('state', '=', 'pending')] |
There was a problem hiding this comment.
Did you really want to change the behaviour here ?
The ('state', '=', 'pending') tuple is now only added in the domain when the domain was None, instead of being always added like before.
A unit test calling this method with a domain in arguments would be fine.
There was a problem hiding this comment.
That is wanted indeed.
The normal way is to just run the attachment at pending state, so the cron does not need any argument. But one can make a cron only with attachment at 'failed' state for instance.
So the idea is that we can ignore the generic domain ('state', '=', 'pending') for more flexibility.
There was a problem hiding this comment.
I agree with @florian-dacosta on this one. An alternative to changing the behavior is to use a context override to stop the hard coded domain, but IMO explicitly adding is better than explicitly removing.
|
When will this be accepted? |
|
This will be accepted when there are enough approves. |
| ===== | ||
|
|
||
| Go the menu Settings > Attachments | ||
| Go the menu Settings > Technical > Database Structure > Meta Data Attachments |
| id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink | ||
| access_attachment_metadata_user,ir.attachment.metadata.user,model_ir_attachment_metadata,base.group_user,1,0,0,0 | ||
| access_attachment_metadata_user,ir.attachment.metadata.user,model_ir_attachment_metadata,,1,0,0,0 | ||
| access_attachment_metadata_user,ir.attachment.metadata.user,model_ir_attachment_metadata,base.group_no_one,1,1,1,1 |
There was a problem hiding this comment.
They have same key, I've fixed that in #856 because it needed to pass tests.
There was a problem hiding this comment.
What is the change you are recommending that @florian-dacosta perform here?
Edit: (sorry for the accidental ping @sylvain-garancher)
There was a problem hiding this comment.
@lasley There is a mistake because 2 security rules have the same id. @cmsalmeida fixed it in the PR of external_file_location migration. I guess it is enought, as for now external_file_location is the only module depending on this one.
There was a problem hiding this comment.
Thanks for the elaboration @florian-dacosta. Works for me too!
There was a problem hiding this comment.
It's running well.
Tested together with external_file_localtion in #856
👍
|
Took the same path like @cmsalmeida 👍 |
|
@sylvain-garancher - have your comments been attended to? |
Syncing from upstream OCA/server-tools (9.0)
5 participants
The tests fail because of : #762
If I play only the test of this module, it is all ok, but if I play base_kanban_stage tests together, it fails.
Indeed, because of base_kanban_stage's tests, the cursor is already in test_mode.