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

[ADD] estate: added a new real estate module #315

Closed
wants to merge 2 commits into from

Conversation

aych-odoo
Copy link

Created a new real estate module for technical training.

Day 1:
  Chapter 2: created directory structure along with manifest file for estate module
  Chapter 3: created estate_properties model
Day 2:
  Chapter 4: created ir.model.access.csv to enable CRUD operation on property
  Chapter 5: created basic action and menu for estate property
  Chapter 6: created basic views for estate property i.e. list, form, search
  Chapter 7: created property type model and defined Many2one relation.
@robodoo
Copy link

robodoo commented Feb 5, 2025

Pull request status dashboard

Copy link

@maad-odoo maad-odoo left a comment

Choose a reason for hiding this comment

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

Apart from the below suggestion, things that can be improved are,

  • there are alot of empty line left unnecessary. Please avoid that.
  • Many field definition are missing the string property. Add the string helps to know the purpose of the field

@@ -0,0 +1,3 @@
id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink
access_estate_property,estate.property,model_estate_property,base.group_user,1,1,1,1
access_estate_property_type,estate.property.type,model_estate_property_type,base.group_user,1,1,1,1

Choose a reason for hiding this comment

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

EOF line is missing

parent="estate_menu_settings"
action="estate_property_types_action"
/>
</odoo>

Choose a reason for hiding this comment

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

Same

<field name="res_model">estate.property.type</field>
<field name="view_mode">list,form</field>
</record>
</odoo>

Choose a reason for hiding this comment

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

Same

<field name="res_model">estate.property</field>
<field name="view_mode">list,form</field>
</record>
</odoo>

Choose a reason for hiding this comment

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

Same

garden = fields.Boolean()
garden_area = fields.Integer(string="Garden Area (sqm)")
garden_orientation = fields.Selection(
selection=GARDEN_ORIENTATION_SELECTION

Choose a reason for hiding this comment

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

This approach is not appropriate in this case.

Typically, we separate selection options from the field definition when they are processed through methods from other models or linked to relational fields. This is particularly important when there's a high likelihood of changes driven by business requirements, such as module dependencies.

Comment on lines +58 to +60
selection=STATE_SELECTION,
required=True,
default=STATE_SELECTION[0][0]

Choose a reason for hiding this comment

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

Same as above

<group>
<field name="selling_price" />
</group>
</group>

Choose a reason for hiding this comment

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

Regarding the groups tag, there are additional attributes or options available to specify the type of group, providing better insight into the kinds of fields it contains.
You can explore them too...

]


class Property(models.Model):

Choose a reason for hiding this comment

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

Class name should be EstatePropery as the model name is estate.propertyto follow the naming convention.

from odoo import models, fields


class PropertyType(models.Model):

Choose a reason for hiding this comment

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

Class name is not right. Refer to the above comment.

@aych-odoo
Copy link
Author

Moved to #326

@aych-odoo aych-odoo closed this Feb 6, 2025
@aych-odoo aych-odoo deleted the 18.0-training-aych branch February 6, 2025 08:54
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.

3 participants