Improve CI reliability, attachment data handling, and safe string utilities by zeel-codder · Pull Request #41 · rtCamp/frappe-gmail-thread

zeel-codder · 2026-01-07T12:41:17Z

Description

This pull request introduces several improvements and fixes across CI/CD workflows and the email handling logic. The main themes are workflow reliability, attachment data accuracy, and safer string handling.

CI/CD Workflow Improvements:

Changed the runs-on setting from self-hosted to ubuntu-latest in both .github/workflows/build-test.yml and .github/workflows/linters.yml to improve reliability and portability of CI jobs. [1] [2]
Added explicit permissions for contents: read in .github/workflows/build-test.yml to ensure proper access control for workflows.

Email Attachment Handling:

Introduced the get_attachments_data function in frappe_gmail_thread/api/activity.py to fetch the latest file_url for each attachment from the database, ensuring attachment links are always up-to-date.
Updated the get_linked_gmail_threads function to use the new get_attachments_data method for more reliable attachment information.
Removed the direct inclusion of file_url in the attachments data within process_attachments to avoid stale links and rely on dynamic retrieval instead.

String Handling Improvements:

Refined the safe_str function in frappe_gmail_thread/utils/helpers.py to handle None, bytes, and string types more robustly, preventing encoding issues when saving email data.

Pre-commit Configuration:

Changed the default_stages in .pre-commit-config.yaml from [commit] to [pre-commit] for more standard pre-commit hook behavior.

Relevant Technical Choices

Testing Instructions

Additional Information:

Screenshot/Screencast

Checklist

I have carefully reviewed the code before submitting it for review.
This code is adequately covered by unit tests to validate its functionality.
I have conducted thorough testing to ensure it functions as intended.
A member of the QA team has reviewed and tested this PR (To be checked by QA or code reviewer)

Fixes #

…achments_data function

…ncoding and decoding

- Change "runs-on" to "ubuntu-latest" in build-test.yml - Change "runs-on" to "ubuntu-latest" in linters.yml Signed-off-by: xieyt <alokmilenium@gmail.com>

…n permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

ci: Update runner type for workflows

…fix/s3-attachments

Enhance attachment handling by retrieving latest file URLs in get_attachments_data function

…fix/surrogate_escape

Fix surrogate handling in safe_str function to ensure proper string encoding and decoding

Copilot

Pull request overview

This pull request improves CI/CD reliability, email attachment handling, and string processing utilities. The changes migrate workflows from self-hosted runners to GitHub-hosted ubuntu-latest runners, introduce dynamic attachment URL retrieval to prevent stale links, and refine string encoding/decoding logic.

Key Changes:

Migrated CI/CD workflows from self-hosted to ubuntu-latest runners with explicit contents: read permissions for better reliability
Implemented dynamic attachment file_url retrieval from the database instead of storing URLs directly in attachment data
Enhanced safe_str function to handle None, bytes, and string types more explicitly

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`.github/workflows/build-test.yml`	Updated runner from `self-hosted` to `ubuntu-latest` and added explicit `contents: read` permissions
`.github/workflows/linters.yml`	Changed runner from `self-hosted` to `ubuntu-latest` for improved portability
`.pre-commit-config.yaml`	Corrected `default_stages` from non-standard `[commit]` to standard `[pre-commit]`
`frappe_gmail_thread/api/activity.py`	Added `get_attachments_data` function to fetch current `file_url` values dynamically from the database
`frappe_gmail_thread/utils/helpers.py`	Removed `file_url` from stored attachment data and refined `safe_str` function with explicit type handling

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-07T12:44:06Z

frappe_gmail_thread/api/activity.py



+def get_attachments_data(email):
+    attachments_data = json.loads(email.attachments_data)


The get_attachments_data function doesn't handle the case where email.attachments_data is None or an empty string. If an email has no attachments, json.loads() will fail with a TypeError or JSONDecodeError. Consider adding a check to return an empty list when attachments_data is falsy.

