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

Fix: On woocommerce test ping issued during a webhook creation, retur… #36705

Merged
merged 1 commit into from
Aug 26, 2023

Conversation

labeneator
Copy link
Contributor

This PR fixes 2 issues:

  1. During the configuration of a woocommerce webhook. An unsigned test ping is issued to erpnext. Erpnext throws a 417 or 500 because of this non JSON payload. This has a detrimental effect of disabling the webhook if the test ping returns a status code > 500 5 times.
  1. During a new order webhook. Next ERP will attempt to rename the customer if the customer already exists in the system. If the old & new details are the same, an exception is thrown. This PR adds a check to ignore a rename if old == new.

PR resolves #33708

References

Issue 1 tcpdump prior to fix


POST /api/method/erpnext.erpnext_integrations.connectors.woocommerce_connection.order HTTP/1.1
X-Forwarded-For: 35.181.44.209
X-Forwarded-Proto: https
X-Frappe-Site-Name: erpnext.testsite.com
Host: erpnext.testsite.com
X-Use-X-Accel-Redirect: True
Connection: close
Content-Length: 12
User-Agent: WooCommerce/8.0.2 Hookshot (WordPress/6.3)
Accept: */*
Accept-Encoding: deflate, gzip, br, zstd
Content-Type: application/x-www-form-urlencoded

webhook_id=2


HTTP/1.1 417 EXPECTATION FAILED
Server: gunicorn
Date: Thu, 17 Aug 2023 15:05:46 GMT
Connection: close
Content-Type: application/json
Content-Length: 1861
Set-Cookie: sid=Guest; Expires=Sun, 20 Aug 2023 18:05:46 GMT; Secure; HttpOnly; Path=/; SameSite=Lax
Set-Cookie: system_user=no; Secure; Path=/; SameSite=Lax
Set-Cookie: full_name=Guest; Secure; Path=/; SameSite=Lax
Set-Cookie: user_id=Guest; Secure; Path=/; SameSite=Lax
Set-Cookie: user_image=; Secure; Path=/; SameSite=Lax

…n a 200 instead of 417. If customer details haven't changed do not rename the customer
@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Aug 18, 2023
@codecov
Copy link

codecov bot commented Aug 26, 2023

Codecov Report

Merging #36705 (7b1ee20) into develop (21e6db2) will decrease coverage by 0.01%.
Report is 93 commits behind head on develop.
The diff coverage is 0.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #36705      +/-   ##
===========================================
- Coverage    64.82%   64.82%   -0.01%     
===========================================
  Files          792      792              
  Lines        61817    61820       +3     
===========================================
  Hits         40074    40074              
- Misses       21743    21746       +3     
Files Changed Coverage Δ
..._integrations/connectors/woocommerce_connection.py 0.00% <0.00%> (ø)

@deepeshgarg007 deepeshgarg007 merged commit 3f07747 into frappe:develop Aug 26, 2023
12 of 15 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-tests This PR needs automated unit-tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Woocommerce connector dropping error if the customer already exist
2 participants