diff --git a/codecov.yml b/codecov.yml index 4f6dfa59..33c4dcb4 100644 --- a/codecov.yml +++ b/codecov.yml @@ -1,2 +1,3 @@ ignore: - - "**/test_helper.py" \ No newline at end of file + - "**/test_helper.py" + - "tests/**" \ No newline at end of file diff --git a/lms/demo/demo_data.py b/lms/demo/demo_data.py index 2677495f..32456a89 100644 --- a/lms/demo/demo_data.py +++ b/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") diff --git a/lms/lms/course_import_export.py b/lms/lms/course_import_export.py index 72aaa65f..903e9a1a 100644 --- a/lms/lms/course_import_export.py +++ b/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")) diff --git a/lms/lms/test_api.py b/lms/lms/test_api.py index 0870429e..e5428437 100644 --- a/lms/lms/test_api.py +++ b/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") diff --git a/lms/lms/test_utils.py b/lms/lms/test_utils.py index 827a7216..7323c773 100644 --- a/lms/lms/test_utils.py +++ b/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)) diff --git a/lms/lms/utils.py b/lms/lms/utils.py index 8ec2c6b5..52469839 100644 --- a/lms/lms/utils.py +++ b/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