Suggested change

attachments_data = json.loads(email.attachments_data)

attachments_data_raw = getattr(email, "attachments_data", None)

if not attachments_data_raw:

return []

try:

attachments_data = json.loads(attachments_data_raw)

except (TypeError, json.JSONDecodeError):

return []

Copilot · 2026-01-07T12:44:06Z

frappe_gmail_thread/api/activity.py

+    for attachment in attachments_data:
+        file_doc_name = attachment.get("file_doc_name")
+        if file_doc_name:
+            file_url = frappe.db.get_value("File", file_doc_name, "file_url")
+            attachment["file_url"] = file_url
+    return attachments_data


When a File document is deleted or doesn't exist, frappe.db.get_value() will return None, but the code doesn't handle this case. The attachment will have file_url set to None, which could cause issues in the frontend or when attempting to access the attachment. Consider either filtering out attachments with missing files or providing a fallback value.

Suggested change

for attachment in attachments_data:

file_doc_name = attachment.get("file_doc_name")

if file_doc_name:

file_url = frappe.db.get_value("File", file_doc_name, "file_url")

attachment["file_url"] = file_url

return attachments_data

filtered_attachments = []

for attachment in attachments_data:

file_doc_name = attachment.get("file_doc_name")

if not file_doc_name:

# Skip attachments without a linked File document

continue

file_url = frappe.db.get_value("File", file_doc_name, "file_url")

if not file_url:

# File document is missing or has no URL; skip this attachment

continue

attachment["file_url"] = file_url

filtered_attachments.append(attachment)

return filtered_attachments

Copilot · 2026-01-07T12:44:07Z

frappe_gmail_thread/utils/helpers.py

        if isinstance(val, str):
-            return val.encode("utf-8", "surrogateescape").decode("utf-8", "replace")
-        return val
+            return val.encode("utf-8", errors="replace").decode("utf-8")


The encoding/decoding operation on strings using errors="replace" may introduce replacement characters (�) for valid unicode strings that contain surrogate pairs or certain special characters. The original implementation with "surrogateescape" was specifically designed to handle surrogates properly. The new approach could corrupt valid unicode data by replacing characters unnecessarily.

Copilot · 2026-01-07T12:44:07Z

frappe_gmail_thread/api/activity.py

+    # instead of using file_url from attachments_data, we have to use frappe.get_value to get the latest file_url
+    for attachment in attachments_data:
+        file_doc_name = attachment.get("file_doc_name")
+        if file_doc_name:
+            file_url = frappe.db.get_value("File", file_doc_name, "file_url")
+            attachment["file_url"] = file_url


The function makes individual database queries for each attachment's file_url within a loop. When emails have multiple attachments and multiple emails are being processed in get_linked_gmail_threads, this creates an N+1 query problem. Consider using frappe.db.get_all() or frappe.get_list() to batch fetch all file URLs in a single query for better performance.

Suggested change

# instead of using file_url from attachments_data, we have to use frappe.get_value to get the latest file_url

for attachment in attachments_data:

file_doc_name = attachment.get("file_doc_name")

if file_doc_name:

file_url = frappe.db.get_value("File", file_doc_name, "file_url")

attachment["file_url"] = file_url

# instead of using file_url from attachments_data, we have to use the latest file_url from the File doc

# Collect all file_doc_names first to avoid N+1 queries

file_doc_names = [

attachment.get("file_doc_name")

for attachment in attachments_data

if attachment.get("file_doc_name")

]

file_url_by_name = {}

if file_doc_names:

# Batch fetch all file URLs in a single query

files = frappe.db.get_all(

"File",

filters={"name": ["in", file_doc_names]},

fields=["name", "file_url"],

)

file_url_by_name = {f.name: f.file_url for f in files}

for attachment in attachments_data:

file_doc_name = attachment.get("file_doc_name")

if file_doc_name:

# Use the latest file_url from the File doc; default to None if not found

