fix: cleanup and more tests

codecov.yml

@@ -1,2 +1,3 @@
 ignore:
-  - "**/test_helper.py"
+  - "**/test_helper.py"
+  - "tests/**"

lms/demo/demo_data.py

@@ -3,14 +3,32 @@ import json
 import frappe
 
 from lms.lms.doctype.lms_course.lms_course import update_course_statistics
-from lms.lms.utils import get_course_progress
+from lms.lms.utils import create_user, get_course_progress
 
 
 def create_demo_data(args: dict = None):
 	course = create_course()
-	student = create_user("Ashley", "Ippolito", "ash@ipp.com", "/assets/lms/images/student.jpg")
-	student1 = create_user("John", "Doe", "john.doe@example.com", "/assets/lms/images/student1.jpeg")
-	student2 = create_user("Jane", "Smith", "jane.smith@example.com", "/assets/lms/images/student2.jpeg")
+	student = create_user(
+		email="ash@ipp.com",
+		first_name="Ashley",
+		last_name="Ippolito",
+		full_name="Ashley Ippolito",
+		user_image="/assets/lms/images/student.jpg",
+	)
+	student1 = create_user(
+		email="john.doe@example.com",
+		first_name="John",
+		last_name="Doe",
+		full_name="John Doe",
+		user_image="/assets/lms/images/student1.jpeg",
+	)
+	student2 = create_user(
+		email="jane.smith@example.com",
+		first_name="Jane",
+		last_name="Smith",
+		full_name="Jane Smith",
+		user_image="/assets/lms/images/student2.jpeg",
+	)
 	create_chapter(course)
 	create_lessons(course)
 	enroll_student_in_course(student, course)
@@ -93,29 +111,14 @@ def create_instructor():
 		return instructor
 
 	return create_user(
-		"Jannat", "Patel", "jannat@example.com", "/assets/lms/images/instructor.png", ["Moderator"]
+		email="jannat@example.com",
+		first_name="Jannat",
+		last_name="Patel",
+		user_image="/assets/lms/images/instructor.png",
+		roles=["Moderator"],
 	)
 
 
-def create_user(first_name, last_name, email, user_image, roles=None):
-	if roles is None:
-		roles = ["LMS Student"]
-
-	filters = {"first_name": first_name, "last_name": last_name, "email": email}
-	if frappe.db.exists("User", filters):
-		return frappe.get_doc("User", filters)
-
-	user = frappe.new_doc("User")
-	user.first_name = first_name
-	user.last_name = last_name
-	user.user_image = user_image
-	user.email = email
-	user.send_welcome_email = False
-	user.add_roles(*roles)
-	user.save()
-	return user
-
-
 def create_chapter(course):
 	prepare_chapter(course, "Introduction")
 	prepare_chapter(course, "Adding content to your lessons")

lms/lms/course_import_export.py

@@ -13,6 +13,8 @@ from frappe import _
 from frappe.utils import escape_html, validate_email_address
 from frappe.utils.file_manager import is_safe_path
 
+from lms.lms.utils import create_user as create_lms_user
+
 
 def export_course_zip(course_name):
 	course = frappe.get_doc("LMS Course", course_name)
@@ -218,7 +220,7 @@ def write_chapters_json(zip_file, chapters):
 	for chapter in chapters:
 		chapter_data = chapter.as_dict()
 		chapter_json = frappe_json_dumps(chapter_data)
-		safe_name = sanitize_filename(chapter.name)
+		safe_name = sanitize_string(chapter.name)
 		zip_file.writestr(f"chapters/{safe_name}.json", chapter_json)
 
 
@@ -226,25 +228,25 @@ def write_lessons_json(zip_file, lessons):
 	for lesson in lessons:
 		lesson_data = lesson.as_dict()
 		lesson_json = frappe_json_dumps(lesson_data)
-		safe_name = sanitize_filename(lesson.name)
+		safe_name = sanitize_string(lesson.name)
 		zip_file.writestr(f"lessons/{safe_name}.json", lesson_json)
 
 
 def write_assessments_json(zip_file, assessments, questions, test_cases):
 	for question in questions:
 		question_json = frappe_json_dumps(question)
-		safe_name = sanitize_filename(question["name"])
+		safe_name = sanitize_string(question["name"])
 		zip_file.writestr(f"assessments/questions/{safe_name}.json", question_json)
 
 	for test_case in test_cases:
 		test_case_json = frappe_json_dumps(test_case)
-		safe_name = sanitize_filename(test_case["name"])
+		safe_name = sanitize_string(test_case["name"])
 		zip_file.writestr(f"assessments/test_cases/{safe_name}.json", test_case_json)
 
 	for assessment in assessments:
 		assessment_json = frappe_json_dumps(assessment)
 		safe_doctype = assessment["doctype"].lower()
