[10.0] migrate connector_magento #255

guewen · 2017-06-16T08:14:36Z

Depends on:

[10.0] Reworked components registry connector#231 (merged)
[10.0] Add Components for connector-ecommerce connector-ecommerce#41 (merged)
[10.0] add new module to manage technical user on company server-tools#858 (merged)
main API migration for magentoerpconnect
update tests
update docs

pedrobaeza · 2017-06-16T08:19:54Z

There are conflicts right now.

Shouldn't this be better renamed to connector-magento?

guewen · 2017-06-16T09:40:33Z

There are conflicts right now.

Yep, I'll fix them, but my focus is elsewhere at this moment, will do it soon.

Shouldn't this be better renamed to connector-magento?

It would be nice, and the right moment to do it, anyway addons that depend on it need changes.

Because it does not apply for Magento 1.9 Closes OCA#126

It reduces the risk of incompatibility between modules if both replace the product mapper.

So we don't replace the entire sale order mapper which has more chances to provoke conflicts between modules.

… new line character to the returned string. This character is not allowed in in the httprequest header. Use base64.b64encode to avoid trailing new line character in the encoded string

It uses the new OCA/connector#138, OCA/connector#139 feature.

… now possible to the analytic account and the fiscal position to use when creating a sale order from magento. Theses options can be specified at any level of the hierarchy of the backend models according to the needs. In this way it become possible for exemple to specify a fiscal position to use for a specific store and take advantage of the taxes mapping defined for this specific position to map the right taxe on the sale order line

So it doesn't fail when it receives a MagicMock instead of a str

sbidoul · 2017-09-13T15:58:04Z

@guewen Except the one failing test, do you think this PR is good enough for merging? That would facilitate further contributions.

guewen · 2017-09-15T08:31:12Z

@guewen Except the one failing test, do you think this PR is good enough for merging? That would facilitate further contributions.

Yes it is and indeed it would make contributions a lot easier, I have to find some time to fix the test.

guewen · 2017-09-15T08:33:29Z

Every time I import an order, a new 'archived' partner is created for both the partner_invoice_id and partner_shipping_id. The orders do use an existing partner for the partner_id field, but not for the other two.

The newly created 'archived' partners do not have any bindings, but are children of the main partner_id which does have bindings.

This is normal: on its side, Magento creates a new address for every order (and the API doesn't return us ids for them), so we have to replicate the same behavior. The addresses are archived so you can use them on the sales orders and related document but not in the searches/lists. This is the difference between the "address book" (the not archived addresses) and the "sales addresses".

jaredkipe · 2017-09-17T15:43:02Z

This is normal: on its side, Magento creates a new address for every order (and the API doesn't return us ids for them), so we have to replicate the same behavior. The addresses are archived so you can use them on the sales orders and related document but not in the searches/lists. This is the difference between the "address book" (the not archived addresses) and the "sales addresses".

Makes sense.

Regarding the failing test case. Perhaps I am not reading it correctly, but I can't find anywhere where it would appear that you are only looking for the stock on the location given by ref("stock.stock_location_components") (5.0 qty).

It would seem that ref("stock.stock_location_components") is inside WH (WH/Stock/Shelf 1), so I would expect its inventory to contribute to WH's inventory (and that is the warehouse assigned to the backend in MagentoTestCase.setUp().

If you want the test to pass, you can put this line after line 192 (my_location_id = ...) binding = binding.with_context(location=my_location_id)

That makes the test pass, but only because of the implementation of the implementation. I think changing line 208 to self.assertEqual(binding.magento_qty, 35.0) is correct.

All this said, I'd rather get to select a field (probably optional) for a location, falling back to the WH location if not set. This would allow you to combine or filter stock going out based on views or physical locations. (e.g. if you have more than one warehouse you can send the stock location 'Physical Locations' to send all of them combined)

lasley · 2017-09-20T22:22:07Z

connector_magento/components/exporter.py

        if not sync:
            return True
-        record = self.backend_adapter.read(self.magento_id,
+        record = self.backend_adapter.read(self.external_id,
                                           attributes=['updated_at'])
        if not record['updated_at']:
            # in rare case it can be empty, in doubt, import it
            return False


Shouldn't this be True?

Seems so... according to the comment I'd say it should be True.

guewen · 2017-09-21T19:05:00Z

Thanks @jaredkipe, you're helping me a lot! It turns out this test has been added here guewen#6 so @Cedric-Pigeon might have his say on your proposal for the location field. I'll start by fixing the test so we can move forward :)

* Rename it because it was shadowing another test * Compute in the location's context

guewen · 2017-09-21T19:16:49Z

The last commit should fix the test. I took the approach to change the location in the context of the binding, because it seems the test tries to check that we are exporting the value for "this" location. @Cedric-Pigeon your approval would be great :)

Thanks again for the help @jaredkipe

jaredkipe · 2017-09-23T15:00:25Z

Thanks @jaredkipe, you're helping me a lot! It turns out this test has been added here guewen#6 so @Cedric-Pigeon might have his say on your proposal for the location field. I'll start by fixing the test so we can move forward :)