attachment["file_url"] = file_url_by_name.get(file_doc_name)

shivamsn97 and others added 9 commits December 29, 2025 13:34

Enhance attachment handling by retrieving latest file URLs in get_att…

3d64e78

…achments_data function

Fix surrogate handling in safe_str function to ensure proper string e…

f2ef011

…ncoding and decoding

ci: Update runner type for workflows

66c0176

- Change "runs-on" to "ubuntu-latest" in build-test.yml - Change "runs-on" to "ubuntu-latest" in linters.yml Signed-off-by: xieyt <alokmilenium@gmail.com>

Potential fix for code scanning alert no. 2: Workflow does not contai…

96482c2

…n permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

Merge pull request #40 from rtCamp/fix/ci

e52461b

ci: Update runner type for workflows

Merge branch 'develop' of github.com:rtCamp/frappe-gmail-thread into …

9840f4b

…fix/s3-attachments

Merge pull request #38 from rtCamp/fix/s3-attachments

a324cc4

Enhance attachment handling by retrieving latest file URLs in get_attachments_data function

Merge branch 'develop' of github.com:rtCamp/frappe-gmail-thread into …

5201d3d

…fix/surrogate_escape

Merge pull request #39 from rtCamp/fix/surrogate_escape

6070258

Fix surrogate handling in safe_str function to ensure proper string encoding and decoding

zeel-codder requested review from Copilot and niraj2477 January 7, 2026 12:41

Copilot started reviewing on behalf of zeel-codder January 7, 2026 12:41 View session

niraj2477 approved these changes Jan 7, 2026

View reviewed changes

zeel-codder merged commit e9bf4ff into main Jan 7, 2026
13 checks passed

Copilot AI reviewed Jan 7, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve CI reliability, attachment data handling, and safe string utilities#41

Improve CI reliability, attachment data handling, and safe string utilities#41
zeel-codder merged 9 commits intomainfrom
develop

zeel-codder commented Jan 7, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 7, 2026

Uh oh!

Copilot AI Jan 7, 2026

Uh oh!

Copilot AI Jan 7, 2026

Uh oh!

Copilot AI Jan 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants



		def get_attachments_data(email):
		attachments_data = json.loads(email.attachments_data)

-    attachments_data = json.loads(email.attachments_data)
+    attachments_data_raw = getattr(email, "attachments_data", None)
+    if not attachments_data_raw:
+        return []
+    try:
+        attachments_data = json.loads(attachments_data_raw)
+    except (TypeError, json.JSONDecodeError):
+        return []

-    # instead of using file_url from attachments_data, we have to use frappe.get_value to get the latest file_url
-    for attachment in attachments_data:
-        file_doc_name = attachment.get("file_doc_name")
-        if file_doc_name:
-            file_url = frappe.db.get_value("File", file_doc_name, "file_url")
-            attachment["file_url"] = file_url
+    # instead of using file_url from attachments_data, we have to use the latest file_url from the File doc
+    # Collect all file_doc_names first to avoid N+1 queries
+    file_doc_names = [
+        attachment.get("file_doc_name")
+        for attachment in attachments_data
+        if attachment.get("file_doc_name")
+    ]
+    file_url_by_name = {}
+    if file_doc_names:
+        # Batch fetch all file URLs in a single query
+        files = frappe.db.get_all(
+            "File",
+            filters={"name": ["in", file_doc_names]},
+            fields=["name", "file_url"],
+        )
+        file_url_by_name = {f.name: f.file_url for f in files}
+    for attachment in attachments_data:
+        file_doc_name = attachment.get("file_doc_name")
+        if file_doc_name:
+            # Use the latest file_url from the File doc; default to None if not found
+            attachment["file_url"] = file_url_by_name.get(file_doc_name)

Conversation

zeel-codder commented Jan 7, 2026

Description

Relevant Technical Choices

Testing Instructions

Additional Information:

Screenshot/Screencast

Checklist

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants