- 
                Notifications
    You must be signed in to change notification settings 
- Fork 2.6k
[ADD] estate{_account}: added estate{_account} moduels - MOALN #1003
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: 19.0
Are you sure you want to change the base?
Conversation
f4dbaf3    to
    405330c      
    Compare
  
    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.
Hi @Mohamed-Khaled308 👋,
The naming of the view and the pip8 are respected almost everywhere. Be careful to write only required code, as we are a lot to work on odoo the code base can quickly become huge.
Don't forget to add a title and a description to your PR. The title must have same structure than a commit -> [ADD/IMP/FIX....] app: what the pr is for. About the description, it must mainly explain why you did the PR and not how you did it.
You did a great job 😃!
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.
Hi @Mohamed-Khaled308 👋,
I did a review of your PR. Here are some comments about the pep8 and the code simplification.
Thanks for your work 😃
| from . import estate_property | ||
| from . import estate_property_type | ||
| from . import estate_property_tag | ||
| from . import estate_property_offer | ||
| from . import res_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.
Should be sorted alphabetically. That's in the pep8 . It's the official documentation to write clean python 😃
| from . import estate_property | |
| from . import estate_property_type | |
| from . import estate_property_tag | |
| from . import estate_property_offer | |
| from . import res_users | |
| from . import estate_property | |
| from . import estate_property_offer | |
| from . import estate_property_tag | |
| from . import estate_property_type | |
| from . import res_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.
Is there any automatic way that checks these format errors and fixes them maybe?
I am using pycharm now btw.
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.
Maybe you can do as explain here? I've never tried it.
| Hi @Mohamed-Khaled308 😃, You should also add a title and a description to your PR. The title must have the same structure than a commit title -> [ADD/IMP/FIX...] app: the purpose of the pr (must start by a verb). And for the description, it must mainly explain why the PR is doing this and not how. Cheers ! | 
c0e98b3    to
    e6b35b9      
    Compare
  
    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.
Thanks for the updates 😃. Just small changes to do, they are not a priority.
The description is quite good. For the title, you could do two small corrections. The pentagram at the end of it is not required. And you don't need to specify twice the modules impacted by you changes. So your title should look like "[ADD] estate{_account}: added modules"
Cheers !
| <field name="validity" string="Validity (days)"/> | ||
| <field name="date_deadline" string="Deadline"/> | ||
| <button name="action_accept_offer" type="object" icon="fa-check" | ||
| invisible="status or property_state in['offer_accepted', 'sold', 'cancelled']"/> | 
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 forgot the space here 😅.
| <button name="action_accept_offer" type="object" icon="fa-check" | ||
| invisible="status or property_state in['offer_accepted', 'sold', 'cancelled']"/> | ||
| <button name="action_refuse_offer" type="object" icon="fa-close" | ||
| invisible="status or property_state in['offer_accepted', 'sold', 'cancelled']"/> | 
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.
Same here.
| from . import estate_property | ||
| from . import estate_property_type | ||
| from . import estate_property_tag | ||
| from . import estate_property_offer | ||
| from . import res_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.
Maybe you can do as explain here? I've never tried it.
| @api.depends("offer_ids") | ||
| def _compute_offer_count(self): | ||
| for type in self: | ||
| type.offer_count = len(type.offer_ids or []) | 
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.
[] shouldn't be necessary as offer_ids will always be a list even if there is no records.

Added new Modules: estate{_account}