@guewen You are very welcome, thank you for your fantastic work on all of these projects!

In my 'customizations' I actually added a location field to use like this. This allowed me to 'group' several warehouses by injecting an intermediate 'view' location. This isn't ideal, I'd prefer to specify multiple locations or warehouses and put those into the context. This should be possible as stock.models.product.Product._get_domain_locations() will use a list of location ids or a list of warehouse ids. I'll get there eventually, I just have larger things to do.

jaredkipe · 2017-09-23T15:21:55Z

@guewen I'm still being plagued by the imports eventually freezing up. When I go to import 3k products, eventually the process will stop without explicitly failing.

Sometimes they fail with a result 'Could not acquire advisory lock'. If this happens I need to restart odoo.

Here is a gist of active Postgres activity when I come back after trying to import stuff overnight. https://gist.github.com/jaredkipe/e81599fe5610e5a7f0eb59334d6529ed

If I find the 'started' jobs, I can requeue them, sometimes that gets them going again.. but only for so long. I'm testing right now with various timeouts for transactions and and idle sessions in Postgres (9.6).

The same problem exists in threaded and worker modes.

guewen · 2017-09-25T06:32:58Z

I'm still being plagued by the imports eventually freezing up. When I go to import 3k products, eventually the process will stop without explicitly failing.

Do you have a magento test database dump to provide or is a private one?

guewen · 2017-09-26T18:23:34Z

Can we merge?

jaredkipe · 2017-09-26T21:11:52Z

Do you have a magento test database dump to provide or is a private one?
It is private. I've found that if I can work the list down more confidently by setting lock_timeout and idle_in_transaction_session_timeout to a couple of minutes, the whole process is much more reliable.

An improvement that could be made to the item import would be to batch the products right after returning from the initial search of product ids as you must wait quite a while between the return (with thousands + product ids) to jobs actually starting to run. (presumably because until the main transaction commits, the queue can't see them).

Bear in mind that the contention I have been dealing with is as it is importing individual products. I have never had a problem with the initial import other than thinking it wasn't working while waiting for it to finish.

Can we merge?

I think so. I think all of the remaining issues I have will have to wait until later as things like the bind by SKU are now integral to my current workflow and I cannot stop to work on them.

jaredkipe · 2017-09-27T01:34:07Z

@guewen I forgot to mention, the dependencies could use updating. Here is the list of modules I needed to install the magento connector and the packages they come from (symlinks)

account_payment_mode -> ../oca/bank-payment/account_payment_mode
account_payment_partner -> ../oca/bank-payment/account_payment_partner
account_payment_sale -> ../oca/bank-payment/account_payment_sale
base_exception -> ../oca-beta/server-tools/base_exception
base_technical_user -> ../oca-beta/server-tools/base_technical_user
component -> ../oca-beta/connector/component
component_event -> ../oca-beta/connector/component_event
connector -> ../oca-beta/connector/connector
connector_base_product -> ../oca-beta/connector/connector_base_product
connector_ecommerce -> ../oca-beta/connector-ecommerce/connector_ecommerce
connector_magento -> ../oca-beta/connector-magento/connector_magento
dbfilter_from_header -> ../oca-server-tools/dbfilter_from_header
partner_gravatar -> ../hibou-gravatar-partner
product_multi_category -> ../oca/product-attribute/product_multi_category
queue_job -> ../oca/queue/queue_job
sale_automatic_workflow -> ../oca/sale-workflow/sale_automatic_workflow
sale_automatic_workflow_payment_mode -> ../oca/sale-workflow/sale_automatic_workflow_payment_mode
sale_exception -> ../oca/sale-workflow/sale_exception

All of those are out of beta now, and if memory serves, the only one I couldn't find a use for was the e-commerce module itself (connector_ecommerce for sure though).

jaredkipe · 2017-09-29T22:59:49Z

I've been looking more into the hangups. Basically the only commonality I'm ever able to see is that the product import binding will be created (logged from _create) but that product is not actually in the database. And then in Postgres there is an 'idle in transaction' with a query of one of the two of these. https://gist.github.com/jaredkipe/1ab32a983e02c9ada64c21e90572f0a6

Okay. After spending more hours working on this. It 'looked' like it was somewhere having to do with downloading the image right after the product is created. It appears that CloudFlare was doing something to make that hang (no timeout?) in _get_binary_image. After hard coding the IP in /etc/hosts in the container suddenly I'm able to do thousands and thousands in the time it would have hung up on a couple.

This also explains why it was so degenerative, it seemed to only get worse and worse. Well I guess because CloudFlare was limiting/throttling it more and more.

guewen · 2017-10-03T15:46:11Z

@jaredkipe thanks for the details, this is very reassuring :)

Could you retarget your pull requests on 10.0 now that's merged? So it'll ease the review.

