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] tms #130

Merged
merged 17 commits into from
Aug 27, 2024
Merged

[ADD] tms #130

merged 17 commits into from
Aug 27, 2024

Conversation

max3903
Copy link
Member

@max3903 max3903 commented Jun 28, 2024

raise ValidationError(_("You must create an TMS order stage first."))

@api.onchange("route")
def _onchange_route(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

To adopt current Odoo approach, change these onchanges to computed stored readonly=False fields instead.

Choose a reason for hiding this comment

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

Can you send an example please?
What i'm trying to do is to empty the "route_id" field when I de-select the "route" boolean.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that flow is supported.

Comment on lines 85 to 86
latitude = fields.Float()
longitude = fields.Float()

Choose a reason for hiding this comment

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

Use partner_latitude from base

stage = fields.Char(related="stage_id.name")

# Relations
company_id = fields.Many2one("res.company", string="Company", default="")

Choose a reason for hiding this comment

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

fix default

driver_id = fields.Many2one("res.partner")

# Types
type = fields.Selection(selection=VEHICLE_TYPES)
Copy link

@jbaudoux jbaudoux Jul 17, 2024

Choose a reason for hiding this comment

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

I think this deserves a m2o instead of a selection
and rename to vehicule_type_id as type is a python class

tms/models/tms_vehicle.py Outdated Show resolved Hide resolved
Copy link

@jbaudoux jbaudoux left a comment

Choose a reason for hiding this comment

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

Can you add the readme directory and explain the concepts of this module

@pedrobaeza
Copy link
Member

Please remove the merge commits in the commit history.

sequence = fields.Integer(default=10)

route = fields.Boolean(string="Use predefined route")
route_id = fields.Many2one("tms.route")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
route_id = fields.Many2one("tms.route")
route_id = fields.Many2one("tms.route", compute="_compute_route_id", store=True, readonly=False)
@api.depends("route")
def _compute_route_id(self):
for order in self:
if not order.route_id and order.route_id:
order.route_id = False

Copy link
Contributor

Choose a reason for hiding this comment

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

tms/views/menu.xml Outdated Show resolved Hide resolved
tms_sale/__manifest__.py Outdated Show resolved Hide resolved
tms/__manifest__.py Outdated Show resolved Hide resolved
tms/models/res_partner.py Outdated Show resolved Hide resolved
tms/views/menu.xml Outdated Show resolved Hide resolved
Copy link
Member

@marcelsavegnago marcelsavegnago left a comment

Choose a reason for hiding this comment

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

I believe that the TMS type can remain only in the TMS tab. Therefore, I don't see the need to add it next to the contact type.

image

Comment on lines 89 to 91
date_start = fields.Datetime(readonly=True)
date_end = fields.Datetime(readonly=True)
duration = fields.Float(readonly=True, string="Trip Duration")
Copy link
Member

Choose a reason for hiding this comment

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

how would these fields be handled in case maintenance is needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

@marcelsavegnago If maintenance of the vehicle is needed, you would not be able to schedule a trip with that vehicle...

Copy link
Member

Choose a reason for hiding this comment

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

I'm referring to data maintenance. From what I understand, the start and end of the trip are recorded when the 'Start' and 'End' buttons are clicked. Therefore, if someone doesn't click 'Start' or 'Finish', we would need to change the recorded values.

@rvalyi
Copy link
Member

rvalyi commented Jul 25, 2024

@max3903 @rousseldenis @marcelsavegnago with the https://github.com/akretion/xsdata-odoo tool I presented at the OCA days 2021, you could quite easily generate all the Odoo data model for the UBL transport management entities:
https://docs.oasis-open.org/ubl/os-UBL-2.3/UBL-2.3.html#S-INTERMODAL-FREIGHT-MANAGEMENT

Aside from having a universally accepted and standard data model, it would also make it a piece of cake to have import and export of UBL XML documents for it...

Wouldn't that be preferable over bootstrapping a custom data model from scratch where all messaging mapping would need to be made and maintained manually?

Again, for reference here is the kind of Abstract Odoo mixins it generates from xsd schemas, here for the Brazilian Electronic Invoicing (the most complex in the world):
https://github.com/OCA/l10n-brazil/blob/16.0/l10n_br_nfe_spec/models/v4_0/leiaute_nfe_v4_00.py

For UBL, I may need to do small adaptations to xsdata-odoo to properly map the UBL intermediaries simple types to proper Odoo field types (I already started, it's not a lot of work.).

Disclaimer: may be the UBL Transport entities are not adapted to what you are trying to manage, but I believe it's pretty close to what @marcelsavegnago is dealing with for the Brazilian CT-e electronic documents. In this case, @marcelsavegnago is actually already using this datamodel generated by xsdata-odoo for the xsd CT-e Brazilian fiscal model https://github.com/OCA/l10n-brazil/tree/14.0/l10n_br_cte_spec/models/v4_0
But eventually UBL is a match for what you need, so I'm just mentioning the possibility.

What do you guys think?

(I'm also available for hire if you need a few days of work to make it work with that UBL schema properly)

@max3903 max3903 merged commit 6fd0dbb into OCA:17.0 Aug 27, 2024
6 checks passed
@max3903 max3903 deleted the 17.0_tms branch August 27, 2024 20:45
@pedrobaeza
Copy link
Member

@max3903 you must use the ocabot command instead of manually merging branches. There's no approval from the reviewers here either. Please refrain from merging without these conditions.

@rousseldenis
Copy link
Contributor

@max3903 I agree with @pedrobaeza. This is a little bit cavalier...

@rousseldenis
Copy link
Contributor

@OCA/logistics-maintainers

@rousseldenis
Copy link
Contributor

IMHO, some reviews comments should have been attended.

  • Partner model has been overcharged with a lot of fields that should have been put elsewhere or renamed. Why not having created a new model that inherits from res.partner (or at least having partner_id field) ?
  • Onchanges should have been removed.

Copy link

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

Indeed, there's no approval, there's a blocking review... This should not have been merged, especially since the PR is not stale. You were not waiting for reviewers feedback since weeks. Even if this module is alpha.

Now, if this is a way to speed up the work on all the other related PRs and to reduce the effort here, I understand your hurry but you should at least mention how you intend to follow up from here (you could have added TODOs in the code or add a roadmap for instance).

Also changes to dotfiles were committed together w/ module's changes which will cause conflicts when porting the 1st commit to other branches... 🙈

As this is the only module available in 17.0 and there's no pending PR if not yours and nobody is using this yet, we could even rewrite the history of this branch.

My $0.02

from odoo.tests.common import TransactionCase


class TestFleetVehicle(TransactionCase):
Copy link

Choose a reason for hiding this comment

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

You should use BaseCase everywhere when tracking is not needed.
setUp code should be moved to setUpClass unless is really needed to run something before every test runs.

@rousseldenis
Copy link
Contributor

As this is the only module available in 17.0 and there's no pending PR if not yours and nobody is using this yet, we could even rewrite the history of this branch.

Agree with that

@rousseldenis
Copy link
Contributor

@OCA/core-maintainers Could we reset the 17.0 branch ?

@max3903 Could you do after that a better PR? Even if it is Alpha, some conventions should be respected at least. Many thanks

@max3903
Copy link
Member Author

max3903 commented Sep 17, 2024

Hello,

Sorry for the late answer, I was on vacation during the last 2 weeks.

@EdgarRetes will take care of the requested changes. I would like to get this merged prior to OCA Days to present it and to start migration to version 18.0 while in Belgium.

@rousseldenis
Copy link
Contributor

I would like to get this merged prior to OCA Days to present it and to start migration to version 18.0 while in Belgium.

Ok, with that, but not at any price. Please respect OCA flows and requirements.

@simahawk Could you restore 17.0 branch?

@max3903 This will need a new PR for tms module.

I can help with review before OCA days.

@simahawk
Copy link

Here's a clean 17.0 branch reset to the commit before the module was introduced https://github.com/OCA/stock-logistics-transport/tree/17.0

Here's a backup of the 17.0 before the reset because translations have been added... in case you want to cherry-pick them

https://github.com/OCA/stock-logistics-transport/commits/17.0-tms-backup/

Let me know when is not needed anymore and I'll delete it.

@rousseldenis
Copy link
Contributor

Here's a clean 17.0 branch reset to the commit before the module was introduced https://github.com/OCA/stock-logistics-transport/tree/17.0

Here's a backup of the 17.0 before the reset because translations have been added... in case you want to cherry-pick them

https://github.com/OCA/stock-logistics-transport/commits/17.0-tms-backup/

Let me know when is not needed anymore and I'll delete it.

thanks!

@EdgarRetes Could you recreate the PR from your ursais:17.0_tmsbranch ? Thanks

@EdgarRetes
Copy link

EdgarRetes commented Sep 18, 2024

Here's a clean 17.0 branch reset to the commit before the module was introduced https://github.com/OCA/stock-logistics-transport/tree/17.0
Here's a backup of the 17.0 before the reset because translations have been added... in case you want to cherry-pick them
https://github.com/OCA/stock-logistics-transport/commits/17.0-tms-backup/
Let me know when is not needed anymore and I'll delete it.

thanks!

@EdgarRetes Could you recreate the PR from your ursais:17.0_tmsbranch ? Thanks

Done! #145

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.