-
Notifications
You must be signed in to change notification settings - Fork 266
[17.0][ADD] fieldservice_base_location: New module fieldservice_base_location #1367
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: 17.0
Are you sure you want to change the base?
[17.0][ADD] fieldservice_base_location: New module fieldservice_base_location #1367
Conversation
ce93cdf to
c3f5bf8
Compare
ppyczko
left a 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.
LGTM, code review and tested in runboat!
peluko00
left a 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.
LGTM, code review and tested in runboat
lbarry-apsl
left a 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.
Code and runboat reviewed, LGTM!
mpascuall
left a 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.
LGTM!
|
This PR has the |
82c8525 to
9a2c51b
Compare
ivantodorovich
left a 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.
IMO this module should be named fieldservice_base_location or fieldservice_partner_location, and be auto-installed when base_location is installed
| class FsmLocation(models.Model): | ||
| _inherit = "fsm.location" | ||
|
|
||
| def write(self, vals): |
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 don't need a write override, since fsm.location inherits from res.partner the city_id, state_id, country_id etc are already computed whenever zip_id changes..
I think the only thing this module needs to do is to add the field to the view, similarly to how base_location is doing for the partner: https://github.com/OCA/partner-contact/blob/17.0/base_location/views/res_partner_view.xml, with possibly also its _fields_view_get override if needed, to accomplish the same UX than the partner view
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 @ivantodorovich, thanks for your feedback.
The thing is that it really needs the overwritten write method, otherwise it ends up throwing errors due to mismatches in partner fields.
For example, when you change the ZIP code or use the autocomplete in the field service location, saving will throw an error because the partner data does not match the ZIP.
The only solution I found was to overwrite the write method.
Regarding the module name, fieldservice_base_location is fine for me.
Let me know your thoughts!
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 @BernatObrador
Is this covered by the unit tests you wrote?
Meaning, would they fail without the write override?
If we keep it, then, please write a detailed comment explaining the error you get and how this solves it.
def write(self, vals):
# OVERRIDE to ...
# ...
# ...9a2c51b to
70675a7
Compare
70675a7 to
339cac3
Compare
|
|
||
| @classmethod | ||
| def _request_handler(cls, s, r, /, **kw): | ||
| """Don't block external requests.""" | ||
| return cls._super_send(s, r, **kw) |
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.
What's this for?
| @classmethod | |
| def _request_handler(cls, s, r, /, **kw): | |
| """Don't block external requests.""" | |
| return cls._super_send(s, r, **kw) |
| class FsmLocation(models.Model): | ||
| _inherit = "fsm.location" | ||
|
|
||
| def write(self, vals): |
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 @BernatObrador
Is this covered by the unit tests you wrote?
Meaning, would they fail without the write override?
If we keep it, then, please write a detailed comment explaining the error you get and how this solves it.
def write(self, vals):
# OVERRIDE to ...
# ...
# ...
This module adds the zip_id field to the fsm.location form, enabling automatic address completion based on the ZIP code entered in the location’s completion field.
To use the module, follow these steps:
Go to Contacts → Configuration → ZIPs.
Create a new ZIP entry.
Navigate to Field Service → Master Data → Locations.
Select an existing location or create a new one.
In the address section, you’ll now see the ZIP code autocomplete field.
Simply enter the ZIP code, and the system will automatically fill in all related address fields.
cc https://github.com/APSL 7083
@miquelalzanillas @lbarry-apsl @mpascuall @peluko00 @javierobcn @ppyczko please review