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] real_estate: Addon to manage real estate #17

Open
wants to merge 9 commits into
base: 16.0
Choose a base branch
from

Conversation

curonny
Copy link

@curonny curonny commented May 24, 2024

No description provided.

@rousseldenis
Copy link
Contributor

What I wanted on the first version is to inherit from all properties of res.partner(and even added ones). As an estate has all properties of addresses.

This approach has a drawback is the implementation it needs. But IMHO, the advantages have more pro than cons. I'm not too much in favor of these changes

@curonny
Copy link
Author

curonny commented May 27, 2024

What I wanted on the first version is to inherit from all properties of res.partner(and even added ones). As an estate has all properties of addresses.

This approach has a drawback is the implementation it needs. But IMHO, the advantages have more pro than cons. I'm not too much in favor of these changes

I understand your point, but one of the problems, which arose when trying to use the currently published version, is that the fields of res.partner many2one list the contacts and also the properties, which forces us to add unnecessary domains so as not to list the properties .
Actually, I think the ideal behavior should be to isolate it, and link it with a product or partner according to the need of each solution to be carried out on this basis.

@pedrobaeza
Copy link
Member

I agree it should be an isolated one with the needed m2o, or maybe fields sync.

@curonny
Copy link
Author

curonny commented Jun 14, 2024

@rousseldenis So, would it be possible to validate the PR, with the new approach?

Copy link
Member

@mymage mymage left a comment

Choose a reason for hiding this comment

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

Hi all,
for a project I'm working on (16.0) I need the management of estate.
My needs are very basic at the moment so I could create my custom module, but if it possible to use an OCA one I am happier.
For what I can understand from the comments and the code, this version is "better" than the 13.0.
So if it is possible to update the commit to have a runboat to test I will do it.
Thanks

@curonny
Copy link
Author

curonny commented Sep 2, 2024

@jelenapoblet is this PR, the other PR it's closed

@jelenapoblet
Copy link

Got it @curonny , thank you! Is there anything else we can do to move this forward?

@curonny
Copy link
Author

curonny commented Sep 2, 2024

Got it @curonny , thank you! Is there anything else we can do to move this forward?

everything is done, waiting for approval

@rousseldenis
Copy link
Contributor

@curonny Could you provide migration scripts if needed ?

