Skip to content

Conversation

@Sylsee
Copy link

@Sylsee Sylsee commented Nov 18, 2025

No description provided.

@robodoo
Copy link

robodoo commented Nov 18, 2025

Pull request status dashboard

Copy link

@yoba-odoo yoba-odoo left a comment

Choose a reason for hiding this comment

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

Nice work 👏
Just some small comments, Also if you can squash the LINT and FIX comments with their parent IMP commit it would be better to keep a clean commit history.

Copy link

@yoba-odoo yoba-odoo left a comment

Choose a reason for hiding this comment

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

Some more changes 😇

@Sylsee Sylsee force-pushed the 19.0-tutorial-sypol branch 2 times, most recently from 84e43df to 2afca8f Compare November 20, 2025 15:09
Copy link

@yoba-odoo yoba-odoo left a comment

Choose a reason for hiding this comment

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

Almost perfect 👌
Just pay attention to the EOL at the end of every file 😄

@Sylsee Sylsee force-pushed the 19.0-tutorial-sypol branch from d6392eb to 3819b02 Compare November 24, 2025 13:47
@api.model
def create(self, offers_list):
for offer in offers_list:
linked_property = self.env['estate.property'].browse(offer['property_id'])

Choose a reason for hiding this comment

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

Suggested change
linked_property = self.env['estate.property'].browse(offer['property_id'])
linked_property = self.env['estate.property'].browse(offer.get('property_id'))

When fetching something from a dictionary we tend to use get to avoid KeyError if the key doesn't exist. I know mostly the property_id will always be there but it is better safe than sorry 😅

Copy link
Author

Choose a reason for hiding this comment

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

Just learn a new python thing ty 😂

Copy link
Author

Choose a reason for hiding this comment

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

By doing that I got this error:
image

I would do that to fix it :

property_id = offer.get('property_id')
if not property_id:
    continue

linked_property = self.env['estate.property'].browse(property_id)

I just think I should raise an error instead of continuing but idk which type of error, I didn't saw any InternalError or something similar in the exceptions definitions. What would you do ?

Choose a reason for hiding this comment

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

Yup, my bad. When using get() and the key doesn't exist it will return None which might cause an Error as browse() is waiting for int

what you can do is fallback on a default value in get() like this:

offer.get('property_id', 0)

Copy link
Author

@Sylsee Sylsee Nov 25, 2025

Choose a reason for hiding this comment

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

But then if their is a property with an id of 0 it will not raise an error but change the wrong property no ?

Copy link

@yoba-odoo yoba-odoo Nov 26, 2025

Choose a reason for hiding this comment

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

Normally we don't have a record with id 0, our id sequences are 1-indexed. But to stick with the better-safe-than-sorry ideology we can do something like

if offer.get('property_id'):
    linked_property = self.env['estate.property'].browse(offer['property_id'])

@Sylsee Sylsee force-pushed the 19.0-tutorial-sypol branch 2 times, most recently from 86d8f5c to e7282d7 Compare November 25, 2025 12:42
@Sylsee Sylsee force-pushed the 19.0-tutorial-sypol branch from e7282d7 to 6dc7bc1 Compare November 25, 2025 12:43
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