Set the proper company_id on an imported partner #71
Conversation
|
Hi, @OSguard @sebastienbeau what do you think? |
In fact, it leaves it empty, so the partners are shared and not restricted to one company. |
|
AFAIK, that's not what's happening on our system (and even though we often put company_id into the context to fix other company issues for new objects, this is not the case with partner import), the company_id on the record is not set, but the partner ends up owned by a company. |
|
The default value for BTW if we apply the solution in #52, we could call You can still add this rule by extending a custom backend. |
|
If you have no rule, then the the company_id of the user will applied: https://github.com/odoo/odoo/blob/7.0/openerp/addons/base/res/res_company.py#L216 So is it possible to have multi connector user for each company? I check our installations, we have no partner without company_id! From my point this patch makes sense and changes not on existing installations. |
|
I also think it makes sense. You are creating a partner with its corresponding sales orders and so on, and this is always going to be associated to one company (the one that the shop belongs to). If then, you want to remove the restriction, and make the customer available to other companies, then clear the field. |
|
Review: |
|
@OSguard Ok, my bad. I was sure they were shared.
What do you mean?
Yes it does change existing installations, for the users who are used to share their customers accross their companies. We have at least one who does (I guess we added a _default with False for them). The ones who want to share the partners will still be able to add a custom mapping. Ok for me as soon as you add the same in BaseAddressImportMapper. |
| storeview = self.session.browse('magento.storeview', | ||
| binding_id) | ||
| if storeview.store_id: | ||
| return {'company_id': storeview.store_id.company_id.id} |
There was a problem hiding this comment.
Maybe leave it to False if we do not pass one of the two if? Or raise an error.
It should not happen, but if it had to happen, it would have the company of the user that runs the jobs and it could mess around.
There was a problem hiding this comment.
Not sure I follow, if it does not pass either 'if', then it would be as if the code was not there at all and everything stays as it was (setting it according the running user). Or are you suggesting returning {'company_id': False} in that case?
There was a problem hiding this comment.
and everything stays as it was (setting it according the running user)
Yes. If it does not pass either 'if', this is an exceptional situation, and in that case, assigning the company of the running user would be worse (example: the company of the connector user is "the holding", the regular users all connects to the children companies, no one will be able to see this customer), IMO, than setting no company at all.
Or are you suggesting returning {'company_id': False} in that case?
Yes.
Or we could also claim that it should never happen and we raise a MappingError
The job has a user in which it will be ececute. So another solution is for each store to run the import as a user which belongs to the right company to import. Can this work? |
|
@OSguard this is exactly what I propose in #52 (comment), #52 (comment) For a multi-company implementations, this is really what should be done because a lot of things may come from the user (properties, default values). |
|
Is it possible without code changes? |
|
@OSguard I don't think so. Because the cron methods are on the backend so all the websites and stores of a backend will have the same user. |
|
@mistotebe is that a solution that fits your needs as well? |
|
@OSguard, yes that would work as well, and although I could provide a patch for the cron, I'm not sure I can provide a changeset that handles all the necessary res.group entries, tooling and documentation to add a user that would be useable for everything to work reliably. |
|
Given that some have already voiced their sentiment that they expect a company to be False in some cases and the fact that I do not feel up to the task of actually implementing all that's needed to get the 'one user per company' solution[0] properly working, I have tried addressing @guewen's concern and nothing else here. Worst case this approach can be rejected and a proper solution (which would already affect/fix a whole range of other issues) tracked in a separate ticket. [0] also note that there is currently no link between a magento_website and a store(view) in the connector (not sure if there is one in Magento), so picking the right user is not possible at the moment either |
| @only_create | ||
| @mapping | ||
| def company_id(self, record): | ||
| import ipdb; ipdb.set_trace() |
|
I agree if we merge this correction which seems already an improvement, and does not preclude to implement a "true" multi-company later. (but the mapping still miss on the address)
There is a link from the storeview (n) → (1) store (n) → (1) website (n) → (1) backend. |
mistotebe
force-pushed
the
7.0-partner-import-company_id
branch
from
8a87c7e
to
ac5e0c2
Compare
1 similar comment
|
Thanks for spotting the ipdb statement, seems connector-magento clone was missing a pre-commit hook to remind me... Ah there it is, but then the company link might be ambiguous anyway, but that's off-topic here I guess. |
mistotebe
force-pushed
the
7.0-partner-import-company_id
branch
from
6ba6569
to
5fad554
Compare
| else: | ||
| return {'company_id': False} | ||
| # Don't return anything, we are merging into an existing partner | ||
| return |
|
LGTM Thanks! 👍 |
|
I guess this needs another reviewer's input? |
|
LGTM 👍 |
|
👍 |
Set the proper company_id on an imported partner
When a partner is being imported, the company id is not set, which leaves the choice to the system. While there is another solution proposed in #52, this makes things work until someone has the time to fix multicompany issues properly.