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

[17.0][FIX] hr_employee_second_lastname: removed useless mapped address_home_id #1370

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

Conversation

DorianMAG
Copy link
Contributor

@DorianMAG DorianMAG commented Jun 19, 2024

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 @luisg123v,
some modules you are maintaining are being modified, check this out!

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.

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
Copy link
Contributor Author

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
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

@DorianMAG DorianMAG requested a review from luisg123v June 20, 2024 08:40
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

@flotho
Copy link
Member

flotho commented Jul 1, 2024

Hi @luisg123v , any chance to have a new review here ?

Copy link
Member

@flotho flotho left a comment

Choose a reason for hiding this comment

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

easy code review,
tested on production
LGTM

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.

Hi,

The commit message doesn't conform guidelines yet.

Current commit message is:

[17.0][FIX] hr_employee_firstname: removal of the mapped method which became useless with the address_home_id field, this no longer exists in v17 and replaced by a non-relational field (field.char) named private_street

It needs the following changes to conform guidelines:

  • Remove version
  • It should be a short title, followed by a blank line, followed by a message
  • Lines should be wrapped to 72 characters

@@ -75,7 +75,6 @@ def _prepare_vals_on_write_firstname_lastname(self, vals):
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 address_id instead?

CC @xmglord

Copy link

Choose a reason for hiding this comment

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

No it should be replaced with work_contact_id since address_home_id was split in multiple fields. I took this 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.

Thank you for your feedback, I will fix this quickly.
Regards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@DorianMAG DorianMAG force-pushed the 17.0-fix-hr_employee_second_lastname branch from 93429c5 to 34e8799 Compare August 26, 2024 08:53
@DorianMAG DorianMAG force-pushed the 17.0-fix-hr_employee_second_lastname branch from 34e8799 to 921afce Compare August 26, 2024 09:15
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.

  • Missing commit description.
  • Please rebase to see if CI is fixed.

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.

7 participants