-		safe_name = sanitize_filename(assessment["name"])
+		safe_name = sanitize_string(assessment["name"])
 		zip_file.writestr(f"assessments/{safe_doctype}_{safe_name}.json", assessment_json)
 
 
@@ -257,7 +259,7 @@ def write_assets(zip_file, assets):
 		file_doc = frappe.get_doc("File", {"file_url": asset})
 		file_path = os.path.abspath(file_doc.get_full_path())
 
-		safe_filename = sanitize_filename(os.path.basename(asset))
+		safe_filename = sanitize_string(os.path.basename(asset))
 		zip_file.write(file_path, f"assets/{safe_filename}")
 
 
@@ -283,7 +285,7 @@ def serve_zip(final_path, zip_filename):
 	if not os.path.exists(final_path) or not os.path.isfile(final_path):
 		frappe.throw(_("File not found"))
 
-	safe_filename = sanitize_filename(zip_filename)
+	safe_filename = sanitize_string(zip_filename)
 
 	try:
 		with open(final_path, "rb") as f:
@@ -365,13 +367,52 @@ def create_user_for_instructors(zip_file):
 			create_user(instructor)
 
 
-def sanitize_name_field(value, max_length=50):
+def sanitize_string(
+	value,
+	allow_spaces=True,
+	max_length=None,
+	replacement_char=None,
+	escape_html_content=True,
+	strip_whitespace=True,
+):
+	"""
+	Unified function to sanitize strings for various use cases.
+
+	Args:
+		value: String to sanitize
+		allow_spaces: Whether to allow spaces in the output (True for names, False for filenames)
+		max_length: Maximum length to truncate to (None for no limit)
+		replacement_char: Character to replace invalid chars with (None to remove them)
+		escape_html_content: Whether to escape HTML entities
+		strip_whitespace: Whether to strip leading/trailing whitespace
+
+	Returns:
+		Sanitized string
+	"""
 	if not value:
 		return value
-	value = escape_html(value.strip()[:max_length])
-	name_pattern = re.compile(r"^[a-zA-Z0-9\s\-\.]+$")
-	if not name_pattern.match(value):
-		value = re.sub(r"[^a-zA-Z0-9\s\-\.]", "", value)
+
+	if strip_whitespace:
+		value = value.strip()
+	if max_length:
+		value = value[:max_length]
+
+	if escape_html_content:
+		value = escape_html(value)
+
+	if allow_spaces:
+		invalid_pattern = r"[^a-zA-Z0-9\s\-\.]"
+		valid_pattern = r"^[a-zA-Z0-9\s\-\.]+$"
+	else:
+		invalid_pattern = r"[^a-zA-Z0-9_\-\.]"
+		valid_pattern = r"^[a-zA-Z0-9_\-\.]+$"
+
+	if replacement_char is None:
+		if not re.match(valid_pattern, value):
+			value = re.sub(invalid_pattern, "", value)
+	else:
+		value = re.sub(invalid_pattern, replacement_char, value)
+
 	return value
 
 
@@ -381,9 +422,9 @@ def validate_user_email(user):
 
 
 def get_user_names(user):
-	first_name = sanitize_name_field(user.get("first_name", ""), 50)
-	last_name = sanitize_name_field(user.get("last_name", ""), 50)
-	full_name = sanitize_name_field(user.get("full_name", ""), 100)
+	first_name = sanitize_string(user.get("first_name", ""), max_length=50)
+	last_name = sanitize_string(user.get("last_name", ""), max_length=50)
+	full_name = sanitize_string(user.get("full_name", ""), max_length=100)
 	parts = full_name.split() if full_name else []
 	return (
 		first_name or (parts[0] if parts else "Imported"),
@@ -393,17 +434,18 @@ def get_user_names(user):
 
 
 def create_user(user):
-	validate_user_email(user)
 	first_name, last_name, full_name = get_user_names(user)
-	user_doc = frappe.new_doc("User")
-	user_doc.email = user["email"].strip().lower()
-	user_doc.first_name = first_name
-	user_doc.last_name = last_name
-	user_doc.full_name = full_name or f"{first_name} {last_name}".strip()
-	user_doc.send_welcome_email = False
-	user_doc.user_image = user.get("user_image")
-	user_doc.insert()
-	user_doc.add_roles("Course Creator")
+	user_doc = create_lms_user(
+		email=user["email"],
+		first_name=first_name,
+		last_name=last_name,
+		full_name=full_name,
+		user_image=user.get("user_image"),
+		roles=["Course Creator"],
+		check_existing=False,
+		validate_email=True,
+	)
+	return user_doc
 
 
 def create_evaluator(zip_file):
@@ -723,10 +765,6 @@ def save_course_structure(zip_file, course_doc, chapter_docs):
 	add_lessons_to_chapters(zip_file, course_doc.name, chapter_docs)
 
 
-def sanitize_filename(filename):
-	return re.sub(r"[^a-zA-Z0-9_\-\.]", "_", filename)
-
-
 def validate_zip_file(zip_file_path):
 	if not os.path.exists(zip_file_path) or not zipfile.is_zipfile(zip_file_path):
 		frappe.throw(_("Invalid ZIP file"))

lms/lms/test_api.py

@@ -11,6 +11,7 @@ from lms.lms.api import (
 	get_course_assessment_progress,
 	import_course_from_zip,
 )
+from lms.lms.course_import_export import sanitize_string
 from lms.lms.test_helpers import BaseTestUtils
 
 
@@ -163,3 +164,15 @@ class TestLMSAPI(BaseTestUtils):
 		)
 		if imported_assessment:
 			self.cleanup_items.append((doctype, imported_assessment))
