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

[12.0][MIG] connector_magento #292

Closed
wants to merge 38 commits into from

Conversation

sebalix
Copy link

@sebalix sebalix commented Apr 19, 2019

PR itself ready to be reviewed, but dependencies are not ready.

Depends on:

oca_dependencies.txt has been updated to include all the dependencies and install the module on Travis.

Documentation generation is stuck because of this bug: odoo/odoo#28125
Don't know what to do about this, excepting disable the api/api_models part of the documentation (need to check if some other parts depend on it).

EDIT: I disabled the documentation for class members using fields as a workaround.

@pedrobaeza pedrobaeza added this to the 12.0 milestone Apr 19, 2019
@pedrobaeza
Copy link
Member

This is missing all the commit history in 10.0, as you are using a different starting branch.

@pedrobaeza
Copy link
Member

Well, not all, but a part. Maybe it was added after the 11.0 PR. Please compare both.

@OCA-git-bot OCA-git-bot mentioned this pull request Apr 19, 2019
1 task
@sebalix
Copy link
Author

sebalix commented Apr 19, 2019

@pedrobaeza indeed, I tried to retrieve all previous commits when the module was still called magentoerpconnect but didn't succeed (doing two git format-patch, one for magentoerpconnect and one for connector_magento). I'll try again.

@pedrobaeza
Copy link
Member

Well, I think it's enough with commits after renaming. I miss 2 o 3 only.

@sebalix sebalix force-pushed the 12.0-mig-connector_magento branch 2 times, most recently from 58b6e63 to 2acd90f Compare April 23, 2019 14:53
guewen and others added 22 commits April 24, 2019 09:25
* Add binding on sales orders
* Ensure that we always have the Connector Manager group on the
  Connector tabs (most are done in the parent views of
  connector_ecommerce)
* Fixed inheritance of the delivery.carrier view
* Use vcr.py to record the exchanges instead of a self-made recorder
* Use new base Components test case classes
* Separate export tests in 2 phases: trigger of the export which check
  that the jobs are delayed, and test the job itself in a second test
I removed it earlier, but it is required when we import products
Unpacks the giftcard code from the PHP serialized information about the giftcards using regex.
Since the value will be a string like '0.0000', the prior tests were insufficient to prevent always adding a line.
* Rename it because it was shadowing another test
* Compute in the location's context
@sebalix
Copy link
Author

sebalix commented Oct 17, 2019

@rvalyi I rebased this branch to remove the pending merge on OCA/connector-ecommerce#49, it's ready to review/merge now.

@sebalix sebalix changed the title [12.0][MIG][WIP] connector_magento [12.0][MIG] connector_magento Oct 21, 2019
@JordiBForgeFlow
Copy link
Member

@sebalix hi! Have you evaluated the complexity to support Magento 2?

@sebalix
Copy link
Author

sebalix commented Mar 6, 2020

