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

[13.0][MIG] connector_magento: Migration to 13.0 #327

Closed
wants to merge 53 commits into from

Conversation

victoralmau
Copy link
Member

@victoralmau victoralmau commented Aug 25, 2021

Superseed: #317

Migration to 13.0

New features:

  • Add export_magento_qty button to force recompute qty in products.
  • Add import sale order wizard by number in backend.
  • Add transaction related to order.
  • Add test to check import orders in different currencies.

Locked by:

Please @pedrobaeza and @joao-p-marques can you review it?

@Tecnativa TT30700

guewen and others added 30 commits March 9, 2021 10:47
* 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
* Rename it because it was shadowing another test
* Compute in the location's context
The wizard is available on the backend.
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: connector-magento-12.0/connector-magento-12.0-connector_magento
Translate-URL: https://translation.odoo-community.org/projects/connector-magento-12-0/connector-magento-12-0-connector_magento/
@victoralmau victoralmau force-pushed the 13.0-mig-connector_magento branch 3 times, most recently from 9ba05ae to 552364f Compare August 30, 2021 14:08
@victoralmau victoralmau force-pushed the 13.0-mig-connector_magento branch 4 times, most recently from 76ae249 to fca6baa Compare September 3, 2021 15:13
@victoralmau victoralmau marked this pull request as ready for review September 3, 2021 15:25
@victoralmau victoralmau force-pushed the 13.0-mig-connector_magento branch 2 times, most recently from fabd1e7 to b81f49a Compare September 7, 2021 07:04
… paid is added (only if the import of paid orders is used in the payment method and a payment acquirer is defined). If the amount paid is equal to the order total, the transaction will be marked as completed and the order will be auto-confirmed.
@victoralmau
Copy link
Member Author

Now connector_ecommerce addon is already merged.
I have relaunched Travis to verify that the tests work correctly.
If you confirm that everything is OK, it could already be merged, right?

@pedrobaeza pedrobaeza added this to the 13.0 milestone Sep 23, 2021
Copy link
Member

@joao-p-marques joao-p-marques left a comment

Choose a reason for hiding this comment

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

Code review


def _call(self, method, arguments=None, http_method=None, storeview=None):
try:
magento_api = getattr(self.work, "magento_api") # noqa: B009
Copy link
Member

Choose a reason for hiding this comment

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

Don's just silence the linter in this case, as it is easy to fix:

Suggested change
magento_api = getattr(self.work, "magento_api") # noqa: B009
magento_api = self.work["magento_api"]

if not url:
raise ValueError("No admin URL configured on the backend.")
if hasattr(self.model, "_get_admin_path"):
admin_func = getattr(self.model, "_get_admin_path") # noqa: B009
Copy link
Member

Choose a reason for hiding this comment

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

Same here as above:

Suggested change
admin_func = getattr(self.model, "_get_admin_path") # noqa: B009
admin_func = self.model["_get_admin_path"]

create_invoice = magento_store.create_invoice_on

if (
create_invoice == "paid"
Copy link
Member

Choose a reason for hiding this comment

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

I see you compare create_invoice with either "paid" or "open" but, a few lines above, it is asigned the value of create_invoice_on, which would suggest a different sort of value... Can you confirm it is being handled properly and explain a bit more why this change?

direct = [
("email", "email"),
("dob", "birthday"),
# (normalize_datetime('created_at'), 'created_at'),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# (normalize_datetime('created_at'), 'created_at'),

Comment on lines +297 to +299
region = record.get("region")
if isinstance(region, dict):
region = region.get("region")
Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm, this would mean accessing record["region"]["region"]. Is that the expected structure?

}, # noqa
{
"exclude": "0",
# noqa
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# noqa

"types": ["thumbnail"],
"url": "http://localhost:9100/media/catalog/product/i/n/"
"ink-eater-krylon-bombear-destroyed-tee-2.jpg",
}, # noqa
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}, # noqa
},

"types": ["small_image"],
"url": "http://localhost:9100/media/catalog/product/i/n/"
"ink-eater-krylon-bombear-destroyed-tee-1.jpg",
}, # noqa
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}, # noqa
},

"types": [],
"url": "http://localhost:9100/media/catalog/product/m/a/"
"connector_magento_1.png",
}, # noqa
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}, # noqa
},

)

def test_import_sale_order_paid(self):
payment_checkmo = self.env["account.payment.mode"].search(
Copy link
Member

Choose a reason for hiding this comment

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

Same doubts here as above

@pedrobaeza
Copy link
Member

For now, we are holding this pull request, as we are going to speed up the transition to Odoo e-commerce, and not needing this, so o more changes should be applied, although we keep it opened as reference for anyone wanting it.

Comment on lines +854 to +866
def tax_id(self, record):
tax_percent = float(record["tax_percent"] or 0)
if tax_percent > 0:
tax_record = self.env["account.tax"].search(
[
("type_tax_use", "=", "sale"),
("amount_type", "=", "percent"),
("amount", "=", tax_percent),
]
)
if not tax_record:
raise FailedJobError("Missing tax for amount: %s" % tax_percent)
return {"tax_id": [(4, tax_record.id)]}

Choose a reason for hiding this comment

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

We faced an issue with tax mapping as previous version a tax was set from the product settings and then a fiscal position was applied that handles correctly final taxes.

This code tries to take a tax with the same percentage even if the tax on a product is different. Extra, in case there are more than one tax (for different countries) with the same percentage it will fail as code expect to provide 1 tax, not all of them.

My suggestion is to remove this mapping and leave Odoo default behaviour when a tax is set on the SO line.

@john-herholz-dt
Copy link

Hello,

I was also working on getting the connector-magento 12.0 branch running for 15.0 and saw this pull request.
I have not yet contributed to OCA modules, but would like to do so.

At the moment I don't know where to start and who is responsible to check PRs.
I saw a lot of migrations are hanging for months because of missing migrations of other depending modules.

Long story short, who can give me a kickstart on contributing to OCA and how to communicate?

@hailangvn
Copy link

hailangvn commented May 15, 2022

Good day @john-herholz-dt, thanks for your initiation. Hope below steps can help.

  • First document to read is How To section to understand about how to start.
  • Then in case you still want to migrate the module, you should put a comment in tracking of migrations to 15.0 like this comment but with WIP instead of PR number. That is to inform other persons to not spend time on same work.
  • While doing migration, the rest of the guide should be read and followed.
  • After creating PR, you then update the comment with the link to the PR. The early you put the link, the better, not wait until when your migration is done. That will help you receive suggestions.

@elvise
Copy link

elvise commented May 27, 2022

hi @victoralmau do you have any plan to finish this task ?

@pedrobaeza
Copy link
Member

pedrobaeza commented May 27, 2022

We are not finally using it, as we switched to Odoo e-commerce, but feel free to take it from this point that is mostly OK.

@pedrobaeza pedrobaeza closed this May 27, 2022
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.