T2534 payment method storing and management

[SamBachmann]

Summary

Refactors the /my2/donations page to group sponsorships by their payment bundle (Contract Group) and introduces full management capabilities for payment methods (Add, Update, Change) directly from myCompassion2.0.

Key Changes

• Grouped View: Sponsorships are now visually grouped by their backend payment method.

• Payment Management: Added a modal interface to:

  • Add: Create new manual payment methods (BVR, LSV, Permanent Order).

  • Update: Edit details (e.g., BVR Reference) for an existing group.

  • Change: Move a specific sponsorship to another existing payment group.

• Feedback: Implemented Toast notifications for user actions (Success/Error).

• Controller: Added routes to handle manual group creation and method switching.

• Backend: Add methods to models ( res_partner, contracts, contract_group) to retrieve, change and update groups.

• Fontend: Handle modals, trigger RPC request, allow optimistic UI.

Results:

/my2/donations page displaying

Change payment method for a contract modal

Update payment method for a group of contract modal

Add a new payment methods

Example of toast

[SamBachmann]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a significant and valuable feature, refactoring the donations page to group sponsorships by payment method and adding comprehensive management capabilities. The changes are well-structured, spanning across the backend controllers and models to the frontend JavaScript widgets and templates. My review focuses on improving robustness, performance, and maintainability. I've identified a few issues, including a duplicated line of code, a potential logic flaw in the JavaScript event handling, and opportunities for optimization by improving data fetching and moving business logic from templates to the model layer. Addressing these points will make the implementation even more solid.

[gemini-code-assist]

This line is a duplicate of the one directly above it. It should be removed to avoid redundancy.

[gemini-code-assist]

The use of a broad except Exception: clause can hide unexpected errors and make debugging more difficult. It's better to catch more specific exceptions that you might anticipate during the group creation process, such as odoo.exceptions.ValidationError. Additionally, you should log the exception to help with troubleshooting in production environments.

[gemini-code-assist]

The current implementation to find a payment icon is inefficient and could be unreliable.

1. Performance: self.env["payment.icon"].sudo().search(...) is called every time get_payment_method_info is executed. This can lead to performance issues if there are many icons or if the method is called frequently, as it queries the database repeatedly for the same data.
2. Correctness: The check if icon.name.lower() in self.payment_mode_id.name.lower(): performs a fuzzy match, which might lead to incorrect icons being displayed if payment mode names are substrings of others (e.g., 'Visa' and 'Visa Electron').

Consider fetching all icons once and caching them, or establishing a more direct relationship between account.payment.mode and payment.icon to avoid searching and string matching.

[gemini-code-assist]

This FormFieldComponent template appears to be a client-side duplication of the server-side theme_compassion_2025.FormFieldComponent. Duplicating component templates increases maintenance overhead, as any changes would need to be applied in both places. While sometimes necessary in Odoo, consider if there's a way to render the backend template via an RPC call or unify the component definitions to have a single source of truth.

[SamBachmann]

/gemini Could you provide me an implementation for that ?

[gemini-code-assist]

Hello SamBachmann, thanks for asking for clarification on this. You're right to ask for an implementation, as achieving a single source of truth for the FormFieldComponent in this context is a bit more involved due to how Odoo's client-side and server-side QWeb templates interact.

The Challenge:

The my_compassion.FormFieldComponent template (lines 159-182 in my_compassion/static/src/xml/my2_payment_method_templates.xml) is a client-side QWeb template. It's loaded by your JavaScript widget (my2_donations.js) and used by other client-side templates like my_compassion.PaymentMethodUpdateForm.

On the other hand, theme_compassion_2025.FormFieldComponent is a server-side QWeb template. Client-side QWeb templates cannot directly call server-side QWeb templates using t-call.

Therefore, simply removing the client-side my_compassion.FormFieldComponent would break the client-side rendering flow for your modals.

Proposed Implementation for Unification (RPC Approach):

To truly unify and avoid this duplication, the most robust approach would be to render the FormFieldComponent server-side and fetch its HTML content via an RPC call when needed. This would involve the following steps:

1. Remove the client-side my_compassion.FormFieldComponent definition.

   • Delete lines 159-182 from my_compassion/static/src/xml/my2_payment_method_templates.xml.

2. Create a new RPC endpoint in your Python controller (my_compassion/controllers/my2_donations.py).

   • This endpoint would take parameters like label, name, value, placeholder, disabled, input_class, and invalid_hint.
   • It would then render the server-side theme_compassion_2025.FormFieldComponent using these parameters and return the generated HTML string.

   # Example in my_compassion/controllers/my2_donations.py
   @http.route('/my2/render_form_field', type='json', auth='user', website=True)
   def render_form_field_component(self, label=None, name=None, value=None, placeholder=None, disabled=False, input_class='', invalid_hint=None, **kw):
       return request.env['ir.ui.view']._render_template(
           'theme_compassion_2025.FormFieldComponent',
           {
               'label': label,
               'name': name,
               'id': name, # Assuming id is same as name for this component
               'value': value,
               'placeholder': placeholder,
               'disabled': disabled,
               'input_class': input_class,
               'invalid_hint': invalid_hint,
           }
       )

3. Modify your JavaScript widget (my_compassion/static/src/js/my2_donations.js) and the my_compassion.PaymentMethodUpdateForm template.

   • In my_compassion.PaymentMethodUpdateForm, remove the t-call="my_compassion.FormFieldComponent" and replace it with a placeholder element (e.g., a div with a specific ID) where the dynamically loaded HTML will be injected.
   • In your _onOpenPaymentMethodUpdate function (or a new helper function), make an RPC call to the new /my2/render_form_field endpoint, passing the necessary data for the form field.
   • Upon receiving the HTML response, inject it into the placeholder element in your modal.

   // Example in my_compassion/static/src/js/my2_donations.js
   _onOpenPaymentMethodUpdate: function (ev) {
       // ... existing code ...
       var methodInfo = detail ? detail.method_info : null;
       var $modalUpdate = $("#payment_method_selector_modal_update");
       var $container = $modalUpdate.find("#modal_container").empty();
   
       // Render the accordion structure first
       $container.html(QWeb.render("my_compassion.PaymentMethodUpdateAccordion", methodInfo));
   
       // Now, fetch and inject the form field HTML via RPC
       this._rpc({
           route: '/my2/render_form_field',
           params: {
               label: _t("Reference Number"),
               name: "ref_number",
               value: methodInfo.ref_number || '',
               // Add other parameters as needed
           },
       }).then(function (formFieldHtml) {
           $modalUpdate.find('#form_field_placeholder').html(formFieldHtml); // Assuming a placeholder div with id 'form_field_placeholder'
       });
   
       this._renderPaymentMethods($modalUpdate, groupId, "#payment_methods_switch_container");
       $modalUpdate.modal("show");
   },

   <!-- Example in my_compassion/static/src/xml/my2_payment_method_templates.xml for PaymentMethodUpdateForm -->
   <t t-name="my_compassion.PaymentMethodUpdateForm">
       <div class="payment-update-form text-left">
           <!-- Placeholder for the server-rendered FormFieldComponent -->
           <div id="form_field_placeholder"></div>
       </div>
   </t>

This approach ensures that the FormFieldComponent logic is maintained in a single server-side template (theme_compassion_2025.FormFieldComponent), and the client-side only receives the rendered HTML, thus eliminating the duplication. This is a medium priority change as it improves maintainability and reduces code duplication.

[SamBachmann]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a significant and valuable feature for payment method management, refactoring the donations page to group sponsorships by payment methods. The implementation is well-structured, leveraging new backend models, controller endpoints, and frontend components, including modals and an Odoo public widget for interactivity. My review focuses on improving performance by addressing a potential N+1 query issue, enhancing the robustness of the new controller endpoints with better input validation and error handling, and correcting a minor typo in a user-facing message. Overall, this is a solid contribution that greatly enhances user capabilities.

[gemini-code-assist]

This method has a performance issue (N+1 problem). The line all_icons = self.env["payment.icon"].sudo().search(...) is executed for every single group when this method is called in a loop from the controller or a template. This will lead to a large number of database queries and slow down page load times, especially for users with many payment methods.

To fix this, you should refactor the method to work on a recordset (self) instead of a single record. This involves removing self.ensure_one() and performing the payment.icon search only once outside the loop. The method would then return a list of dictionaries.

