Skip to content
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

[PORT] disable_odoo_online. [REF] remove obsolete code. OCA convention. #583

Merged
merged 3 commits into from
Oct 23, 2016

Conversation

legalsylvain
Copy link
Contributor

  • PORT disable_odoo_online
  • remove some obsolete code part.

Thanks for your reviews.

cc @StefanRijnhart, @hbrunn.

@legalsylvain legalsylvain added this to the 10.0 milestone Oct 22, 2016
@pedrobaeza pedrobaeza mentioned this pull request Oct 22, 2016
63 tasks
Images
------

* Odoo Community Association: `Icon <https://github.com/OCA/maintainer-tools/blob/master/template/module/static/description/icon.svg>`_.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you dont need the OCA logo, no need to provide the credits

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

_inherit = 'publisher_warranty.contract'

@api.multi
def update_notification(self, cron_mode=True, context=None):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asking myself whether this is legal for people using EE.
For them it is not allowed to temper with the notification system.
This makes this module IMO not usable for them while the other features could make sense.
Would it be an option to split this function out of the module in a separate one?

Copy link
Member

@yelizariev yelizariev Oct 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As suggested by Odoo staff, we made similar feature in web_debranding optional, but don't allow to switch it off (i.e. we always send request) for enterprise users.

Code to check whether it enterprise:

from odoo.release import version_info
is_enterprise = version_info[5] == 'e'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems fair to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the test.

# Copyright (C) 2013 Therp BV (<http://therp.nl>).
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html).

from openerp import api, models
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from odoo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@alexis-via
Copy link
Contributor

disable_odoo_online used to remove user menu entries (top-right drop-down menu) such as "My odoo.com account"... but you dropped that part, so after installation of the module on v10, the menu entry "My odoo.com account" is still present. I would prefer to keep that part and remove such user menu entries.

_inherit = 'publisher_warranty.contract'

@api.multi
def update_notification(self, cron_mode=True, context=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost seems like Odoo SA is shooting themselves in the foot by using a method available in Community in order to phone home in Enterprise. Solution seems sound though.

Also, think we can drop the context arg?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes wirh api.multi decorator we have access to context from self.env.context

@legalsylvain
Copy link
Contributor Author

@alexis-via : Thanks ! I didn't understand this part of code. I guessed it was obsolete. thanks for pointing this problem. Fixed.
@ALL thanks for your reviews. fixes and improvement done.

Copy link
Contributor

@alexis-via alexis-via left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review and test. Thanks for the work !

@elicoidal
Copy link

👍 Thanks for the rework!

@lasley lasley merged commit dff8628 into OCA:10.0 Oct 23, 2016
hieulucky111 pushed a commit to hieulucky111/server-tools that referenced this pull request Nov 6, 2017
…n. (OCA#583)

* [PORT] disable_odoo_online. [REF] remove obsolete code. OCA convention.

* [FIX] restauring old feature

* [IMP] do not disable some feature for Enterprise Edition. [REF] some improvements
jeanpaulrobineau pushed a commit to apertoso/server-tools that referenced this pull request Mar 12, 2018
…n. (OCA#583)

* [PORT] disable_odoo_online. [REF] remove obsolete code. OCA convention.

* [FIX] restauring old feature

* [IMP] do not disable some feature for Enterprise Edition. [REF] some improvements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants