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 : adding new module in tutorials #216

Open
wants to merge 22 commits into
base: 18.0
Choose a base branch
from

Conversation

AbdelrahmanFawzy1
Copy link

No description provided.

@robodoo
Copy link

robodoo commented Dec 17, 2024

Pull request status dashboard

Copy link

@kfr-odoo kfr-odoo left a comment

Choose a reason for hiding this comment

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

You are on the right track

  • Configure your IDE to get ride of EOF issues. "Insert Final Newline" in VScode.
  • Remove all # -*- coding: utf-8 -*-, they are not required anymore (since version 16.0).
  • Keep your python imports in alphabetical order to prevent duplication (ref: estate_property.py)

'summary': "Realistic business advertisements",
'application': True,
'installable': True,
'auto_install': False,

Choose a reason for hiding this comment

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

This is already the default (ref: doc)





Choose a reason for hiding this comment

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

two blank lines before class declaration

Suggested change

@api.onchange('garden')
def _onchange_garden(self):
self.garden_area = 10 if self.garden else False
self.garden_orientation = 'n' if self.garden else False

Choose a reason for hiding this comment

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

Does not seem to match your selection field declaration


@api.onchange('garden')
def _onchange_garden(self):
self.garden_area = 10 if self.garden else False

Choose a reason for hiding this comment

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

Suggested change
self.garden_area = 10 if self.garden else False
self.garden_area = 10 if self.garden else 0

estate/models/estate_property.py Outdated Show resolved Hide resolved
estate/models/estate_property.py Outdated Show resolved Hide resolved

def action_accept(self):
for record in self:
try:

Choose a reason for hiding this comment

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

Why is the try catch necessary ? Either you fix the issue or add a comment to explain why it might behave unexpectedly

record.property_id.action_process_accept(self)
record.status = 'accepted'
except UserError as e:
raise e

Choose a reason for hiding this comment

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

Only raise builtin odoo errors for optimal UX

Suggested change
raise e
raise UserError("error message")

estate/models/estate_property_offer.py Outdated Show resolved Hide resolved
@kfr-odoo
Copy link

@AbdelrahmanFawzy1 The CI tool is still red, please try to fix the issues

raise UserError("Cancelled Items cannot be sold.")
self.state = "sold"

def action_set_sold(self):

Choose a reason for hiding this comment

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

no loop in action coming from a form view

def _compute_date_deadline(self):
for record in self:
if not record.create_date:
record.create_date = fields.Date.today()

Choose a reason for hiding this comment

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

You should probably refrain from writing on super fields such as id, create_date, etc...

raise UserError("this property has already an accepted offer!!")
self.state = "offer_accepted"
self.selling_price = offer.price
self.partner_id = offer.partner_id

Choose a reason for hiding this comment

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

missing return

Copy link
Author

Choose a reason for hiding this comment

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

Why do I need it? I am just setting self attributes?



@api.ondelete(at_uninstall=False)
def prevent_delete(self):

Choose a reason for hiding this comment

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

naming convention

Suggested change
def prevent_delete(self):
def _unlink_property(self):

("check_price", "CHECK(price >= 0)", "Offer prices MUST be postive."),
]

def action_accept(self):

Choose a reason for hiding this comment

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

no loop in form action

@@ -0,0 +1,30 @@
from odoo import models, Command

class InheritedEstateProperty(models.Model):

Choose a reason for hiding this comment

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

Suggested change
class InheritedEstateProperty(models.Model):
class EstateProperty(models.Model):

@@ -0,0 +1,2 @@
id,name,model_id/id,group_id/id,perm_read,perm_write,perm_create,perm_unlink
estate.access_estate_property,access_estate_property,estate.model_estate_property,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.

This access rule exists already in the estate module

Copy link
Author

Choose a reason for hiding this comment

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

Doesn't each module have to own its own access rules?! Is it because we inherit from the estate module, then its access rules apply here as well?

this.todoItemsNr =0;
useAutofocus("todo_input");
}
addTodo(inp) {

Choose a reason for hiding this comment

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

JS convention

Suggested change
addTodo(inp) {
addTodo(event ) {


}
inp.target.value = '';
this.render();

Choose a reason for hiding this comment

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

You should probably not need to call this method explicitly

Suggested change
this.render();


@api.model_create_multi
def create(self, vals):
property_record = self.env['estate.property'].browse(vals['property_id'])

Choose a reason for hiding this comment

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

Suggested change
property_record = self.env['estate.property'].browse(vals['property_id'])
property_record = self.env['estate.property'].browse(vals['property_id']).exists()

existing_offers = self.search([
('property_id', '=', vals['property_id'])
])
if any(offer.price >= vals['price'] for offer in existing_offers):

Choose a reason for hiding this comment

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

The price comparison should ba a part of the search query above for optimal performance

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