T2240 no money hold extension reminders

[ecino]

No description provided.

[gemini-code-assist]

Summary of Changes

Hello @ecino, I'm Gemini Code Assist¹! 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 introduces targeted improvements to the system's notification mechanisms and document generation. It ensures that 'no money hold' extension reminders are triggered only for the relevant hold type and standardizes the order of images when creating correspondence, leading to more accurate and consistent output.

Highlights

• Refined 'No Money Hold' Notifications: The logic for sending notifications related to 'no money hold' extensions has been updated. Notifications are now explicitly conditional on the hold type being NO_MONEY_HOLD, preventing unintended notifications for other hold types.
• Consistent Image Sorting for Correspondence: Image attachments for generated letters and PDFs are now consistently sorted in reverse order. This change applies to both the letter generation job and the PDF creation process, ensuring a predictable order for images.

🧠 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. ↩

[gemini-code-assist]

Code Review

This pull request aims to limit 'no money hold' extension reminders to only apply to holds of type NO_MONEY_HOLD. The changes in sbc_compassion/models/correspondence_s2b_generator.py to sort images by reverse order seem correct and improve consistency. The main logic change in child_compassion/models/compassion_hold.py correctly implements the desired behavior for notifications. However, it introduces a duplicated conditional check, which impacts code clarity and maintainability. I've left a comment with a suggestion to refactor this for better code quality.

[gemini-code-assist]

This change correctly restricts notifications to NO_MONEY_HOLD types. However, it introduces a duplicated check for hold.child_id.sponsor_id (first at line 479, then again at line 493). This makes the code less efficient and harder to maintain.

Consider refactoring the postpone_no_money_hold method to avoid this duplication. A better structure would be:

if hold.child_id.sponsor_id:  # Check that a sponsorship exists
    # ... (logic to extend the hold)
    hold.write(hold_vals)

    if hold.type == HoldType.NO_MONEY_HOLD.value:
        body = (
            "The no money hold for child {local_id} was expiring on "
            "{old_expiration} and was extended to {new_expiration} "
            "({extension_description} extension).{additional_text}"
        )
        hold.message_post(
            body=_(body.format(**values)),
            subject=_("No money hold extension"),
            subtype_xmlid="mail.mt_comment",
        )
else:
    if hold.type == HoldType.NO_MONEY_HOLD.value:
        body = _(
            "The no money hold for child {local_id} is expiring on "
            "{old_expiration} and will not be extended since "
            "no sponsorship exists for this child."
        )
        hold.message_post(body=body.format(**values))

This would require restructuring the logic starting from line 479.