jaredkipe · 2017-10-03T15:48:49Z

@guewen Will do! (I asked about that between getting the notice that this was merged and getting this comment. :D )

The cache entries for events were including instance of the components, hence, they included the work instance / odoo environment. Only the component class and the events it provides are new cached, the instances of components being instanciated when needed. Reported on: OCA/connector-magento#255 (comment)

guewen added the work in progress label Jun 16, 2017

pedrobaeza mentioned this pull request Jun 16, 2017

Migration to version 10.0 #238

Open

8 tasks

guewen force-pushed the 10.0-components branch from 7463d1b to 8d578f1 Compare June 20, 2017 11:57

guewen and others added 25 commits June 20, 2017 14:03

ConnectorSession in the wrong module

7b8dccd

Be more specific in the notice

7b100c2

Because it does not apply for Magento 1.9 Closes OCA#126

Replace only a price mapper instead of the full product mapper

ff81b89

It reduces the risk of incompatibility between modules if both replace the product mapper.

Use a specialized mapper for the pricelist

0407028

So we don't replace the entire sale order mapper which has more chances to provoke conflicts between modules.

invalid character in header value base64.encodestring adds a trailing…

f5cbc97

… new line character to the returned string. This character is not allowed in in the httprequest header. Use base64.b64encode to avoid trailing new line character in the encoded string

Prevent concurrent import with an advisory lock

5cfb38d

It uses the new OCA/connector#138, OCA/connector#139 feature.

Adjust docs build to use 'dependencies' path

c6cd5a7

add missing ACL to allow warehouse user to deliver an order

587e2a1

Keep advisory lock only if we need to update data into Odoo

ac42236

Fix flake8 warning, operator after line break

cd95e55

Use a MagicMock that supports __getitem__

021db40

Mock fields.Datetime.to_string()

e8b0db7

So it doesn't fail when it receives a MagicMock instead of a str

Add import for virtual and downloadable products as services.

786379f

Set addresses of guests to False as well

42f89ff

Tests

4dd8717

typo on field used in ecommerce to manage giftcode

96d7811

do not create a gift SO line if gift amount is 0

4b96f42

fix 'NoneType' object has no attribute 'split' bug

4d48e59

OCA#129 - Wrap synchonize metadata method and give the user a warning

df9013a

Follow up review

0522a33

Code refactoring to allow override and save image datas

2aa72cc

[IMP] used oca-autopep8

8a1fecb

[FIX] version and dependency

3f00318

[IMP] used simple headers & README.rst updated

2757cac

lasley reviewed Sep 20, 2017

View reviewed changes

Change return value matching with the comment

482d321

Fix failing test

f02affb

* Rename it because it was shadowing another test * Compute in the location's context

guewen merged commit c4e1bef into OCA:10.0 Oct 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[10.0] migrate connector_magento #255

[10.0] migrate connector_magento #255

guewen commented Jun 16, 2017 •

edited

Loading

pedrobaeza commented Jun 16, 2017

guewen commented Jun 16, 2017

sbidoul commented Sep 13, 2017

guewen commented Sep 15, 2017 •

edited

Loading

guewen commented Sep 15, 2017

jaredkipe commented Sep 17, 2017

lasley Sep 20, 2017

guewen Sep 21, 2017

guewen commented Sep 21, 2017

guewen commented Sep 21, 2017

jaredkipe commented Sep 23, 2017

jaredkipe commented Sep 23, 2017

guewen commented Sep 25, 2017

guewen commented Sep 26, 2017

jaredkipe commented Sep 26, 2017

jaredkipe commented Sep 27, 2017

jaredkipe commented Sep 29, 2017 •

edited

Loading

guewen commented Oct 3, 2017

jaredkipe commented Oct 3, 2017

[10.0] migrate connector_magento #255

[10.0] migrate connector_magento #255

Conversation

guewen commented Jun 16, 2017 • edited Loading

pedrobaeza commented Jun 16, 2017

guewen commented Jun 16, 2017

sbidoul commented Sep 13, 2017

guewen commented Sep 15, 2017 • edited Loading

guewen commented Sep 15, 2017

jaredkipe commented Sep 17, 2017

lasley Sep 20, 2017

Choose a reason for hiding this comment

guewen Sep 21, 2017

Choose a reason for hiding this comment

guewen commented Sep 21, 2017

guewen commented Sep 21, 2017

jaredkipe commented Sep 23, 2017

jaredkipe commented Sep 23, 2017

guewen commented Sep 25, 2017

guewen commented Sep 26, 2017

jaredkipe commented Sep 26, 2017

jaredkipe commented Sep 27, 2017

jaredkipe commented Sep 29, 2017 • edited Loading

guewen commented Oct 3, 2017

jaredkipe commented Oct 3, 2017

guewen commented Jun 16, 2017 •

edited

Loading

guewen commented Sep 15, 2017 •

edited

Loading

jaredkipe commented Sep 29, 2017 •

edited

Loading