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

8.0 add addresses mgmt #6

Merged
merged 34 commits into from
Apr 2, 2015
Merged

Conversation

gurneyalex
Copy link
Member

Create 4 addons:

  • stock_transport_multi_address
  • sale_transport_multi_address
  • purchase_transport_multi_address
  • purchase_requisition_transport_multi_address

which work together to manage origin and delivery addresses, as well as consignees on company processes.

These modules unify and integrate what was previously available scattered in various more or less independent addons from differents OCA repositories:

Needs merging together with:

@lepistone
Copy link
Member

@gurneyalex I think the READMEs are a bit messed up: for example, the one for stock talk about sale.

@max3903 max3903 modified the milestone: 8.0 Mar 20, 2015
@gurneyalex
Copy link
Member Author

@lepistone: fixed, thanks

@bealdav
Copy link
Member

bealdav commented Mar 21, 2015

Hi @gurneyalex, thanks for you module series but why they are in plural form ?
95% of the modules are in singular, why don't follow this convention easier to remember ?
Yes I know, these modules manages several addresses but sale module also manage several sales, at least I hope.
Technical folder name could be 'stock_address' and name display module could be 'Stock Addresses'.
Don't you think ? Thanks

@pedrobaeza
Copy link
Member

I see better to put stock_multi_address to determine the plural meaning.

@bealdav
Copy link
Member

bealdav commented Mar 21, 2015

@gurneyalex Have you considered to use this module
https://github.com/akretion/carrier-delivery-ak/blob/7.0/delivery_dropoff_site/__openerp__.py.
It seems the concept is near, consignee is like dropoff site
@guewen use it for so_colissimo

@gurneyalex
Copy link
Member Author

@bealdav because we are dealing with multiple addresses (origin, delivery and consignee).
@pedrobaeza I don't find "stock-multi-address" more helpful or less generic.

Would you object to stock_transport_addresses? or stock_transport_address (but this sounds like the module only handle 1 address which it does not).

@gurneyalex
Copy link
Member Author

@bealdav: regarding dropoff site

  1. it's v7
  2. it does not handle consignee or origin

@bealdav
Copy link
Member

bealdav commented Mar 23, 2015

@gurneyalex thanks for your answer:

  • delivery_dropoff_site: it handles consignee here https://github.com/akretion/carrier-delivery-ak/blob/7.0/delivery_dropoff_site/stock.py#L20 and delivery here
    https://github.com/akretion/carrier-delivery-ak/blob/7.0/delivery_dropoff_site/partner.py#L60. You're right my module is not really visible on ak repo, and some dependencies should be removed to join OCA.
  • I don't know exactly what is the use case of your modules (example welcome). In the different addresses, is there different carrier providers. If yes, the module shouldn't be more generic if it supports to store in Odoo the specific carrier data (if any) with 'inherits' mechanism in these base modules ? I'm not sure of nothing because I don't know the use case, it's just an idea.
  • about module name, your approach has its logic. But on my side, if I write a module called 'image' to manage images with a field image, I don't want change the name if I add a new field to manage other image types. I think (but maybe it's only me, and in this case, no problem) if i let the possibility to have plural, then I finish with less consistency (mrp_operations, res_groups for table). KISS could also applied to module folder name (you have summary to be more explicit and name in _openerp.py for plural and sound good). But if OCA says, we should use plural when it needs, Akretion should rename 98% of its modules. And then we'll do it.
    It just miss a convention here http://odoo-community.org/page/how-to to close my sterile debates as says @rvalyi ;-) [ EDIT: I just see convention is defined. I suppose 'prefer' will no close debates ;-( ]
    my 0.5 cts

@pedrobaeza
Copy link
Member

I agree with @bealdav. At first, I fell in the same mistake naming my modules, but it is less consistency. You can call it stock_transport_multi_address if you want to specify a little more.

This is what I've done with product_images module, renamed to product_multi_image here: OCA/product-attribute#57

@sebastienbeau
Copy link
Member

GREAT work Alex ! For the delivery_dropoff_site we can depend of your module without any problem. It's just that we need to take in account this change for so-colissimo (@guewen no sure that you need to do both colissimo and socolissimo).
For the module name I also prefered without "s". Even if there is many it's better (I think) to always use singular.

Thanks

@gurneyalex
Copy link
Member Author

ok I'll update the names to xxx_transport_multi_address

@gurneyalex
Copy link
Member Author

@bealdav I am about to create the 7.0 branch in this repository, and then I propose we work together on delivery_dropoff_site to get something which can be migrated to 8.0 to these modules (and if necessary amend the xxx_transport_multi_address modules to ease this migration) . Would this be ok with you?

@bealdav
Copy link
Member

bealdav commented Mar 24, 2015

@gurneyalex Thank you alex, but on our side we will focus on the present and future -> v8. We only use this ddrop_off_site only for one client and it works in v7. When we'll migrate so_colissimo in v8 we'll add dependency on your module. Thank you for job with your full tested modules. It's also for that we want to depend from them.
cc @sebastienbeau

"""Find a picking type from the address.

There is a similar onchange in the module
purchase_requisition_delivery_address. A similar logic to choose the
Copy link

Choose a reason for hiding this comment

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

two spaces

@yvaucher
Copy link
Member

👍

@gurneyalex
Copy link
Member Author

@yvaucher thanks for fixing the test

@nbessi
Copy link

nbessi commented Mar 31, 2015

👍 due to the high numbers of commit after first review pass, I think we should wait 5 days before merging

@yvaucher
Copy link
Member

yvaucher commented Apr 1, 2015

@nbessi In fact it was rebased so there are not that much commit in addition

@pedrobaeza @sebastienbeau issue due to naming of module was fixed is it ok for you?

Description
===========

This module (together with stock_addresses and purchase_addresses) manages in a
Copy link
Member

Choose a reason for hiding this comment

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

Change this text to match new module names

@yvaucher
Copy link
Member

yvaucher commented Apr 2, 2015

@pedrobaeza Thanks, I fixed those README files

@pedrobaeza
Copy link
Member

I think now it's ready for merging 👍

jgrandguillaume added a commit that referenced this pull request Apr 2, 2015
@jgrandguillaume jgrandguillaume merged commit d950dd3 into OCA:8.0 Apr 2, 2015
max3903 pushed a commit to ursais/stock-logistics-transport that referenced this pull request Jun 19, 2024
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.

10 participants