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

[MIG] hr_employee_firstname: Migrated to 10.0 #276

Merged
merged 10 commits into from
Jan 24, 2017
Merged

[MIG] hr_employee_firstname: Migrated to 10.0 #276

merged 10 commits into from
Jan 24, 2017

Conversation

espo-tony
Copy link
Contributor

No description provided.

Copy link

@LeartS LeartS left a comment

Choose a reason for hiding this comment

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

There is a superflous indentation level in the xml file left after the removal of the <data> tag, but ok for me.

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.

  • Module code look good
  • Module install correctly on 10 database and do the job.

super(HrEmployee, self)._auto_init(cr, context=context)
self._update_employee_names(cr, SUPERUSER_ID, context=context)
@api.model_cr_context
def _auto_init(self):
Copy link
Member

Choose a reason for hiding this comment

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

This should be done in a post_init_hook

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 totally agree.
I didn't move this functionality into a post_init_hook because in the previous versions of this module _auto_init is still used. As this is just a PR for migration I thought better to keep the continuity with the previous versions. I think that changing this should be rather done in a separate pool request.
What's your opinion about this?

Copy link
Member

Choose a reason for hiding this comment

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

I think this should be done right here. Migration means sometimes a refactorization, and something that is not correct at all (this is not multi-DB aware) should be cut from the beginning.

@@ -1,23 +1 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Don't remove encoding

@@ -4,7 +4,7 @@
#
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 file, as it's not needed and confuses when outdated.

@@ -1 +1,22 @@
# -*- coding: utf-8 -*-
##############################################################################
Copy link
Member

Choose a reason for hiding this comment

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

@njeudy
Copy link

njeudy commented Nov 21, 2016

@pedrobaeza is there a way to commit on the same merge request ? or @espo-tony should accept me in espo-tony:10_porting_hr_employee_firstname ?

@LeartS
Copy link

LeartS commented Nov 21, 2016

You can do a PR against the branch of this PR in @espo-tony's repo, when he merges it this PR will automatically update

@espo-tony
Copy link
Contributor Author

@pedrobaeza
In my last commit I've implemented the post_init_hook as requested.
For the runbot failure do you have any clue on what could be the problem? It doesn't seem to depend on my commit, nor on Odoo at all.

@pedrobaeza
Copy link
Member

Yeah, runbot is failing due to the change in the infrastructure to Docker. It seems there's still something to fine-tune, @moylop260 and @gurneyalex...

@gurneyalex
Copy link
Member

No idea... I just triggered a rebuild to see

@pedrobaeza
Copy link
Member

It's also happening on l10n-spain: runbot.odoo-community.org/runbot/build/3274108

@gurneyalex
Copy link
Member

could be related to OCA/runbot-addons#111

Copy link

@leemannd leemannd left a comment

Choose a reason for hiding this comment

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

One dependencie change & a question about the version

# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html).

from openerp import SUPERUSER_ID
from openerp.api import Environment

Choose a reason for hiding this comment

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

s/openerp/odoo/

<attribute name="invisible">1</attribute>
<attribute name="no_label">1</attribute>
<attribute name="required">0</attribute>
</xpath>

Choose a reason for hiding this comment

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

IMHO I find this way to do to be more readable . Just a question, is there any reason to have taken V8 of the module instead of the V9 ?

Copy link
Member

Choose a reason for hiding this comment

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

It's better to not make replace for avoiding losing other attributes (for example, a custom context), so I prefer this one.

@leemannd
Copy link

@pedrobaeza Post init_hook have been updated. Is it possible to have a review? Thank you.
@espo-tony Thanks for the port.

@pedrobaeza
Copy link
Member

Please shorten headers as stated and I'll merge.

@espo-tony
Copy link
Contributor Author

@leemannd Thank you very much for your contribution.

@pedrobaeza pedrobaeza merged commit d087cc8 into OCA:10.0 Jan 24, 2017
@espo-tony espo-tony deleted the 10_porting_hr_employee_firstname branch January 24, 2017 13:27
leemannd pushed a commit to leemannd/hr that referenced this pull request Jun 12, 2017
feketemihai pushed a commit to feketemihai/hr that referenced this pull request Nov 14, 2017
feketemihai pushed a commit to feketemihai/hr that referenced this pull request Nov 14, 2017
feketemihai pushed a commit to feketemihai/hr that referenced this pull request Nov 16, 2017
sambarros pushed a commit to sambarros/hr that referenced this pull request Jul 26, 2018
i-vyshnevska pushed a commit to i-vyshnevska/hr that referenced this pull request Mar 5, 2019
i-vyshnevska pushed a commit to i-vyshnevska/hr that referenced this pull request Mar 12, 2019
quentingigon pushed a commit to quentingigon/hr that referenced this pull request Oct 11, 2019
luistorresm pushed a commit to vauxoo-dev/hr that referenced this pull request Oct 12, 2020
komsan-S pushed a commit to ecosoft-odoo/hr that referenced this pull request Nov 10, 2020
luistorresm pushed a commit to vauxoo-dev/hr that referenced this pull request Oct 18, 2021
luistorresm pushed a commit to vauxoo-dev/hr that referenced this pull request Oct 19, 2021
luistorresm pushed a commit to vauxoo-dev/hr that referenced this pull request Nov 5, 2022
andreagidaltig pushed a commit to vauxoo-dev/hr that referenced this pull request Nov 6, 2023
andreagidaltig pushed a commit to vauxoo-dev/hr that referenced this pull request Nov 8, 2023
luisg123v pushed a commit to Vauxoo/hr that referenced this pull request Nov 14, 2023
andreagidaltig pushed a commit to vauxoo-dev/hr that referenced this pull request Nov 16, 2023
andreagidaltig pushed a commit to vauxoo-dev/hr that referenced this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants