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

11.0 mig disable odoo online #1

Merged
merged 3 commits into from Mar 23, 2018

Conversation

hieulucky111
Copy link
Contributor

No description provided.

<odoo>

<!-- Menus moved under Tools\Parameters -->
<record model="ir.ui.menu" id="base.module_mi">
Copy link
Member

Choose a reason for hiding this comment

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

This is something unrelated to the module.

* update notifier code is deactivated and the function is overwritten
* apps and updates menu items in settings are hidden inside Technical \\Parameters

Installation
Copy link
Member

Choose a reason for hiding this comment

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

Remove this section

<odoo>

<record id="mail.ir_cron_module_update_notification" model="ir.cron">
<field name="active" eval="False" />
Copy link
Member

Choose a reason for hiding this comment

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

What this does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to deactivate function update_notification in module mail. Function update_notification used to "Send a message to Odoo's publisher warranty server to check the validity of the contracts, get notifications, etc...".

Copy link

Choose a reason for hiding this comment

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

Hmmmm I thought we had to leave this active, and instead disabled it in PublisherWarrantyContract.update_notification. Am I remembering incorrectly @legalsylvain?

Copy link

@lasley lasley left a comment

Choose a reason for hiding this comment

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

Thanks @hieulucky111 - few minor comments inline

:target: https://www.gnu.org/licenses/agpl
:alt: License: AGPL-3

==================
Copy link

Choose a reason for hiding this comment

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

Please extend the = to the end of the header

===========

Bugs are tracked on `GitHub Issues
<https://github.com/OCA/server-tools/issues>`_. In case of trouble, please
Copy link

Choose a reason for hiding this comment

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

Update URI

<odoo>

<record id="mail.ir_cron_module_update_notification" model="ir.cron">
<field name="active" eval="False" />
Copy link

Choose a reason for hiding this comment

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

Hmmmm I thought we had to leave this active, and instead disabled it in PublisherWarrantyContract.update_notification. Am I remembering incorrectly @legalsylvain?


@api.multi
def update_notification(self, cron_mode=True):
if version_info[5] == 'e':
Copy link

Choose a reason for hiding this comment

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

PublisherWarrantyContract

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain more?

Copy link

Choose a reason for hiding this comment

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

I honestly don't remember. Let's just assume I was being dumb!

Copy link

@lasley lasley left a comment

Choose a reason for hiding this comment

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

Thanks @hieulucky111

@lasley lasley added this to the 11.0 milestone Jan 17, 2018
Copy link
Member

@astirpe astirpe left a comment

Choose a reason for hiding this comment

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

We already use this module in production and it works fine!
I just have one question, for the rest this module is ok.

<odoo>

<record id="mail.ir_cron_module_update_notification" model="ir.cron">
<field name="active" eval="True" />
Copy link
Member

Choose a reason for hiding this comment

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

OK let's say (as @lasley said) that active should be True. But then my question is: why do we keep this line? What about removing the entire ir_cron.xml file?

Copy link

Choose a reason for hiding this comment

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

Looks to me like this logic was here before we realized that we were not allowed to disable the update notification for EE. Tracing it into the code, this cron does call update_notification.

In this light, I think we've broken contract for anyone that has this installed on EE

Copy link
Member

@astirpe astirpe Jan 17, 2018

Choose a reason for hiding this comment

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

Example: if someone decides to disable manually this cron for some reason, we are forcing it to set to active=True again as soon as he installs this module (or makes an update all, for example). There is not even set odoo noupdate="1" in the actual solution. It's still not clear to me, sorry.

Copy link

Choose a reason for hiding this comment

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

@hbrunn - you wrote this one, do you remember?

Copy link
Member

Choose a reason for hiding this comment

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

my version of this was disabling it: https://github.com/OCA/server-tools/blob/10.0/disable_odoo_online/data/ir_cron.xml#L5 - so yes, whoever noticed this shouldn't be done any more should have just ditched the file. check the commit history. speaking of which, you shouldn't approve PRs which don't take the commit history with them, especially for cases like the current one, it's utterly annoying having to puzzle together the evolution of a module/file if the link is cut

Copy link
Member

Choose a reason for hiding this comment

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

Clear, thank you!
@hieulucky111 can you please remove the ir_cron.xml file?

@pedrobaeza
Copy link
Member

@hieulucky111 can you redo commit history following migration guide?

@astirpe
Copy link
Member

astirpe commented Feb 19, 2018

@pedrobaeza is it ok to merge this one?

"license": "AGPL-3",
"category": "base",
"depends": [
'base',
Copy link
Member

Choose a reason for hiding this comment

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

No need to depend on base if you depend on other things.

Copy link

@njeudy njeudy left a comment

Choose a reason for hiding this comment

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

Code check and test

tungocbui and others added 3 commits March 23, 2018 18:51
…. Update the version in the file __openerp__.py
…n. (#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
@pedrobaeza pedrobaeza merged commit 809b8df into OCA:11.0 Mar 23, 2018
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

9 participants