fix(shipment): apply code review

2026-05-26 16:34:46 +00:00 · 2020-10-01 01:23:29 +00:00
parent 1c9410e5e8
commit 000494f548
22 changed files with 405 additions and 444 deletions
--- a/erpnext/erpnext_integrations/doctype/letmeship/letmeship.json
+++ b/erpnext/erpnext_integrations/doctype/letmeship/letmeship.json
@@ -24,14 +24,14 @@
  },
  {
   "fieldname": "api_password",
-   "fieldtype": "Data",
+   "fieldtype": "Password",
   "label": "API Password",
   "read_only_depends_on": "eval:doc.enabled == 0"
  }
 ],
 "issingle": 1,
 "links": [],
- "modified": "2020-08-05 16:33:44.548230",
+ "modified": "2020-10-21 10:28:37.607717",
 "modified_by": "Administrator",
 "module": "ERPNext Integrations",
 "name": "LetMeShip",
--- a/erpnext/erpnext_integrations/doctype/letmeship/letmeship.py
+++ b/erpnext/erpnext_integrations/doctype/letmeship/letmeship.py
@@ -20,9 +20,7 @@ def get_letmeship_available_services(delivery_to_type, pickup_address,
 	delivery_address, shipment_parcel, description_of_content, pickup_date,
 	value_of_goods, pickup_contact=None, delivery_contact=None):
 	# Retrieve rates at LetMeShip from specification stated.
-	enabled = frappe.db.get_single_value('LetMeShip','enabled')
-	api_id = frappe.db.get_single_value('LetMeShip','api_id')
-	api_password = frappe.db.get_single_value('LetMeShip','api_password')
+	api_id, api_password, enabled = frappe.db.get_value('LetMeShip', 'LetMeShip', ['enabled', 'api_password', 'api_id'])
 	if not enabled or not api_id or not api_password:
 		return []

@@ -41,62 +39,16 @@ def get_letmeship_available_services(delivery_to_type, pickup_address,
 		'Accept': 'application/json',
 		'Access-Control-Allow-Origin': 'string'
 	}