real_estate_latitude = fields.Float(digits=(10, 7))
real_estate_longitude = fields.Float(digits=(10, 7))
comment = fields.Html()
owner_ids = fields.Many2many(
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be enhanced in order to support different kind of property (bare ownership, ....).

Choose a reason for hiding this comment

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

Not sure if Im catching the idea. Right now we can assign rental estate type and if some filter is needed at contact level you could set it up in some MISC field.

Copy link
Contributor

Choose a reason for hiding this comment

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

The owner field relation is maybe too generic.

As there are several kind of (legal) ownerhsip on a property (bare ownership, usufruct, ...)

Choose a reason for hiding this comment

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

I created a middle model to handle those cases and added some ownerhsip type examples I found.

Copy link

@cvinh cvinh Jan 17, 2025

Choose a reason for hiding this comment

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

The owner field relation is maybe too generic.

As there are several kind of (legal) ownerhsip on a property (bare ownership, usufruct, ...)

... payer for taxes who can be someone else than the owner...
More complicated case : one payer for rubbish tax and another one for water tax
So maybe add some selection field with kind of relation with the estate... how about keeping inheritage on res.partner and integrating partner_multi_relation ?

Copy link

Choose a reason for hiding this comment

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

Why not using partner_multi_relation to have any kind of relation between real_estate and res_partner ? let's discuss that here

country_code = fields.Char(
related="country_id.code",
)
real_estate_latitude = fields.Float(digits=(10, 7))
Copy link
Contributor

Choose a reason for hiding this comment

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

We are in the real estate model, so removing real_estateshould be great for both fields

Choose a reason for hiding this comment

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

I removed those fields, maybe we can add it in the future with some geolocation feature

Copy link

Choose a reason for hiding this comment

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

@Christian-RB I think Denis means 'remove real_estate from the name of the field' not 'remove the field'

Choose a reason for hiding this comment

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

I removed the fields since they were not being displayed at all

@rousseldenis
Copy link
Contributor

@curonny Could you rebase ?

index=True,
)
active = fields.Boolean(default=True)
image = fields.Image(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you use theimage.mixinpresent in v16 in order to manage all sizes ?

Choose a reason for hiding this comment

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

Done, thanks for the tip

@Christian-RB Christian-RB force-pushed the 16.0-ADD-real-estate branch 3 times, most recently from 9e719b7 to 906ae2b Compare December 18, 2024 15:10
@Christian-RB
Copy link

Hi, @rousseldenis I will continue @curonny's work here, sorry for the delay. I have already applied some changes following your comments and I'm checking the migration scripts right now. Thanks in advance.

@cvinh
Copy link

cvinh commented Jan 20, 2025

Hello

After doing some research, we see that there are different approaches to manage real estates

  • In OCA we have a repo that inherits on res.partner as here in v13... there is a lot of work on that one and we can imagine it's in production somewhere

  • In Odoo industry and here, we have the property inheriting from account.analytic.account which is unexpected but nice when you think you could have some accounting data afterwards. It's only data, there is nothing more...

I don't know which is the best but I think account.analytic.account must be considered

Also, is there a chance for every approach to converge to a unique OCA's approach ?
Is there a place where contributors can discuss about that ?

@cvinh
Copy link

cvinh commented Jan 20, 2025

cc @max3903

@cvinh
Copy link

cvinh commented Jan 20, 2025

cc @vehi-invitu

@rousseldenis
Copy link
Contributor

Hello

After doing some research, we see that there are different approaches to manage real estates

  • In OCA we have a repo that inherits on res.partner as here in v13... there is a lot of work on that one and we can imagine it's in production somewhere
  • In Odoo industry and here, we have the property inheriting from account.analytic.account which is unexpected but nice when you think you could have some accounting data afterwards. It's only data, there is nothing more...

I don't know which is the best but I think account.analytic.account must be considered

Also, is there a chance for every approach to converge to a unique OCA's approach ? Is there a place where contributors can discuss about that ?

@cvinh Hi Cyril, indeed, I started the modules stack few years ago and have an instance running in v13. A discussion on this can be done here or in a separate issue.

In fact, I've implemented analytic too. You can create an issue, I will try to get in touch with updates as I've been busy for months on other topics.

@pedrobaeza
Copy link
Member

I also think that property should be a separated entity, and then connected to whatever you want (partner, analytic, product...), like for example projects auto-create analytic accounts on creation.

@cvinh
Copy link

cvinh commented Jan 20, 2025

Well do you think pms and vertical-estate can converge somehow ?
I know PMS and REMS can diverge according to business workflows but if I'm telling you that we need properties in a 3rd business (city halls or government that tax properties) it obvious that we should have an basic model describing the property itself and then link it to pms or vertical-estate or vertical-somethingelse...
BTW, analytic, address, owner, geodata and other fields could be common to both approaches before they diverge...
WDYT ?

@pedrobaeza
Copy link
Member

Yeah, both may have the same base property definition.

@cvinh
Copy link

cvinh commented Jan 20, 2025

Yeah, both may have the same base property definition.

What's your vision achieving this ? Another repo ?, take pms as basis and inherit it for vertical-* ?

@pedrobaeza
Copy link
Member

You can have the base module in OCA/pms, and inherit here.

@rousseldenis
Copy link
Contributor

You can have the base module in OCA/pms, and inherit here.

We can do that but in a new module as pms one is very big.

@pedrobaeza
Copy link
Member

Yeah, I was talking just about the repository host. The module may be base_property or similar.

@jelenapoblet
Copy link

I agree with the approach. Creating a lightweight base_property module that inherits from OCA/pms is a smart solution. @Christian-RB

@Christian-RB
Copy link

I believe I understand the approach. The idea would be as follows:

  • Analyze the PMS module to identify common property values shared with the Real Estate module.
  • Create a new module base_property in OCA/pms to house the shared data, and establish dependencies for both the PMS and Real Estate modules on this new module.
  • Develop migration scripts to reassign the relevant fields to the new module (this will likely be necessary).
  • Adapt the migration logic here to align with this structure and ensure consistency.

Let me know if I am right

@cvinh
Copy link

cvinh commented Jan 20, 2025

I believe I understand the approach. The idea would be as follows:

  • Analyze the PMS module to identify common property values shared with the Real Estate module.
  • Create a new module base_property in OCA/pms to house the shared data, and establish dependencies for both the PMS and Real Estate modules on this new module.
  • Develop migration scripts to reassign the relevant fields to the new module (this will likely be necessary).
  • Adapt the migration logic here to align with this structure and ensure consistency.

Let me know if I am right

I'm opening a new issue in pms repo to discuss about the common fields to have in base_property module

@max3903
Copy link
Member

max3903 commented Jan 20, 2025

We can do that but in a new module as pms one is very big.

FYI pms has been refactored in this branch:
https://github.com/OCA/pms/tree/14.0-pms_base

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants