refactor: missing validations, code clean-up

erpnext/non_profit/doctype/member/member.py

@@ -55,14 +55,16 @@ class Member(Document):
 	def make_customer_and_link(self):
 		if self.customer:
 			frappe.msgprint(_("A customer is already linked to this Member"))
-		cust = create_customer(frappe._dict({
+
+		customer = create_customer(frappe._dict({
 			'fullname': self.member_name,
-			'email': self.email_id or self.email,
+			'email': self.email_id,
 			'phone': None
 		}))
 
-		self.customer = cust
+		self.customer = customer
 		self.save()
+		frappe.msgprint(_("Customer {0} has been created succesfully.").format(self.customer))
 
 
 def get_or_create_member(user_details):

erpnext/non_profit/doctype/membership/membership.js

@@ -4,16 +4,22 @@
 frappe.ui.form.on('Membership', {
 	setup: function(frm) {
 		frappe.db.get_single_value("Membership Settings", "enable_razorpay").then(val => {
-			if (val) frm.set_df_property('razorpay_details_section', 'hidden', false);
+			if (val) frm.set_df_property("razorpay_details_section", "hidden", false);
 		})
 	},
 
 	refresh: function(frm) {
 		!frm.doc.invoice && frm.add_custom_button("Generate Invoice", () => {
-			frm.call("generate_invoice", {
+			frm.call({
-				save: true
+				doc: frm.doc,
-			}).then(() => {
+				method: "generate_invoice",
+				args: {save: true},
+				freeze: true,
+				freeze_message: __("Creating Membership Invoice"),
+				callback: function(r) {
+					if (r.invoice)
 						frm.reload_doc();
+				}
 			});
 		});
 
@@ -27,6 +33,6 @@ frappe.ui.form.on('Membership', {
 	},
 
 	onload: function(frm) {
-		frm.add_fetch('membership_type', 'amount', 'amount');
+		frm.add_fetch("membership_type", "amount", "amount");
 	}
 });

erpnext/non_profit/doctype/membership/membership.py

@@ -14,16 +14,21 @@ from erpnext.non_profit.doctype.member.member import create_member
 from frappe import _
 import erpnext
 
-
 class Membership(Document):
 	def validate(self):
 		if not self.member or not frappe.db.exists("Member", self.member):
-			member_name = frappe.get_value('Member', dict(email=frappe.session.user))
+			# for web forms
+			self.create_member_from_website_user()
+
+		self.validate_membership_period()
+
+	def create_member_from_website_user(self):
+		member_name = frappe.get_value("Member", dict(email_id=frappe.session.user))
 
 		if not member_name:
-				user = frappe.get_doc('User', frappe.session.user)
+			user = frappe.get_doc("User", frappe.session.user)
 			member = frappe.get_doc(dict(
-					doctype='Member',
+				doctype="Member",
 				email_id=frappe.session.user,
 				membership_type=self.membership_type,
 				member_name=user.get_fullname()
@@ -33,6 +38,7 @@ class Membership(Document):
 		if self.get("__islocal"):
 			self.member = member_name
 
+	def validate_membership_period(self):
 		# get last membership (if active)
 		last_membership = erpnext.get_last_membership(self.member)
 
@@ -40,7 +46,7 @@ class Membership(Document):
 		if last_membership and not frappe.session.user == "Administrator":
 			# if last membership does not expire in 30 days, then do not allow to renew
 			if getdate(add_days(last_membership.to_date, -30)) > getdate(nowdate()) :
-				frappe.throw(_('You can only renew if your membership expires within 30 days'))
+				frappe.throw(_("You can only renew if your membership expires within 30 days"))
 
 			self.from_date = add_days(last_membership.to_date, 1)
 		elif frappe.session.user == "Administrator":
@@ -57,7 +63,7 @@ class Membership(Document):
 		if status_changed_to not in ("Completed", "Authorized"):
 			return
 		self.load_from_db()
-		self.db_set('paid', 1)
+		self.db_set("paid", 1)
 		settings = frappe.get_doc("Membership Settings")
 		if settings.enable_invoicing and settings.create_for_web_forms:
 			self.generate_invoice(with_payment_entry=settings.make_payment_entry, save=True)
@@ -71,28 +77,45 @@ class Membership(Document):
 			frappe.throw(_("An invoice is already linked to this document"))
 
 		member = frappe.get_doc("Member", self.member)
-		plan = frappe.get_doc("Membership Type", self.membership_type)
-		settings = frappe.get_doc("Membership Settings")
-
 		if not member.customer:
 			frappe.throw(_("No customer linked to member {0}").format(frappe.bold(self.member)))
 
-		if not settings.debit_account:
+		plan = frappe.get_doc("Membership Type", self.membership_type)
-			frappe.throw(_("You need to set <b>Debit Account</b> in Membership Settings"))
+		settings = frappe.get_doc("Membership Settings")
-
+		self.validate_membership_type_and_settings(plan, settings)
-		if not settings.company:
-			frappe.throw(_("You need to set <b>Default Company</b> for invoicing in Membership Settings"))
 
 		invoice = make_invoice(self, member, plan, settings)
 		self.invoice = invoice.name
 
 		if with_payment_entry:
+			self.make_payment_entry(settings, invoice)
+
+		if save:
+			self.save()
+
+		return invoice
+
+	def validate_membership_type_and_settings(self, plan, settings):
+		settings_link = get_link_to_form("Membership Type", self.membership_type)
+
+		if not settings.debit_account:
+			frappe.throw(_("You need to set <b>Debit Account</b> in {0}").format(settings_link))
+
+		if not settings.company:
+			frappe.throw(_("You need to set <b>Default Company</b> for invoicing in {0}").format(settings_link))
+
+		if not plan.linked_item:
+			frappe.throw(_("Please set a Linked Item for the Membership Type {0}").format(
+				get_link_to_form("Membership Type", self.membership_type)))
+
+	def make_payment_entry(self, settings, invoice):
 		if not settings.payment_account:
-				frappe.throw(_("You need to set <b>Payment Account</b> in Membership Settings"))
+			frappe.throw(_("You need to set <b>Payment Account</b> in {0}").format(
+				get_link_to_form("Membership Type", self.membership_type)))
 
 		from erpnext.accounts.doctype.payment_entry.payment_entry import get_payment_entry
 		frappe.flags.ignore_account_permission = True
-			pe = get_payment_entry(dt='Sales Invoice', dn=invoice.name, bank_amount=invoice.grand_total)
+		pe = get_payment_entry(dt="Sales Invoice", dn=invoice.name, bank_amount=invoice.grand_total)
 		frappe.flags.ignore_account_permission=False
 		pe.paid_to = settings.payment_account
 		pe.reference_no = self.name
@@ -100,18 +123,13 @@ class Membership(Document):
 		pe.save(ignore_permissions=True)
 		pe.submit()
 
-		if save:
-			self.save()
-
-		return invoice
-
 	def send_acknowlement(self):
 		settings = frappe.get_doc("Membership Settings")
 		if not settings.send_email:
-			frappe.throw(_("You need to enable <b>Send Acknowledge Email</b> in Membership Settings"))
+			frappe.throw(_("You need to enable <b>Send Acknowledge Email</b> in {0}").format(
+				get_link_to_form("Membership Settings", "Membership Settings")))
 
 		member = frappe.get_doc("Member", self.member)
-
 		if not member.email_id:
 			frappe.throw(_("Email address of member {0} is missing").format(frappe.utils.get_link_to_form("Member", self.member)))
 
@@ -135,50 +153,56 @@ class Membership(Document):
 		}
 
 		if not frappe.flags.in_test:
-			frappe.enqueue(method=frappe.sendmail, queue='short', timeout=300, is_async=True, **email_args)
+			frappe.enqueue(method=frappe.sendmail, queue="short", timeout=300, is_async=True, **email_args)
 		else:
 			frappe.sendmail(**email_args)
 
 	def generate_and_send_invoice(self):
-		invoice = self.generate_invoice(save=False)
+		self.generate_invoice(save=False)
 		self.send_acknowlement()
 
+
 def make_invoice(membership, member, plan, settings):
 	invoice = frappe.get_doc({
-		'doctype': 'Sales Invoice',
+		"doctype": "Sales Invoice",
-		'customer': member.customer,
+		"customer": member.customer,
-		'debit_to': settings.debit_account,
+		"debit_to": settings.debit_account,
-		'currency': membership.currency,
+		"currency": membership.currency,
-		'is_pos': 0,
+		"company": settings.company,
-		'items': [
+		"is_pos": 0,
+		"items": [
 			{
-				'item_code': plan.linked_item,
+				"item_code": plan.linked_item,
-				'rate': membership.amount,
+				"rate": membership.amount,
-				'qty': 1
+				"qty": 1
 			}
 		]
 	})
-
+	invoice.set_missing_values()
 	invoice.insert(ignore_permissions=True)
 	invoice.submit()
 
+	frappe.msgprint(_("Sales Invoice created successfully"))
+
 	return invoice
 
+
 def get_member_based_on_subscription(subscription_id, email):
 	members = frappe.get_all("Member", filters={
-					'subscription_id': subscription_id,
+					"subscription_id": subscription_id,
-					'email_id': email
+					"email_id": email
 				}, order_by="creation desc")
 
 	try:
-		return frappe.get_doc("Member", members[0]['name'])
+		return frappe.get_doc("Member", members[0]["name"])
 	except:
 		return None
 
+
 def verify_signature(data):
 	if frappe.flags.in_test:
 		return True
-	signature = frappe.request.headers.get('X-Razorpay-Signature')
+	signature = frappe.request.headers.get("X-Razorpay-Signature")
 
 	settings = frappe.get_doc("Membership Settings")
 	key = settings.get_webhook_secret()
@@ -187,6 +211,7 @@ def verify_signature(data):
 
 	controller.verify_signature(data, signature, key)
 
+
 @frappe.whitelist(allow_guest=True)
 def trigger_razorpay_subscription(*args, **kwargs):
 	data = frappe.request.get_data(as_text=True)
@@ -195,16 +220,16 @@ def trigger_razorpay_subscription(*args, **kwargs):
 	except Exception as e:
 		log = frappe.log_error(e, "Webhook Verification Error")
 		notify_failure(log)
-		return { 'status': 'Failed', 'reason': e}
+		return { "status": "Failed", "reason": e}
 
 	if isinstance(data, six.string_types):
 		data = json.loads(data)
 	data = frappe._dict(data)
 
-	subscription = data.payload.get("subscription", {}).get('entity', {})
+	subscription = data.payload.get("subscription", {}).get("entity", {})
 	subscription = frappe._dict(subscription)
 
-	payment = data.payload.get("payment", {}).get('entity', {})
+	payment = data.payload.get("payment", {}).get("entity", {})
 	payment = frappe._dict(payment)
 
 	try:
@@ -214,15 +239,15 @@ def trigger_razorpay_subscription(*args, **kwargs):
 		member = get_member_based_on_subscription(subscription.id, payment.email)
 		if not member:
 			member = create_member(frappe._dict({
-				'fullname': payment.email,
+				"fullname": payment.email,
-				'email': payment.email,
+				"email": payment.email,
-				'plan_id': get_plan_from_razorpay_id(subscription.plan_id)
+				"plan_id": get_plan_from_razorpay_id(subscription.plan_id)
 			}))
 
 			member.subscription_id = subscription.id
 			member.customer_id = payment.customer_id
 			if subscription.notes and type(subscription.notes) == dict:
-				notes = '\n'.join("{}: {}".format(k, v) for k, v in subscription.notes.items())
+				notes = "\n".join("{}: {}".format(k, v) for k, v in subscription.notes.items())
 				member.add_comment("Comment", notes)
 			elif subscription.notes and type(subscription.notes) == str:
 				member.add_comment("Comment", subscription.notes)
@@ -252,28 +277,30 @@ def trigger_razorpay_subscription(*args, **kwargs):
 		message = "{0}\n\n{1}\n\n{2}: {3}".format(e, frappe.get_traceback(), __("Payment ID"), payment.id)
 		log = frappe.log_error(message, _("Error creating membership entry for {0}").format(member.name))
 		notify_failure(log)
-		return { 'status': 'Failed', 'reason': e}
+		return { "status": "Failed", "reason": e}
 
-	return { 'status': 'Success' }
+	return { "status": "Success" }
 
 
 def notify_failure(log):
 	try:
-		content = """Dear System Manager,
+		content = _("""
-Razorpay webhook for creating renewing membership subscription failed due to some reason. Please check the following error log linked below
+			Dear System Manager,
-
+			Razorpay webhook for creating renewing membership subscription failed due to some reason.
+			Please check the following error log linked below
 			Error Log: {0}
+			Regards, Administrator
+		""").format(get_link_to_form("Error Log", log.name))
 
-Regards,
-Administrator""".format(get_link_to_form("Error Log", log.name))
 		sendmail_to_system_managers("[Important] [ERPNext] Razorpay membership webhook failed , please check.", content)
 	except:
 		pass
 
+
 def get_plan_from_razorpay_id(plan_id):
-	plan = frappe.get_all("Membership Type", filters={'razorpay_plan_id': plan_id}, order_by="creation desc")
+	plan = frappe.get_all("Membership Type", filters={"razorpay_plan_id": plan_id}, order_by="creation desc")
 
 	try:
-		return plan[0]['name']
+		return plan[0]["name"]
 	except:
 		return None

erpnext/non_profit/doctype/membership_settings/membership_settings.js

@@ -11,7 +11,7 @@ frappe.ui.form.on("Membership Settings", {
 			});
 		}
 
-		frm.set_query('inv_print_format', function(doc) {
+		frm.set_query("inv_print_format", function() {
 			return {
 				filters: {
 					"doc_type": "Sales Invoice"
@@ -19,7 +19,7 @@ frappe.ui.form.on("Membership Settings", {
 			};
 		});
 
-		frm.set_query('membership_print_format', function(doc) {
+		frm.set_query("membership_print_format", function() {
 			return {
 				filters: {
 					"doc_type": "Membership"
@@ -27,12 +27,23 @@ frappe.ui.form.on("Membership Settings", {
 			};
 		});
 
-		frm.set_query('debit_account', function(doc) {
+		frm.set_query("debit_account", function() {
 			return {
 				filters: {
-					'account_type': 'Receivable',
+					"account_type": "Receivable",
-					'is_group': 0,
+					"is_group": 0,
-					'company': frm.doc.company
+					"company": frm.doc.company
+				}
+			};
+		});
+
+		frm.set_query("payment_account", function () {
+			var account_types = ["Bank", "Cash"];
+			return {
+				filters: {
+					"account_type": ["in", account_types],
+					"is_group": 0,
+					"company": frm.doc.company
 				}
 			};
 		});