-	payload = {'pickupInfo': {
-		'address': {
-			'countryCode': pickup_address.country_code,
-			'zip': pickup_address.pincode,
-			'city': pickup_address.city,
-			'street': pickup_address.address_line1,
-			'addressInfo1': pickup_address.address_line2,
-			'houseNo': '',
-		},
-		'company': pickup_address.address_title,
-		'person': {
-			'title': pickup_contact.title,
-			'firstname': pickup_contact.first_name,
-			'lastname': pickup_contact.last_name
-		},
-		'phone': {
-			'phoneNumber': pickup_contact.phone,
-			'phoneNumberPrefix': pickup_contact.phone_prefix
-		},
-		'email': pickup_contact.email,
-	}, 'deliveryInfo': {
-		'address': {
-			'countryCode': delivery_address.country_code,
-			'zip': delivery_address.pincode,
-			'city': delivery_address.city,
-			'street': delivery_address.address_line1,
-			'addressInfo1': delivery_address.address_line2,
-			'houseNo': '',
-		},
-		'company': delivery_address.address_title,
-		'person': {
-			'title': delivery_contact.title,
-			'firstname': delivery_contact.first_name,
-			'lastname': delivery_contact.last_name
-		},
-		'phone': {
-			'phoneNumber': delivery_contact.phone,
-			'phoneNumberPrefix': delivery_contact.phone_prefix
-		},
-		'email': delivery_contact.email,
-	}, 'shipmentDetails': {
-		'contentDescription': description_of_content,
-		'shipmentType': 'PARCEL',
-		'shipmentSettings': {
-			'saturdayDelivery': False,
-			'ddp': False,
-			'insurance': False,
-			'pickupOrder': False,
-			'pickupTailLift': False,
-			'deliveryTailLift': False,
-			'holidayDelivery': False,
-		},
-		'goodsValue': value_of_goods,
-		'parcelList': parcel_list,
-		'pickupInterval': {'date': pickup_date},
-	}}
+	payload = generate_payload(
+		pickup_address=pickup_address,
+		pickup_contact=pickup_contact,
+		delivery_address=delivery_address,
+		delivery_contact=delivery_contact,
+		description_of_content=description_of_content,
+		value_of_goods=value_of_goods,
+		parcel_list=parcel_list,
+		pickup_date=pickup_date
+	)
 	try:
 		available_services = []
 		response_data = requests.post(
@@ -128,6 +80,7 @@ def get_letmeship_available_services(delivery_to_type, pickup_address,
 					.format(response_data['message'])
 			)
 	except Exception as exc:
+		frappe.log_error(frappe.get_traceback())
 		frappe.msgprint(
 			_('Error occurred while fetching LetMeShip Prices: {0}')
 				.format(str(exc)),
@@ -142,9 +95,7 @@ def create_letmeship_shipment(pickup_address, delivery_address, shipment_parcel,
 	pickup_contact=None, delivery_contact=None):
 	# Create a transaction at LetMeShip
 	# LetMeShip have limit of 30 characters for Company field
-	enabled = frappe.db.get_single_value('LetMeShip','enabled')
-	api_id = frappe.db.get_single_value('LetMeShip','api_id')
-	api_password = frappe.db.get_single_value('LetMeShip','api_password')
+	api_id, api_password, enabled = frappe.db.get_value('LetMeShip', 'LetMeShip', ['enabled', 'api_password', 'api_id'])
 	if not enabled or not api_id or not api_password:
 		return []

@@ -162,6 +113,72 @@ def create_letmeship_shipment(pickup_address, delivery_address, shipment_parcel,
 		'Accept': 'application/json',
 		'Access-Control-Allow-Origin': 'string'
 	}
+	payload = generate_payload(
+		pickup_address=pickup_address,
+		pickup_contact=pickup_contact,
+		delivery_address=delivery_address,
+		delivery_contact=delivery_contact,
+		description_of_content=description_of_content,
+		value_of_goods=value_of_goods,
+		parcel_list=parcel_list,
+		pickup_date=pickup_date,
+		service_info=service_info,
+		tracking_notific_email=tracking_notific_email,
+		shipment_notific_email=shipment_notific_email
+	)
+	try:
+		response_data = requests.post(
+			url=url,
+			auth=(api_id, api_password),
+			headers=headers,
+			data=json.dumps(payload)
+		)
+		response_data = json.loads(response_data.text)
+		if 'shipmentId' in response_data:
+			shipment_amount = response_data['service']['priceInfo']['totalPrice']
+			awb_number = ''
+			url = 'https://api.letmeship.com/v1/shipments/{id}'.format(id=response_data['shipmentId'])
+			tracking_response = requests.get(url, auth=(api_id, api_password),headers=headers)
+			tracking_response_data = json.loads(tracking_response.text)
+			if 'trackingData' in tracking_response_data:
+				for parcel in tracking_response_data['trackingData']['parcelList']:
+					if 'awbNumber' in parcel:
+						awb_number = parcel['awbNumber']
+			return {
+				'service_provider': LETMESHIP_PROVIDER,
+				'shipment_id': response_data['shipmentId'],
+				'carrier': service_info['carrier'],
+				'carrier_service': service_info['service_name'],
+				'shipment_amount': shipment_amount,
+				'awb_number': awb_number,
+			}
+		elif 'message' in response_data:
+			frappe.throw(
+				_('Error occurred while creating Shipment: {0}')
+					.format(response_data['message'])
+			)
+	except Exception as exc:
+		frappe.log_error(frappe.get_traceback())
+		frappe.msgprint(
+			_('Error occurred while creating Shipment: {0}')
+				.format(str(exc)),
+			indicator='orange',
+			alert=True
+		)
+
+def generate_payload(
+	pickup_address,
+	pickup_contact,
+	delivery_address,
+	delivery_contact,
+	description_of_content,
+	value_of_goods,
+	parcel_list,
+	pickup_date,
+	service_info=None,
+	tracking_notific_email=None,
+	shipment_notific_email=None
+):
 	payload = {
 		'pickupInfo': {
 			'address': {
@@ -205,18 +222,6 @@ def create_letmeship_shipment(pickup_address, delivery_address, shipment_parcel,
 			},
 			'email': delivery_contact.email,
 		},
-		'service': {
-			'baseServiceDetails': {
-				'id': service_info['id'],
-				'name': service_info['service_name'],
-				'carrier': service_info['carrier'],
-				'priceInfo': service_info['price_info'],
-			},
-			'supportedExWorkType': [],
-			'messages': [''],
-			'description': '',
-			'serviceInfo': '',
-		},
 		'shipmentDetails': {
 			'contentDescription': description_of_content,
 			'shipmentType': 'PARCEL',
@@ -233,10 +238,24 @@ def create_letmeship_shipment(pickup_address, delivery_address, shipment_parcel,
 			'parcelList': parcel_list,
 			'pickupInterval': {
 				'date': pickup_date
+			}
+		}
+	}
+
+	if service_info:
+		payload['service'] = {
+			'baseServiceDetails': {
+				'id': service_info['id'],
+				'name': service_info['service_name'],
+				'carrier': service_info['carrier'],
+				'priceInfo': service_info['price_info'],
 			},
-			'contentDescription': description_of_content,
-		},
-		'shipmentNotification': {
+			'supportedExWorkType': [],
+			'messages': [''],
+			'description': '',
+			'serviceInfo': '',
+		}
+		payload['shipmentNotification'] = {
 			'trackingNotification': {
 				'deliveryNotification': True,
 				'problemNotification': True,
@@ -247,77 +266,47 @@ def create_letmeship_shipment(pickup_address, delivery_address, shipment_parcel,
 				'notificationText': '',
 				'emails': [ shipment_notific_email ]
 			}
-		},
-		'labelEmail': True,
-	}
+		}
+		payload['labelEmail'] = True
+	return payload
+
+def get_letmeship_label(shipment_id):
 	try:
-		response_data = requests.post(
-			url=url,
-			auth=(api_id, api_password),
-			headers=headers,
-			data=json.dumps(payload)
+		# Retrieve shipment label from LetMeShip
+		api_id = frappe.db.get_single_value('LetMeShip','api_id')
+		api_password = frappe.db.get_single_value('LetMeShip','api_password')
+		headers = {
+			'Content-Type': 'application/json',
+			'Accept': 'application/json',
+			'Access-Control-Allow-Origin': 'string'
+		}
+		url = 'https://api.letmeship.com/v1/shipments/{id}/documents?types=LABEL'\
+			.format(id=shipment_id)
+		shipment_label_response = requests.get(
+			url,
+			auth=(api_id,api_password),
+			headers=headers
 		)
-		response_data = json.loads(response_data.text)
-		if 'shipmentId' in response_data:
-			shipment_amount = response_data['service']['priceInfo']['totalPrice']
-			awb_number = ''
-			url = 'https://api.letmeship.com/v1/shipments/{id}'.format(id=response_data['shipmentId'])
-			tracking_response = requests.get(url, auth=(api_id, api_password),headers=headers)
-			tracking_response_data = json.loads(tracking_response.text)
-			if 'trackingData' in tracking_response_data:
-				for parcel in tracking_response_data['trackingData']['parcelList']:
-					if 'awbNumber' in parcel:
-						awb_number = parcel['awbNumber']
-			return {
-				'service_provider': LETMESHIP_PROVIDER,
-				'shipment_id': response_data['shipmentId'],
-				'carrier': service_info['carrier'],
-				'carrier_service': service_info['service_name'],
-				'shipment_amount': shipment_amount,
-				'awb_number': awb_number,
-			}
-		elif 'message' in response_data:
+		shipment_label_response_data = json.loads(shipment_label_response.text)
+		if 'documents' in shipment_label_response_data:
+			for label in shipment_label_response_data['documents']:
+				if 'data' in label:
+					return json.dumps(label['data'])
+		else:
 			frappe.throw(
-				_('Error occurred while creating Shipment: {0}')
-					.format(response_data['message'])
+				_('Error occurred while printing Shipment: {0}')
+					.format(shipment_label_response_data['message'])
 			)
 	except Exception as exc:
+		frappe.log_error(frappe.get_traceback())
 		frappe.msgprint(
-			_('Error occurred while creating Shipment: {0}')
+			_('Error occurred while printing Shipment: {0}')
 				.format(str(exc)),
 			indicator='orange',
 			alert=True
 		)


-def get_letmeship_label(shipment_id):
-	# Retrieve shipment label from LetMeShip
-	api_id = frappe.db.get_single_value('LetMeShip','api_id')
-	api_password = frappe.db.get_single_value('LetMeShip','api_password')
-	headers = {
-		'Content-Type': 'application/json',
-		'Accept': 'application/json',
-		'Access-Control-Allow-Origin': 'string'
-	}
-	url = 'https://api.letmeship.com/v1/shipments/{id}/documents?types=LABEL'\
-		.format(id=shipment_id)
-	shipment_label_response = requests.get(
-		url,
-		auth=(api_id,api_password),
-		headers=headers
-	)
-	shipment_label_response_data = json.loads(shipment_label_response.text)
-	if 'documents' in shipment_label_response_data:
-		for label in shipment_label_response_data['documents']:
-			if 'data' in label:
-				return json.dumps(label['data'])
-	else:
-		frappe.throw(
-			_('Error occurred while printing Shipment: {0}')
-				.format(shipment_label_response_data['message'])
-		)
-
-
 def get_letmeship_tracking_data(shipment_id):
 	# return letmeship tracking data
 	api_id = frappe.db.get_single_value('LetMeShip','api_id')
@@ -359,6 +348,7 @@ def get_letmeship_tracking_data(shipment_id):
 					.format(tracking_data['message'])
 			)
 	except Exception as exc:
+		frappe.log_error(frappe.get_traceback())
 		frappe.msgprint(
 			_('Error occurred while updating Shipment: {0}')
 				.format(str(exc)),
--- a/erpnext/erpnext_integrations/doctype/packlink/packlink.py
+++ b/erpnext/erpnext_integrations/doctype/packlink/packlink.py
@@ -72,6 +72,7 @@ def get_packlink_available_services(pickup_address, delivery_address, shipment_p

 		return available_services
 	except Exception as exc:
+		frappe.log_error(frappe.get_traceback())
 		frappe.msgprint(
 			_('Error occurred on Packlink: {0}')
 				.format(str(exc)), indicator='orange',
@@ -152,6 +153,7 @@ def create_packlink_shipment(pickup_address, delivery_address, shipment_parcel,
 				'awb_number': '',
 			}
 	except Exception as exc:
+		frappe.log_error(frappe.get_traceback())
 		frappe.msgprint(
 			_('Error occurred while creating Shipment: {0}')
 				.format(str(exc)),
@@ -215,6 +217,7 @@ def get_packlink_tracking_data(shipment_id):
 				'tracking_url': tracking_url
 			}
 	except Exception as exc:
+		frappe.log_error(frappe.get_traceback())
 		frappe.msgprint(_('Error occurred while updating Shipment: {0}').format(
 			str(exc)), indicator='orange', alert=True)
 	return []
--- a/erpnext/erpnext_integrations/doctype/sendcloud/sendcloud.json
+++ b/erpnext/erpnext_integrations/doctype/sendcloud/sendcloud.json
@@ -24,7 +24,7 @@
  },
  {
   "fieldname": "api_secret",
-   "fieldtype": "Data",
+   "fieldtype": "Password",
   "label": "API Secret",
   "read_only_depends_on": "eval:doc.enabled == 0"
  }
@@ -32,7 +32,7 @@
 "index_web_pages_for_search": 1,
 "issingle": 1,
 "links": [],
- "modified": "2020-08-18 09:48:50.836233",
+ "modified": "2020-10-21 10:28:57.710549",
 "modified_by": "Administrator",
 "module": "ERPNext Integrations",
 "name": "SendCloud",
--- a/erpnext/erpnext_integrations/doctype/sendcloud/sendcloud.py
+++ b/erpnext/erpnext_integrations/doctype/sendcloud/sendcloud.py
@@ -16,9 +16,7 @@ class SendCloud(Document):

 def get_sendcloud_available_services(delivery_address, shipment_parcel):
 	# Retrieve rates at SendCloud from specification stated.
-	enabled = frappe.db.get_single_value('SendCloud', 'enabled')
-	api_key = frappe.db.get_single_value('SendCloud', 'api_key')
-	api_secret = frappe.db.get_single_value('SendCloud', 'api_secret')
+	api_key, api_secret, enabled = frappe.db.get_value('SendCloud', 'SendCloud', ['enabled', 'api_key', 'api_secret'])
 	if not enabled or not api_key or not api_secret:
 		return []

@@ -40,6 +38,7 @@ def get_sendcloud_available_services(delivery_address, shipment_parcel):
 					available_services.append(available_service)
 		return available_services
 	except Exception as exc:
+		frappe.log_error(frappe.get_traceback())
 		frappe.msgprint(_('Error occurred on SendCloud: {0}').format(
 			str(exc)), indicator='orange', alert=True)

@@ -53,9 +52,7 @@ def create_sendcloud_shipment(
 	value_of_goods
 ):
 	# Create a transaction at SendCloud
-	enabled = frappe.db.get_single_value('SendCloud', 'enabled')
-	api_key = frappe.db.get_single_value('SendCloud', 'api_key')
-	api_secret = frappe.db.get_single_value('SendCloud', 'api_secret')
+	api_key, api_secret, enabled = frappe.db.get_value('SendCloud', 'SendCloud', ['enabled', 'api_key', 'api_secret'])
 	if not enabled or not api_key or not api_secret:
 		return []

@@ -105,6 +102,7 @@ def create_sendcloud_shipment(
 				'awb_number': awb_number
 			}
 	except Exception as exc:
+		frappe.log_error(frappe.get_traceback())
 		frappe.msgprint(_('Error occurred while creating Shipment: {0}').format(
 			str(exc)), indicator='orange', alert=True)

@@ -130,17 +128,15 @@ def get_sendcloud_tracking_data(shipment_id):
 		api_key = frappe.db.get_single_value('SendCloud', 'api_key')
 		api_secret = frappe.db.get_single_value('SendCloud', 'api_secret')
 		shipment_id_list = shipment_id.split(', ')
-		tracking_url = ''
 		awb_number = []
 		tracking_status = []
 		tracking_status_info = []
+		tracking_urls = []
 		for ship_id in shipment_id_list:
 			tracking_data_response = \
 				requests.get('https://panel.sendcloud.sc/api/v2/parcels/{id}'.format(id=ship_id), auth=(api_key, api_secret))
 			tracking_data = json.loads(tracking_data_response.text)
-			tracking_url_template = \
-				'<a href="{{ tracking_url }}" target="_blank"><b>{{ _("Click here to Track Shipment") }}</b></a><br>'
-			tracking_url += frappe.render_template(tracking_url_template, {'tracking_url': tracking_data['parcel']['tracking_url']})
+			tracking_urls.append(tracking_data['parcel']['tracking_url'])
 			awb_number.append(tracking_data['parcel']['tracking_number'])
 			tracking_status.append(tracking_data['parcel']['status']['message'])
 			tracking_status_info.append(tracking_data['parcel']['status']['message'])
@@ -148,9 +144,10 @@ def get_sendcloud_tracking_data(shipment_id):
 			'awb_number': ', '.join(awb_number),
 			'tracking_status': ', '.join(tracking_status),
 			'tracking_status_info': ', '.join(tracking_status_info),
-			'tracking_url': tracking_url
+			'tracking_url': ', '.join(tracking_urls)
 		}
 	except Exception as exc:
+		frappe.log_error(frappe.get_traceback())
 		frappe.msgprint(_('Error occurred while updating Shipment: {0}').format(
 			str(exc)), indicator='orange', alert=True)

--- a/erpnext/erpnext_integrations/utils.py
+++ b/erpnext/erpnext_integrations/utils.py
@@ -68,6 +68,4 @@ def get_tracking_url(carrier, tracking_number):
 	url_reference = frappe.get_value('Parcel Service', carrier, 'url_reference')
 	if url_reference:
 		tracking_url = frappe.render_template(url_reference, {'tracking_number': tracking_number})
-		tracking_url_template =  '<a href="{{ tracking_url }}" target="_blank"><b>{{ _("Click here to Track Shipment") }}</a></b>'
-		tracking_url = frappe.render_template(tracking_url_template, {'tracking_url': tracking_url})
 	return tracking_url