-
Notifications
You must be signed in to change notification settings - Fork 2.2k
18.0 estate ibmo #818
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
base: 18.0
Are you sure you want to change the base?
18.0 estate ibmo #818
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are a few comments
estate/__manifest__.py
Outdated
'application': True, | ||
'auto_install': False, | ||
'license': 'LGPL-3', | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try to put a return character at the end of the last line in every file.
This way, when a new line is added to the file, that line is not impacted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
estate/models/estate_property.py
Outdated
garage = fields.Boolean(string='Garage') | ||
garden = fields.Boolean(string='Garden') | ||
garden_area = fields.Integer(string='Garden Area') | ||
arden_orientation = fields.Selection(string='Arden Orientation', selection=[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arden_orientation
=> garden_orientation
😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
estate/models/estate_property.py
Outdated
('north', 'North'), | ||
('south', 'South'), | ||
('east', 'East'), | ||
('west', 'West') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In python, we try to follow the following convention for quoting strings:
- single quote for technical terms
- double quotes for displayed texts
=> ('north', "North")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
estate/views/estate_menus.xml
Outdated
|
||
<menuitem id="estate_property_menu_root" name="Estate Property Menu"> | ||
<menuitem id="estate_property_level_menu" name="First Level"> | ||
<menuitem id="estate_property_menu_action" action="estate_property_menu_action"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This menuitem has the same id as the action. Do not mention menu
in the action id, this will solve the data import issue you are encountering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
estate/models/estate_property.py
Outdated
arden_orientation = fields.Selection(string='Arden Orientation', selection=[ | ||
('north', 'North'), | ||
('south', 'South'), | ||
('east', 'East'), | ||
('west', 'West') | ||
]) No newline at end of file | ||
garden_orientation = fields.Selection(string='Arden Orientation', selection=[ | ||
('north', "North"), | ||
('south', "South"), | ||
('east', "East"), | ||
('west', "West") | ||
]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string='Arden
=> string="Garden
😉
But, let's try to make those changes in the commit where they originally appeared.
Look at the git rebase -i
command to rewrite you branch's history.
You will then need to use git push --force-with-lease
to send such changes to the repo. (Try not to use git push --force
to avoid accidents when working together with other people on a common branch)
Upon merge, you will want the commits of your PR to actually describe what is changed and why. The intermediary development attempts and typo fixes are not interresting for the future reader who will try to find out why some lines were added three years ago. So, for small intra-branch fixes, always try to make them in the commit where they should initially have been correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@MostafaTwfiq Didn't you forget to push further changes ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are a few comments.
Do not hesitate to ask for help on how to rewrite your commit history.
for record in self: | ||
if (not float_utils.float_is_zero(record.selling_price, precision_digits=2) | ||
and float_utils.float_compare(record.selling_price, record.expected_price * 0.9, precision_digits=2) < 0): | ||
raise ValidationError("Selling price can't be lower than 90% of expected price") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those strings are not implicitly translated. You have to call _
explicitly. (from odoo import _
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
estate/__manifest__.py
Outdated
@@ -3,7 +3,6 @@ | |||
'depends': ['base'], | |||
'category': 'Real Estate/Brokerage', | |||
'data': [ | |||
#'security/estate_security.xml', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to make such changes when they initially appeared - so that they just never happened.
See #818 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
estate/models/__init__.py
Outdated
from . import estate_property_type | ||
from . import estate_property_tag | ||
from . import estate_property_offer | ||
from . import estate_property_extend_users |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing return at the end of the last line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
estate/models/estate_property.py
Outdated
('offer received', "Offer Received"), | ||
('offer accepted', "Offer Accepted"), | ||
('offer_received', "Offer Received"), | ||
('offer_accepted', "Offer Accepted"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to fix it inside its original commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
estate/views/estate_menus.xml
Outdated
<menuitem id="estate_property_menu_root" name="Estate Property"> | ||
<menuitem id="estate_property_level_menu" name="Estate Property"> | ||
<menuitem id="estate_property_menu_root" name="Real Estate"> | ||
<menuitem id="estate_property_level_menu" name="Advertisements"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to fix it inside its original commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
d668542
to
957f5cf
Compare
this.addTodo = this.addTodo.bind(this); | ||
this.inputFocus = this.inputFocus.bind(this); | ||
this.onStatusChange = this.onStatusChange.bind(this); | ||
this.deleteTodoItem = this.deleteTodoItem.bind(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also achieve this by using .bind
in the template XML.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
80d4758
to
092308e
Compare
No description provided.