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

[16.0][ADD] partner_address_ok #1782

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

Conversation

norlinhenrik
Copy link

No description provided.

@norlinhenrik norlinhenrik changed the title [ADD] partner_address_ok [16.0][ADD] partner_address_ok Jun 16, 2024
Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Hi @norlinhenrik. What is an adress "OK" ?

@norlinhenrik norlinhenrik force-pushed the 16.0-add-partner_address_ok branch 2 times, most recently from 265129f to 58f05f7 Compare June 16, 2024 18:06
@norlinhenrik
Copy link
Author

Hi @norlinhenrik. What is an adress "OK" ?

A valid postal address.
When we send physical mail to a mailing list (contact tag), we need to filter out those who don't have a valid address.

@legalsylvain
Copy link
Contributor

A valid postal address. When we send physical mail to a mailing list (contact tag), we need to filter out those who don't have a valid address.

Thanks !

  • Maybe such text could be added in the "help" field ?
  • By default, it should be False, don't you think ?

@Deriman-Alonso
Copy link
Member

Hi, as @legalsylvain already said, it should be False by default.
Also, could you check those tests?.
The rest looks just fine 👍.

@norlinhenrik
Copy link
Author

On the PR for 14.0 I set default=True.
In this PR for 16.0 it is removed. Then by default the boolean value is False.

@norlinhenrik
Copy link
Author

I set default=False to be explicit.
I also set a help description.

@@ -0,0 +1,2 @@
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl.html).
from . import test_partner_contact_id
Copy link
Member

Choose a reason for hiding this comment

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

Hi, I don't think this is the correct test you're trying to import, please fix this.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for finding this error. I have fixed it now.

class ResPartner(models.Model):
_inherit = "res.partner"

address_ok = fields.Boolean(
Copy link
Contributor

@leemannd leemannd Jul 22, 2024

Choose a reason for hiding this comment

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

I'm a bit perplexed.

It adds a boolean fields without adding any logic such as testing the validity of the address. We have something that exists in -> https://github.com/odoo/odoo/blob/17.0/addons/snailmail/models/snailmail_letter.py#L453

It could be achieved with tags on partners.
I would have prefered a field naming like is_valid_postal_address which is self explanatory.

NB: I will not block this PR. Feel free to take into account these points

Copy link
Author

Choose a reason for hiding this comment

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

Thank you @leemannd for very good feedback!

Copy link
Contributor

@leemannd leemannd Jul 22, 2024

Choose a reason for hiding this comment

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

NB: another solution that was used in 12.0 was to add it into the type -> https://github.com/OCA/partner-contact/tree/12.0/partner_postal_address
But it may not fulfill your need.

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.

4 participants