-
-
Notifications
You must be signed in to change notification settings - Fork 48
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
[16.0][IMP] spreadsheet_dashboard_oca: Dashboard editability #31
[16.0][IMP] spreadsheet_dashboard_oca: Dashboard editability #31
Conversation
type="object" | ||
string="Copy" | ||
icon="fa-copy" | ||
groups="base.group_system" |
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.
Only setting users??
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 haven't changed this. Only being consistent with the other buttons. But yes, it seems a bit restricted. What do you think, @etobella ?
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.
It was done this way in order to set some groups, but we can create a new set if we think that it is the best option.
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.
only a minor comment
type="object" | ||
string="Copy" | ||
icon="fa-copy" | ||
groups="base.group_system" |
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.
It was done this way in order to set some groups, but we can create a new set if we think that it is the best option.
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.
a remark inline.
Otherwise, LGTM.
Thanks for catching that point.
@@ -30,3 +32,13 @@ def _inverse_spreadsheet_raw(self): | |||
record.data = base64.encodebytes( | |||
json.dumps(record.spreadsheet_raw).encode("UTF-8") | |||
) | |||
|
|||
def _compute_can_edit(self): | |||
"""We can edit if the record doesn't have XML-ID, or the XML-ID is noupdate=1""" |
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.
If we export a record (compatible for import) an xml id is generated.
With the current algorithm, if we export a record, it will become not editable. Don't you think ?
Quite theoritical use case though.
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.
Not really, as the XML-ID entry __export__.<whatever>
will be noupdate=1.
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.
Not really, as the XML-ID
entry __export__.<whatever>
will be noupdate=1.
I just tested in a 16.0 CE database.
- create a new product template, via UI
# select id, name, categ_id, type from product_template where id = 28;
id | name | categ_id | type
----+-----------------------------+----------+-------
28 | {"en_US": "my new product"} | 1 | consu
- export the product via UI, (checking compatible for import)
select id, module, name, model, res_id, noupdate from ir_model_data where res_id = 28 and model = 'product.template';
id | module | name | model | res_id | noupdate
-------+------------+------------------------------+------------------+--------+----------
15659 | __export__ | product_template_28_c3c1d954 | product.template | 28 | f
If I understand correctly, noupdate = False, so can_edit = False
did I missed something ? Sorry if my comment is not relevant.
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.
Ouch, I didn't expect that one... Anyway, fixed 😉
All the spreadsheet dashboards coming from Odoo in the modules spreadsheet_dashboard* are noupdate="0", which means that if you modify anything via this module, and then you update the module, all the changes will be lost. To avoid frustrations and to allow seamless updates, the following mechanisms have been put in place: - Spreadsheet dashboards now have an active field. - Only the manually created dashboards or those coming from data with noupdate="1" will be editable. - There's a mechanism for copying existing dashboards. So, for modifying one of the standard dashboards, the steps will be: - Duplicate it through the "Copy" button. - Disable the standard one. - Edit the copy. TT49379
5e7293a
to
5b9316b
Compare
/ocabot merge patch |
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at 2ba9692. Thanks a lot for contributing to OCA. ❤️ |
All the spreadsheet dashboards coming from Odoo in the modules spreadsheet_dashboard* are noupdate="0", which means that if you modify anything via this module, and then you update the module, all the changes will be lost.
To avoid frustrations and to allow seamless updates, the following mechanisms have been put in place:
So, for modifying one of the standard dashboards, the steps will be:
@Tecnativa TT49379