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] location, territory, branch, district, region #27

Merged
merged 2 commits into from
Oct 25, 2018

Conversation

max3903
Copy link
Member

@max3903 max3903 commented Oct 18, 2018

Fixing comments on #22

Copy link
Member

@dreispt dreispt left a comment

Choose a reason for hiding this comment

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

I suggest model design improvements.


class Branch(models.Model):
_name = 'branch'
_description = 'branch'
Copy link
Member

Choose a reason for hiding this comment

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

I think this overlaps with hr.department.
That's what I'm using today for the same purpose.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dreispt The idea behind branch is mostly where the company has offices. Department is not necessarily geographically centralized...

_description = 'branch'

name = fields.Char(string='Name')
district_id = fields.Many2one('district', string='District')
Copy link
Member

Choose a reason for hiding this comment

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

If we add a partner_id Address field to the Branch/Department, we automatically have a full address.
If we need additional fields for it, modules like base_location would do that for us.

_description = 'District'

name = fields.Char(string='Name')
region_id = fields.Many2one('region', string='Region')
Copy link
Member

Choose a reason for hiding this comment

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

Instead of the fixed District -> Region -> ... multimodel fixed structure, we could have a flexible single model hierarchy structure. We could even include "Branch" and "Territory" as hierarchy layers, and avoid forcing a dependency on HR (it would then become an optional link).

Copy link
Member Author

Choose a reason for hiding this comment

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

@dreispt Your comments makes total sense from a functional perspective, but I think @wolfhall has specific geospatial reasons behind those models.

@wolfhall Am I right?

@@ -9,6 +11,13 @@ class FSMLocation(models.Model):
_inherits = {'res.partner': 'partner_id'}
Copy link
Member

Choose a reason for hiding this comment

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

New style delegation inheritance: remove _inherits and instead add delegage=True on the partner_id field.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

fieldservice/models/fsm_location.py Show resolved Hide resolved

name = fields.Char(string='Name')
complete_name = fields.Char("Full Location Name",
compute='_compute_complete_name', store=True)
Copy link
Member

Choose a reason for hiding this comment

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

+index=True

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

ondelete='restrict')
notes = fields.Html(string='Notes')
parent_left = fields.Integer('Left Parent', index=True)
parent_right = fields.Integer('Right Parent', index=True)
Copy link
Member

Choose a reason for hiding this comment

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

I think these are not used anymore, but I need to check (maybe on v12 only)

Copy link
Member Author

Choose a reason for hiding this comment

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

@dreispt v12 ;)

building = fields.Char(string='Building', size=35)
floor = fields.Char(string='Floor', size=35)
unit = fields.Char(string='Unit', size=35)
room = fields.Char(string='Room', size=35)
Copy link
Member

Choose a reason for hiding this comment

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

These building/floor/room details don't need to be structured data.
When needed they can perfectly be included in the Location's name or Description.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dreispt that's why we removed them.

Copy link

@wolfhall wolfhall left a comment

Choose a reason for hiding this comment

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

Looks good

@wolfhall wolfhall merged commit e24921b into OCA:11.0 Oct 25, 2018
@dreispt
Copy link
Member

dreispt commented Oct 25, 2018

Can we have the Branch /Territory hierarchy instead?

@brian10048
Copy link
Contributor

brian10048 commented Oct 26, 2018

I like where this is going, but from reading the code and from a user's perspective the location model is confusingly named when we have already established the fsm_location model. As @wolfhall suggested in #22 the newer location model aims to represent a specific point of service within the fsm_location, so could we rename it service_point or something similar?

That being said, wouldn't we have a relationship of one fsm_location to many service_points? Each service_point is unique to that fsm_location. The configuration area could establish multiple service_point_types that create the hierarchy of service_points within the fsm_location others have mentioned needing.

Let me know if this makes sense and I'll put something in motion to demonstrate Please see #33

@max3903 max3903 deleted the 11.0-fix-location branch March 16, 2019 18:55
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.

5 participants