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

[12.0][MIG] hr_family #487

Closed
wants to merge 9 commits into from

Conversation

umiphos
Copy link

@umiphos umiphos commented Oct 22, 2018

This is a migration for hr_family.

This migration was done as it was in 11.0, no big changes, just the manifest

Copy link
Contributor

@luisg123v luisg123v left a comment

Choose a reason for hiding this comment

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

Remember to extract commits from the module in v11, so historic is not lost and your changes are visible

For more info, see: https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-12.0#user-content-technical-method-to-migrate-a-module-from-110-to-120-branch

miketelahun and others added 8 commits October 22, 2018 15:58
* Move out of '__unported__'
* Move files in 'models' and 'views' subdirectories
* Migrate/fix views
* Migrate to the new API
* Module installable
* README added
* Move each model to a separate file
* Clean up data models
* README updated
* Use of CamelCase for data model classes + replace 'dob' terms with 'date_of_birth'
* Add security group on family's page
* Add read access for employee to allow to open employee form view without exception
* Add possibility to define gender on children
* Use editable bottom
* Use the same selection like employee
* Remove useless form view
OCA Transbot updated translations from Transifex

OCA Transbot updated translations from Transifex

OCA Transbot updated translations from Transifex

OCA Transbot updated translations from Transifex

OCA Transbot updated translations from Transifex

OCA Transbot updated translations from Transifex

OCA Transbot updated translations from Transifex

OCA Transbot updated translations from Transifex

OCA Transbot updated translations from Transifex
@umiphos umiphos force-pushed the 12.0-hr_family-migration-umiphos branch from 9659145 to b4557e5 Compare October 22, 2018 16:10
@umiphos
Copy link
Author

umiphos commented Oct 22, 2018

Trying to install it as a dummy, this error comes by

bad query: b" INSERT INTO ir_translation(name, lang, res_id, src, type, value, module, state, comments)\n                           SELECT name, lang, res_id, src, type, value, module, state, comments\n                           FROM tmp_ir_translation_import\n                           WHERE type = 'model'\n                           ON CONFLICT (type, lang, name, res_id) WHERE type = 'model'\n                            DO UPDATE SET (name, lang, res_id, src, type, value, module, state, comments) = (EXCLUDED.name, EXCLUDED.lang, EXCLUDED.res_id, EXCLUDED.src, EXCLUDED.type, EXCLUDED.value, EXCLUDED.module, EXCLUDED.state, EXCLUDED.comments);\n                       "
ERROR: ON CONFLICT DO UPDATE command cannot affect row a second time
HINT:  Ensure that no rows proposed for insertion within the same command have duplicate constrained values.

But I found that this error has been reported previously odoo/odoo#27992

@oca-clabot
Copy link

Hey @umiphos, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/cla
Here is a list of the users:

  • @umiphos (login unknown in OCA database)

Appreciation of efforts,
OCA CLAbot

@umiphos umiphos force-pushed the 12.0-hr_family-migration-umiphos branch from 6358a23 to b209f2a Compare October 22, 2018 20:23
@pedrobaeza pedrobaeza added this to the 12.0 milestone Oct 23, 2018
@OCA-git-bot OCA-git-bot mentioned this pull request Oct 23, 2018
32 tasks
Copy link
Member

@tarteo tarteo left a comment

Choose a reason for hiding this comment

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

Code looks good.

hr_family/README.rst Show resolved Hide resolved
Employee Family Information
===========================

This module allows you to enter extra information about employee's family.
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on the use cases of this module? I don't see a reason why somebody should use this module.

Copy link
Contributor

Choose a reason for hiding this comment

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

I may jump in here: some employees get presents for their children's birthdays. Yet I'm not sure if this module should be as-is, since there may be different types of relations and probably it would be better to refactor this module to have family_member model with relation_type, and rest of fields.

@pedrobaeza what would be a procedure if there's a different way for getting similar features?

Copy link
Member

Choose a reason for hiding this comment

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

Well, the idea is to get the best solution for a problem. Sometimes the answer is not trivial, and depends on use cases, implementation budget, etc.