+
+	def test_sanitize_string_filename_behavior(self):
+		result = sanitize_string(
+			"my file@name!.txt", allow_spaces=False, replacement_char="_", escape_html_content=False
+		)
+		self.assertEqual(result, "my_file_name_.txt")
+
+	def test_sanitize_string_name_field_behavior(self):
+		result = sanitize_string(
+			"John#Doe$", allow_spaces=True, max_length=50, replacement_char=None, escape_html_content=True
+		)
+		self.assertEqual(result, "JohnDoe")

lms/lms/test_utils.py

@@ -7,6 +7,7 @@ from frappe.utils import getdate, to_timedelta
 from lms.lms.doctype.lms_certificate.lms_certificate import is_certified
 from lms.lms.test_helpers import BaseTestUtils
 from lms.lms.utils import (
+	create_user,
 	get_average_rating,
 	get_batch_details,
 	get_chapters,
@@ -157,3 +158,24 @@ class TestLMSUtils(BaseTestUtils):
 		self.assertEqual(batch_details.evaluation_end_date, getdate(self.batch.evaluation_end_date))
 		self.assertEqual(len(batch_details.instructors), len(self.batch.instructors))
 		self.assertEqual(len(batch_details.students), 2)
+
+	def test_create_user(self):
+		user = create_user(
+			email="testuser@example.com", first_name="Test", last_name="User", roles=["LMS Student"]
+		)
+		self.assertEqual(user.email, "testuser@example.com")
+		self.assertEqual(user.first_name, "Test")
+		self.assertEqual(user.last_name, "User")
+		self.assertEqual(user.full_name, "Test User")
+		self.assertIn("LMS Student", [role.role for role in user.roles])
+		self.cleanup_items.append(("User", user.name))
+
+	def test_create_user_with_full_name(self):
+		user = create_user(
+			email="fullnameuser@example.com", full_name="John Michael Doe", roles=["Course Creator"]
+		)
+		self.assertEqual(user.first_name, "John")
+		self.assertEqual(user.last_name, "Michael Doe")
+		self.assertEqual(user.full_name, "John Michael Doe")
+		self.assertIn("Course Creator", [role.role for role in user.roles])
+		self.cleanup_items.append(("User", user.name))

lms/lms/utils.py

@@ -23,6 +23,7 @@ from frappe.utils import (
 	nowtime,
 	pretty_date,
 	rounded,
+	validate_email_address,
 )
 from pypika import Case
 from pypika import functions as fn
@@ -86,6 +87,46 @@ def generate_slug(title: str, doctype: str):
 	return slugify(title, used_slugs=slugs)
 
 
+def process_user_names(first_name, last_name, full_name):
+	if not first_name and full_name:
+		name_parts = full_name.split()
+		first_name = name_parts[0] if name_parts else "User"
+		last_name = " ".join(name_parts[1:]) if len(name_parts) > 1 else ""
+
+	if not full_name:
+		full_name = f"{first_name} {last_name or ''}".strip()
+
+	return first_name, last_name or "", full_name
+
+
+def create_user_document(email, first_name, last_name, full_name, user_image=None):
+	user_doc = frappe.new_doc("User")
+	user_doc.email = email
+	user_doc.first_name = first_name
+	user_doc.last_name = last_name
+	user_doc.full_name = full_name
+	user_doc.send_welcome_email = False
+	if user_image:
+		user_doc.user_image = user_image
+	user_doc.insert()
+	return user_doc
+
+
+def create_user(email, first_name=None, last_name=None, full_name=None, user_image=None, roles=None):
+	validate_email_address(email, True)
+	existing_user = frappe.db.exists("User", email)
+	if existing_user:
+		return frappe.get_doc("User", email)
+
+	first_name, last_name, full_name = process_user_names(first_name, last_name, full_name)
+	user_doc = create_user_document(email, first_name, last_name, full_name, user_image)
+
+	if not roles:
+		roles = ["LMS Student"]
+	user_doc.add_roles(*roles)
+	return user_doc
+
+
 def get_membership(course: str, member: str = None):
 	if not member:
 		member = frappe.session.user