After refactoring, you will also need to update the call sites in my_compassion/controllers/my2_donations.py and any templates to call this method on the entire recordset at once, rather than in a loop.

    def get_payment_method_info(self):
        """
        Returns a list of dicts containing display info for the groups' payment methods.
        Used in MyCompassion2.0 portal.
        """
        if not self:
            return []

        all_icons = self.env["payment.icon"].sudo().search([("image", "!=", False)])
        icon_data = [(icon.name.lower(), icon.id) for icon in all_icons]

        results = []
        for group in self:
            info = {
                "icon": False,
                "ref_number": False,
                "label": _("Unknown Method"),
                "expire_date": False,
                "is_card": False,
                "mode_id": group.payment_mode_id.id if group.payment_mode_id else False,
                "group_id": group.id,
            }

            if group.payment_mode_id:
                info["label"] = group.payment_mode_id.display_name
                info["type"] = "mode"
                info["ref_number"] = group.bvr_reference or False

                pm_name_lower = group.payment_mode_id.name.lower()
                for icon_name, icon_id in icon_data:
                    if icon_name in pm_name_lower:
                        info["icon"] = icon_id
                        break
            results.append(info)
        return results

[gemini-code-assist]

The parameters contract_id and group_id are cast to int without proper error handling. If a non-integer value is passed in the JSON request, this will raise a ValueError and result in a 500 Internal Server Error. It's safer to wrap the casting in a try-except block and raise a BadRequest for invalid input. This will provide a clearer error to the client.

This same issue is present in other new endpoints in this file where int() is used on request parameters.

    def change_payment_method_contract(self, contract_id, group_id, **kwargs):
        """
        Changes the payment method for a specific contract.

        :param contract_id: ID of the recurring.contract to update.
        :param group_id: ID of an existing group to merge into
        """
        partner = request.env.user.partner_id
        try:
            contract_id = int(contract_id)
            group_id = int(group_id)
        except (ValueError, TypeError):
            raise BadRequest()

        # Verify that the contract belongs to the user
        contract = (
            request.env["recurring.contract"]
            .sudo()
            .search([("id", "=", contract_id), ("partner_id", "=", partner.id)])
        )
        if not contract:
            raise NotFound()

        success = contract.change_contract_group(group_id)
        return {"success": success}

[gemini-code-assist]

Catching a broad odoo.exceptions.ValidationError and returning a generic error message hides the specific reason for the failure from the user and makes debugging more difficult. For instance, if a unique constraint is violated, Odoo's default JSON-RPC error handling would provide a much more informative message.

Consider removing this try...except block and letting Odoo's framework handle the ValidationError. This will provide more meaningful feedback to the user.

Suggested change

	except odoo.exceptions.ValidationError:
	return {
	"success": False,
	"error": _("An unexpected error occurred. Please try again."),
	}
	# Let Odoo's default error handler manage ValidationErrors for better feedback.
	new_group = (
	request.env["recurring.contract.group"]
	.sudo()
	.create(
	{
	"partner_id": partner.id,
	"payment_mode_id": payment_mode.id,
	"recurring_unit": recurring_unit,
	"recurring_value": int(advance_billing_months),
	"active": True,
	}
	)
	)
	new_bvr_ref = new_group.compute_partner_bvr_ref(partner)
	if new_bvr_ref:
	new_group.bvr_reference = new_bvr_ref
	return {"success": True, "group_id": new_group.id}

[SamBachmann]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a significant and well-structured refactoring of the donations page, adding payment method management capabilities. The backend logic is sound, with new controller endpoints and model methods to support the frontend functionality. The frontend is nicely refactored into an Odoo JavaScript widget, handling modals and UI updates gracefully.

My review includes a few suggestions for improvement:

• Translating user-facing error messages.
• Optimizing a database query for better performance.
• Removing leftover debugging code.
• Improving file naming for better maintainability.

Overall, this is a solid implementation of a complex feature.

[gemini-code-assist]

The error message 'Operation failed' is not translated. Since this is a user-facing message, it should be wrapped in _() to allow for translation.

Suggested change

	return {"success": False, "error": "Operation failed"}
	return {"success": False, "error": _("Operation failed")}

[gemini-code-assist]

The error message 'Operation failed' is not translated. Since this is a user-facing message, it should be wrapped in _() to allow for translation.

Suggested change

	return {"success": False, "error": "Operation failed"}
	return {"success": False, "error": _("Operation failed")}

[gemini-code-assist]

Fetching all payment icons from the database on every call to get_payment_method_info is inefficient and could lead to performance issues, especially if the number of icons grows. It's better to cache the list of icons. You could use Odoo's @tools.ormcache() decorator on a model method that fetches the icons to avoid repeated database queries.

[gemini-code-assist]

A console.log statement is present here. These are useful for debugging but should be removed from the code before merging into a production branch.