For this case, indeed it's a very specific case that was implemented quickly, but it's very uncommon. If you ask me, I would go for a generalization using the same scheme as partner_multi_relation, defining relations and linking to partners, but this time having one of the sides of the relation being the employee, and then you can make use of all the possible interesting information in the partner.

Copy link
Contributor

@alexey-pelykh alexey-pelykh Oct 24, 2018

Choose a reason for hiding this comment

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

Maybe to proceed in-between, since employee family being in contacts/partners seems overhead? Usually it's name + phone + DOB + gender w/o extra information?

Copy link
Author

Choose a reason for hiding this comment

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

the effort to make a more generic hr_family is in here #489 thanks @alexey-pelykh, we can continue this conversation over there

Copy link
Contributor

@alexey-pelykh alexey-pelykh Nov 1, 2018

Choose a reason for hiding this comment

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

I'd keep migration separate from refactor, plus approaches differ a bit

Copy link
Author

Choose a reason for hiding this comment

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

I agree with you, this should keep as a migration, and #489 should be an improvement, I deleted everything from this change that is not part of the migration

alexey-pelykh
alexey-pelykh previously approved these changes Oct 24, 2018
@oca-clabot
Copy link

Hey @umiphos,
We acknowledge that the following users have signed our Contributor License Agreement:

Appreciation of efforts,
OCA CLAbot

@alexey-pelykh
Copy link
Contributor

FYI: #489

@umiphos
Copy link
Author

umiphos commented Oct 25, 2018

@pedrobaeza in the recent add to #489 this PR should be closed, right?

@alexey-pelykh
Copy link
Contributor

IMHO, it would be great to just port hr_family to 12 w/o that particular enhancement, it order for end-users to select what they are used to

@umiphos
Copy link
Author

umiphos commented Oct 25, 2018

@alexey-pelykh thanks for the answer, let's wait to see opinions, also, I make a similar change than #489, but I will remove that change if this module should stay as a migration.

@alexey-pelykh
Copy link
Contributor

For sure! And sorry for jumping in :)

@umiphos umiphos force-pushed the 12.0-hr_family-migration-umiphos branch from 03ab331 to b209f2a Compare November 1, 2018 16:01
@umiphos umiphos changed the title [MIG] hr_family [12.0][MIG] hr_family Nov 1, 2018
@BT-mgomez
Copy link

@gurneyalex @alexey-pelykh

Can this be merged to 12.0 ? it was approved long ago.

@BT-mgomez
Copy link

/ocabot merge

@OCA-git-bot
Copy link
Contributor

Sorry @BT-mgomez you do not have push permission, so I can't merge for you.

@alexey-pelykh
Copy link
Contributor

@BT-mgomez not enough approves. My opinions stands: though this PR is a good migration, maybe we still should consider writing migration script to hr_employee_relative

@jarroyomorales
Copy link

I also think it is better just to write a migration script to 12.0 and stop this module in 11.0 since it doesnt provide any extra information comparing it to the relatives module.

@alexey-pelykh alexey-pelykh self-requested a review August 14, 2019 07:57
@alexey-pelykh alexey-pelykh dismissed their stale review August 14, 2019 09:20

I think it would be better to have a migration script indeed

@umiphos
Copy link
Author

umiphos commented Aug 14, 2019

I agree you all to keep this in 11.0 and not migrate it, also, there was another module that has more information and a better structure, @alexey-pelykh

  • can I close this PR?
  • to whom we have to report this?
  • what's a migration script?

thanks in advance

@alexey-pelykh
Copy link
Contributor

@umiphos I think it's safe to keep this PR but rewrite the migration to a module that does the migration and installs the other module as dependency. right, @pedrobaeza ?

for migrations, see https://github.com/OCA/maintainer-tools/tree/master/template/module/migrations/8.0.1.0.0

@jarroyomorales
Copy link

Should we close this then?

@alexey-pelykh
Copy link
Contributor

Guess so! @umiphos as far as I've understood, you're ok with closing this PR, but if I've understood your poorly, please let me know

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.