@JordiBForgeFlow Hi! Not on latest versions of Odoo, but we are currently migrating this connector to Magento 2.2+ for OpenERP 7.0... :) (based on the work already started by @StefanRijnhart on Odoo 8.0 here: #289 ). I should take time to forward port some fixes in his PR BTW.
But nothing complex, a lot of code is reusable as-is, we mainly need to split ConnectorUnit/Component for each version to subclass a common one.

@@ -0,0 +1,163 @@
interactions:
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 file was added unintendedly.

@StefanRijnhart
Copy link
Member

Great work! I've got just the one comment.

@StefanRijnhart
Copy link
Member

@sebalix please throw your fixes in my direction. I'll be starting to port Magento2 compatibility to 12.0 based to this branch if noone else is on it already (@JordiBForgeFlow?)

@StefanRijnhart
Copy link
Member

@sebalix I took the liberty of removing this ':w' file myself and pushing it to your branch because that is the only flaw I could find this PR. Hoping we can merge it soon.

@StefanRijnhart
Copy link
Member

Oh, I see runbot fails. Probably on these warnings:

2020-04-04 08:50:16,654 170 WARNING openerp_test odoo.models: The model magento.config.specializer has no _description
2020-04-04 08:50:16,660 170 WARNING openerp_test odoo.models: The model magento.binding.backend.read has no _description
2020-04-04 08:50:17,948 170 WARNING openerp_test odoo.addons.base.models.ir_model: Two fields (company, company_id) of res.partner() have the same label: Company.
2020-04-04 08:50:17,994 170 WARNING openerp_test odoo.addons.base.models.ir_model: Two fields (company_id, company) of magento.res.partner() have the same label: Company.
2020-04-04 08:50:18,054 170 WARNING openerp_test odoo.addons.base.models.ir_model: Two fields (company_id, company) of magento.address() have the same label: Company.
2020-04-04 08:50:18,148 170 WARNING openerp_test odoo.addons.base.models.ir_model: Two fields (product_variant_id, odoo_id) of magento.product.product() have the same label: Product.
2020-04-04 08:50:18,555 170 WARNING openerp_test odoo.addons.base.models.ir_model: Two fields (company, company_id) of res.users() have the same label: Company.
2020-04-04 08:50:19,167 170 WARNING openerp_test odoo.models: exception.rule.create() with unknown fields: rule_group
2020-04-04 08:50:19,173 170 WARNING openerp_test odoo.models: exception.rule.create() with unknown fields: rule_group
2020-04-04 08:50:19,957 170 WARNING openerp_test odoo.addons.base.models.ir_ui_view: The group base.group_multi_salesteams defined in view magento.storeview.form does not exist!

@sebalix
Copy link
Author

sebalix commented Apr 6, 2020

@StefanRijnhart thank you for fixing this branch! Feel free to push on my branch to get it merged, I'm quite busy at the moment so I can't work on it sadly.

@StefanRijnhart
Copy link
Member

Thanks, I'll have a look at the warnings then.

@StefanRijnhart
Copy link
Member

Runbot green! Travis borked on failed nodejs download (and feedback missing in Github?) but previous run was green: https://travis-ci.org/github/OCA/connector-magento/builds/671728251

@StefanRijnhart
Copy link
Member

StefanRijnhart commented Apr 6, 2020

@sebalix on your review I will squash my fixes into your migration commitGuessing I will be prevented from force pushing the branch so please go ahead with squashing my fixes into your commit yourself.

Copy link
Author

@sebalix sebalix left a comment

Choose a reason for hiding this comment

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

@StefanRijnhart I reviewed your commit, LG (but can't approve my own PR), feel free to squash your commits

connector_magento/tests/fixtures/cassettes/:w Outdated Show resolved Hide resolved
@sebalix
Copy link
Author

sebalix commented Apr 8, 2020

@JordiBForgeFlow a bit late sorry, no sadly I didn't see these branches until now.

@StefanRijnhart
Copy link
Member

@sebalix thanks for the review. Can you squash my little commits into your migration commit?

for storeview in lang_storeviews:
lang_record = self._get_magento_data(storeview.external_id)
map_record = mapper.map_record(lang_record)
record = list(map_record.values())
Copy link
Member

Choose a reason for hiding this comment

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

Don't convert this dictionary of values to list. You can tell from line 320 that record needs to stay in dictionary format so that you can fetch its items()

item_qty = {}
# get product and quantities to ship from the picking
for line in binding.move_lines:
sale_line = line.procurement_id.sale_line_id
Copy link
Member

Choose a reason for hiding this comment

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

sale_line = line.sale_line_id

@StefanRijnhart StefanRijnhart self-requested a review April 17, 2020 13:03
@StefanRijnhart StefanRijnhart mentioned this pull request May 8, 2020
@pedrobaeza
Copy link
Member

@sebalix do you accept this to be superseded by #304 ?

@sebalix
Copy link
Author

sebalix commented Jul 2, 2020

@StefanRijnhart @pedrobaeza sorry for the delay, yes of course, I'll close this one.

@sebalix sebalix closed this Jul 2, 2020
@StefanRijnhart
Copy link
Member

Thanks, and thank you for your work on this @sebalix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.