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]car_rental: car rental #36

Open
wants to merge 43 commits into
base: 17.0
Choose a base branch
from

Conversation

kir-odoo
Copy link
Contributor

@kir-odoo kir-odoo commented Nov 7, 2023

task-3460478

@jva-odoo jva-odoo changed the title 17.0 industry car rental dhrs [ADD]car_rental: car rental Nov 8, 2023
@jva-odoo jva-odoo force-pushed the 17.0-industry-car-rental-dhrs branch from 95024e9 to 396353e Compare November 8, 2023 11:41
@jva-odoo jva-odoo added the ready PR is ready for review and integration label Nov 8, 2023
@xavieralt xavieralt removed their assignment Nov 16, 2023
Copy link
Collaborator

@vava-odoo vava-odoo left a comment

Choose a reason for hiding this comment

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

Hi @kir-odoo
Thanks for this work!
Please find below a (small 👀) list of comments. Please apply to all files.
Since this is my first PR review in industry, there may be some comments that we need to discuss, so don't hesitate to reply if you don't agree.
Cheers

car_rental/__manifest__.py Outdated Show resolved Hide resolved
car_rental/__manifest__.py Outdated Show resolved Hide resolved
car_rental/__manifest__.py Outdated Show resolved Hide resolved
car_rental/data/account_analytic_account.xml Outdated Show resolved Hide resolved
car_rental/data/account_analytic_plan.xml Outdated Show resolved Hide resolved
car_rental/demo/sale_order_post.xml Outdated Show resolved Hide resolved
car_rental/__manifest__.py Outdated Show resolved Hide resolved
car_rental/__manifest__.py Outdated Show resolved Hide resolved
car_rental/__manifest__.py Outdated Show resolved Hide resolved
car_rental/__manifest__.py Show resolved Hide resolved
Copy link
Collaborator

@vava-odoo vava-odoo left a comment

Choose a reason for hiding this comment

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

Second review based on a functional testing. I'll add a few other remarks on the task.

car_rental/__manifest__.py Outdated Show resolved Hide resolved
car_rental/__manifest__.py Outdated Show resolved Hide resolved
car_rental/__manifest__.py Outdated Show resolved Hide resolved
car_rental/demo/res_config_settings.xml Outdated Show resolved Hide resolved
car_rental/__manifest__.py Outdated Show resolved Hide resolved
car_rental/__manifest__.py Outdated Show resolved Hide resolved
car_rental/__manifest__.py Outdated Show resolved Hide resolved
@vava-odoo
Copy link
Collaborator

Missing file: car_rental/static/description/icon.png

@dhrs-odoo dhrs-odoo force-pushed the 17.0-industry-car-rental-dhrs branch 4 times, most recently from 182aaf2 to faf8436 Compare November 24, 2023 05:36
@dhrs-odoo dhrs-odoo force-pushed the 17.0-industry-car-rental-dhrs branch 13 times, most recently from 211067e to 363cd93 Compare December 8, 2023 08:40
Copy link
Collaborator

@vava-odoo vava-odoo left a comment

Choose a reason for hiding this comment

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

Hi @dhrs-odoo
Still a few things to improve here.
Most important is without sale.temporal.recurrence you can't have rental prices...
Also, could you make sure the runbot is green?
Cheers

car_rental/data/ir_model_fields.xml Outdated Show resolved Hide resolved
<br />
]]>
</field>
<field name="email_normalized">megha@gmail.com</field>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better be cautious

Suggested change
<field name="email_normalized">megha@gmail.com</field>
<field name="email_normalized">megha@example.com</field>

car_rental/demo/purchase_order.xml Outdated Show resolved Hide resolved
car_rental/demo/sale_order_line.xml Outdated Show resolved Hide resolved
car_rental/demo/sale_order_line.xml Outdated Show resolved Hide resolved
car_rental/demo/website.xml Show resolved Hide resolved
car_rental/demo/sale_order_post.xml Outdated Show resolved Hide resolved
car_rental/demo/website_view.xml Outdated Show resolved Hide resolved
@@ -0,0 +1,33 @@
<?xml version='1.0' encoding='UTF-8'?>
<odoo>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need this file? There is no theme to apply...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes we need this file

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unused?

@dhrs-odoo dhrs-odoo force-pushed the 17.0-industry-car-rental-dhrs branch 3 times, most recently from bdbcc0d to 7ce9e16 Compare December 20, 2023 07:04
@dhrs-odoo dhrs-odoo force-pushed the 17.0-industry-car-rental-dhrs branch from 80a3d3c to 58ddf6d Compare April 19, 2024 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready PR is ready for review and integration
Projects
None yet
5 participants