Skip to content

T2840 add category and description code snippet#2070

Open
avuff wants to merge 12 commits into14.0from
T2840-add-category-and-description-code-snippet
Open

T2840 add category and description code snippet#2070
avuff wants to merge 12 commits into14.0from
T2840-add-category-and-description-code-snippet

Conversation

@avuff
Copy link

@avuff avuff commented Jan 27, 2026

No description provided.

ecino and others added 12 commits October 2, 2025 15:14
- For MyCompassion2, the order of the attachments need to be reversed
* T2624 Add callbacks for pdf generation

* T2288 Force-the-maximum-pages-of-a-letter-to-be-15 (#2051)

* Reapply "Add page nb check"

This reverts commit 8d4d27f.

* Run precommit

* Add args safety to callbacks

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

* Remove redundant error raised

---------

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

* Remove callbacks

* Code improvements

* Fix concurency cohenrence

* Fix error propagation when more than 15 pages

* Run precommit

---------

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: Emanuel Cino <ecino@compassion.ch>
Remove unecessary update_nb_sposorships

Refactor names and run precommit
…-it-should-be-one-is-an-isolated-case' into 14.0-MyCompassion2
* Fix sytax error.

* Refactor and ensure atomiity of transaction

* Fix concurrency problem

* Refactor and add comments

* Remove typos

* Update sbc_compassion/models/correspondence_s2b_generator.py

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

* Update sbc_compassion/models/correspondence_s2b_generator.py

Fix sytax error.

Refactor and ensure atomiity of transaction

Fix concurrency problem

Refactor and add comments

Remove typos

Co-Authored-By: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

* Remove unecessary cursors creation

* Refactor

* Rollback changes

* Define isaolated_write and refactor

* Reorder the DONE status update

* Allow updating the generation status from the back end

---------

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
# Conflicts:
#	sbc_compassion/models/correspondence_s2b_generator.py
#	sponsorship_compassion/models/res_partner.py
* Restirct notif. to only hold of type SUB_CHILD_HOLD

* Run precommit

* Correct wrong type of Hold

* Fix typo
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @avuff, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the communication snippet functionality by introducing categorization and additional descriptive fields, making snippets more organized and informative. Concurrently, it significantly improves the robustness of the PDF generation process by adding detailed status tracking, error handling, and a page limit, along with better temporary file management and personalized sponsor name handling.

Highlights

  • Communication Snippet Categorization: Introduced a new model, CommunicationSnippetCategory, allowing users to organize communication snippets into distinct categories for better management and discoverability.
  • Enhanced Communication Snippet Details: Added a description field and a category_id (Many2one to the new category model) to the CommunicationSnippet model, providing more context and organization for each snippet.
  • UI Updates for Snippets: The user interface for communication snippets has been updated to display the new category_id and description fields in both the form and tree views, improving usability.
  • Robust PDF Generation Status and Error Handling: Implemented generation_status and generation_error_message fields in CorrespondenceS2bGenerator to track the progress and capture errors during PDF generation. The preview and generate_letters_job methods now use an isolated_write to update these statuses, ensuring updates persist even if the main transaction rolls back.
  • PDF Page Limit Enforcement: A MAX_PAGE_COUNT constant has been added to limit the number of pages in generated PDF letters, preventing excessively long documents and raising a UserError if the limit is exceeded during preview.
  • Improved Sponsor Name Formatting: The %firstname% placeholder in correspondence generation now prioritizes the sponsor's preferred_name before falling back to firstname or name, ensuring more personalized communication.
  • Refactored Temporary File Management: The PDF generation process in correspondence_template.py has been refactored to use a temp_files list and a finally block, ensuring all temporary files are properly closed and cleaned up, improving resource management and stability.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new category for communication snippets, adds a description field to communication snippets, and enhances PDF generation for correspondence letters. The changes include modifications to models, views, security access, and PDF generation logic. The review focuses on error handling, code clarity, and potential performance improvements.

},
)
for atchmt in self.image_ids
for atchmt in self.image_ids.sorted(reverse=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Sorting attachments in reverse order might not be the desired behavior. It's important to ensure that the order of attachments is correct for the intended use case. Confirm that reversing the order is indeed what's needed.

(header, ""), # Headers (front/back)
{"Original": [text]}, # Text
self.mapped("image_ids.datas"), # Images
self.mapped("image_ids").sorted(reverse=True).mapped("datas"), # Images
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Sorting the images in reverse order might not be the desired behavior. It's important to ensure that the order of images is correct for the intended use case. Confirm that reversing the order is indeed what's needed.

# ----------------------------------------------------
# NOTIFICATION (Only for NO_MONEY_HOLD)
# ----------------------------------------------------
if hold.type == HoldType.NO_MONEY_HOLD.value:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This if statement checks the hold type again, which seems redundant since the code block is already within the for hold in self.filtered(lambda h: h.no_money_extension < 3) loop and if hold.type == HoldType.NO_MONEY_HOLD.value: block. Consider removing this redundant check to simplify the code.


except Exception as error:
# If the operation fails, notify the user with the error message
self.env.cr.rollback()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Rolling back the database transaction in the except block is crucial for maintaining data integrity. However, it's also important to ensure that the user is properly notified about the failure. Consider adding a more specific error message or logging the exception details for debugging purposes.

Comment on lines +305 to +306
"%firstname%": sponsor.preferred_name or sponsor.firstname or sponsor.name,
"%lastname%": sponsor.lastname or "",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider using sponsor.name as a fallback only if sponsor.preferred_name and sponsor.firstname are empty to ensure a more robust solution.

            "%age%": str(child.age),
            "%firstname%": sponsor.preferred_name or sponsor.firstname or sponsor.name,
            "%lastname%": sponsor.lastname or "",

Comment on lines +339 to +348
def isolated_write(self, vals):
"""Use a separate transaction to update the letter_generator."""
if len(self) != 1:
return False

with self.env.registry.cursor() as new_cr:
new_env = self.env(cr=new_cr)
new_s2b_generator = new_env[self._name].browse(self.id)
new_s2b_generator.write(vals)
new_cr.commit()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The isolated_write method is used to update the letter generator in a separate transaction. It's important to ensure that this method is robust and handles potential errors gracefully. Consider adding error handling to catch exceptions that might occur during the write operation or commit.

Comment on lines +277 to +279
except Exception:
_logger.error("Cannot read PDF made by FPDF.", exc_info=True)
return False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The except Exception block is too broad and might catch unexpected errors. Consider catching specific exceptions like FileNotFoundError or IOError to handle known error scenarios and re-raise any other exceptions.

Comment on lines +282 to +307
# Clean up all temporary files (they will be auto-deleted when closed)
for temp_file in temp_files:
try:
temp_file.close()
except Exception as e:
_logger.warning("Failed to close temp file: %s", str(e))

# Clean up stderr file
if std_err_file:
try:
std_err_file.close()
except Exception:
pass
try:
std_err_file_path = self.path_to("stderr.txt")
if os.path.exists(std_err_file_path):
os.remove(std_err_file_path)
except Exception as e:
_logger.warning("Failed to clean up stderr file: %s", str(e))

# Clean up PDF file if still open
if pdf_file:
try:
pdf_file.close()
except Exception:
pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The finally block attempts to clean up temporary files. It's important to ensure that this cleanup is always performed, even if exceptions occur. Consider adding more specific error handling within the finally block to catch and log any exceptions that might occur during the cleanup process.

icon="fa-search-plus"
class="oe_stat_button"
states="draft"
context="{'raise_error': True}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Adding the raise_error context key to the preview button will raise a UserError if there is an exception. This can be useful for debugging purposes, but it might not be the desired behavior in a production environment. Consider removing this context key or making it configurable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants