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.

Sign up for GitHub

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

Jump to bottom

[17.0][FIX] hr_employee_firstname: removed useless mapped address_home_id #1369

Open
wants to merge 1 commit into
base: 17.0
Choose a base branch
from
Open

[17.0][FIX] hr_employee_firstname: removed useless mapped address_home_id #1369

wants to merge 1 commit into from

Conversation

DorianMAG
Copy link
Contributor

@DorianMAG DorianMAG commented Jun 19, 2024 •
edited
Loading

Remove useless mapped.
The source code has changed in OCB/OdooSA.
The relational field no longer exists on the address in V17.
https://github.com/OCA/OCB/blob/3afef8250c829406a8fd227a6bd9f71c4f80ff29/addons/hr/models/hr_employee.py#L49

@OCA-git-bot
Copy link
Contributor

Hi @Savoir-faire Linux, @luisg123v,
some modules you are maintaining are being modified, check this out!

luisg123v
luisg123v suggested changes Jun 19, 2024
View reviewed changes
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.

Same as in #1370

Regarding commit message:

  • Title doesn't conform guidelines
  • Missing commit description

Besides, regarding PR description:

  • Not clear enough, why the OCB should affect? This should work with mnative Odoo
  • Link is not a permalink

@DorianMAG DorianMAG force-pushed the 17.0-fix-hr_employee_firstname branch from 4a6a5a6 to 529904e Compare June 20, 2024 08:30
@DorianMAG
Copy link
Contributor Author

Same as in #1370

Regarding commit message:

* Title doesn't conform guidelines

* Missing commit description

Besides, regarding PR description:

* Not clear enough, why the OCB should affect? This should work with mnative Odoo

* Link is not a permalink

Hi @luisg123v
Thank you for your review.
The commit description is corrected.
I updated my message regarding the OCB/OdooSA source code.
Updating the link with a permalink.

Regards

@DorianMAG DorianMAG requested a review from luisg123v June 20, 2024 08:33
@DorianMAG
Copy link
Contributor Author

If the partner_firstname module is installed,
this makes it impossible to install this module,
because the address_home_id field no longer exists in v17,
and it is replaced by a non-relational field which therefore no longer returns
a recordset partner

JulienMartinez
JulienMartinez approved these changes Jun 20, 2024
View reviewed changes
Copy link

@JulienMartinez JulienMartinez left a comment

Choose a reason for hiding this comment

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

LGTM

@mahmud-ecoservice
Copy link

Hi @luisg123v ,
I have the same issue. The address_home_id field is not exists in v17 and causing problems with the module installation.

@DorianMAG DorianMAG force-pushed the 17.0-fix-hr_employee_firstname branch from 882a0b4 to 529904e Compare August 8, 2024 07:50
luisg123v
luisg123v suggested changes Aug 21, 2024
View reviewed changes
hr_employee_firstname/models/hr_employee.py
@@ -168,7 +168,6 @@ def _install_employee_firstname(self):
def _update_partner_firstname(self):
for employee in self:
partners = employee.mapped("user_id.partner_id")
partners |= employee.mapped("address_home_id")
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be replaced by work_contact_id? Or is it expected for it to be removed?

@luisg123v
Copy link
Contributor

Hi @DorianMAG,

I still see the old commit message. Could you check again, please?

Regards,

@DorianMAG
Copy link
Contributor Author

Hi @DorianMAG,

I still see the old commit message. Could you check again, please?

Regards,

Hello, yes, thank you, I'll take a look at it quickly.

@DorianMAG DorianMAG force-pushed the 17.0-fix-hr_employee_firstname branch from 529904e to d9a012f Compare August 26, 2024 09:10
@DorianMAG
Copy link
Contributor Author

@luisg123v
It's done, but the test are now failing. Should I do something else?

@DorianMAG DorianMAG requested a review from luisg123v August 30, 2024 08:25
luisg123v
luisg123v suggested changes Oct 31, 2024
View reviewed changes
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.

  • Commit message is not correct yet, it now seems the combination of two different commits.
  • Please rebase to see if CI is fixed.

@DorianMAG DorianMAG force-pushed the 17.0-fix-hr_employee_firstname branch from d9a012f to 5e7f5b8 Compare November 22, 2024 15:57
@DorianMAG
Copy link
Contributor Author

  • Commit message is not correct yet, it now seems the combination of two different commits.

    • Please rebase to see if CI is fixed.

Done
regards

@DorianMAG DorianMAG requested a review from luisg123v November 22, 2024 15:59
@DorianMAG DorianMAG force-pushed the 17.0-fix-hr_employee_firstname branch from 5e7f5b8 to b435982 Compare December 11, 2024 09:20
@DorianMAG @DoDoViVi
[FIX] hr_employee_firstname: replace address_home_id field by work_co…
cab3b44 
…ntact_id for v17

this no longer exists in v17 and replaced by a non-relational field (field.char) named private_street
@DorianMAG DorianMAG force-pushed the 17.0-fix-hr_employee_firstname branch from b435982 to cab3b44 Compare December 11, 2024 09:39
cvinh
cvinh approved these changes Dec 12, 2024
View reviewed changes
Copy link

@cvinh cvinh left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JulienMartinez JulienMartinez JulienMartinez approved these changes

@gaelTorrecillas gaelTorrecillas gaelTorrecillas approved these changes

@cvinh cvinh cvinh approved these changes

@dreispt dreispt dreispt approved these changes

@luisg123v luisg123v Awaiting requested review from luisg123v

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@DorianMAG @OCA-git-bot @mahmud-ecoservice @luisg123v @dreispt @cvinh @gaelTorrecillas @JulienMartinez