-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
13.0 mig web disable export group #1417
13.0 mig web disable export group #1417
Conversation
[IMP] Automatically add root and admin to group [FIX] Website in manifest
Currently translated at 100.0% (3 of 3 strings) Translation: web-12.0/web-12.0-web_disable_export_group Translate-URL: https://translation.odoo-community.org/projects/web-12-0/web-12-0-web_disable_export_group/zh_CN/
|
||
* David Vidal <david.vidal@tecnativa.com> | ||
|
||
* Ammar Officewala <https://twitter.com/AmmarOfficewala> |
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 must be your email address and not any other link.
Please fix the travis. |
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.
Hi and thanks for the port.
After a functional test it seems to be working great, I have left some comments as well.
Thanks
Sidebar.include({ | ||
_addItems: function (sectionCode, items) { | ||
var _items = items; | ||
if (!session.is_superuser && sectionCode === 'other' && items.length && !session.group_export_data) { |
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.
While the user will not be able to see the element, they will still be able to export using web/export/
and other endpoints. Is this something we accept?
|
||
ListController.include({ | ||
_updateButtons: function (mode) { | ||
if (this.$buttons) { |
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 sure about this total overwrite of the super function, what if we called super and then conditionally hide/show the element? Other people might override here and by not calling super we can break stuff.
tested functionally and works fine here. The user wont be able to click on Export anymore, since this option is not there anymore. |
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.
Functional test in EE13
Please rebase and fix Travis |
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.
See previous comment in april
Hi @ammarofficewla , |
Superseded by #1714 as no answer for contributor and Travis is red. |
No description provided.