Refactor iPay integration for improved logging, user association, and payment processing by kipsang01 · Pull Request #28 · truehostcloud/spree_ipay

kipsang01 · 2026-03-27T14:05:27Z

Updated logging in gateway callbacks to use Rails.logger for better integration.
Added user_id association to Spree::IpaySource model and updated user_id setter method.
Refactored Spree::PaymentMethod::Ipay to streamline payment processing and improve error handling.
Introduced new form fields for phone number and transaction ID in the iPay payment source form.
Updated Spree initializer to accommodate new attributes for iPay sources.
Added migration to include user_id in spree_ipay_sources table.
Enhanced engine configuration for better autoloading and initialization.
Updated gemspec to reflect new dependencies and Ruby version requirements.

… payment processing - Updated logging in gateway callbacks to use Rails.logger for better integration. - Added user_id association to Spree::IpaySource model and updated user_id setter method. - Refactored Spree::PaymentMethod::Ipay to streamline payment processing and improve error handling. - Introduced new form fields for phone number and transaction ID in the iPay payment source form. - Updated Spree initializer to accommodate new attributes for iPay sources. - Added migration to include user_id in spree_ipay_sources table. - Enhanced engine configuration for better autoloading and initialization. - Updated gemspec to reflect new dependencies and Ruby version requirements.

willymwai · 2026-03-27T14:05:38Z

/describe

willymwai · 2026-03-27T14:05:38Z

/improve

willymwai · 2026-03-27T14:05:38Z

/review

willymwai · 2026-03-27T14:05:39Z

/improve

willymwai · 2026-03-27T14:05:39Z

/review

qodo-code-review · 2026-03-27T14:05:54Z

User description

Updated logging in gateway callbacks to use Rails.logger for better integration.
Added user_id association to Spree::IpaySource model and updated user_id setter method.
Refactored Spree::PaymentMethod::Ipay to streamline payment processing and improve error handling.
Introduced new form fields for phone number and transaction ID in the iPay payment source form.
Updated Spree initializer to accommodate new attributes for iPay sources.
Added migration to include user_id in spree_ipay_sources table.
Enhanced engine configuration for better autoloading and initialization.
Updated gemspec to reflect new dependencies and Ruby version requirements.

PR Type

Enhancement, Bug fix

Description

Refactored iPay payment method to simplify form generation and improve code maintainability
Migrated from Spree 4.x to Spree 5.x with updated dependencies and initialization
Enhanced error handling with raise: false flags and improved logging using Rails.logger
Added user_id association to IpaySource model with proper user relationship management
Streamlined payment processing with extracted helper methods and cleaner API endpoints
Updated gemspec to require Ruby 3.1+ and Spree 5.0+

Diagram Walkthrough

flowchart LR
  A["Spree 4.x Integration"] -->|Upgrade| B["Spree 5.x Compatible"]
  B -->|Refactor| C["Simplified Payment Method"]
  C -->|Extract| D["Form Generation Helper"]
  C -->|Add| E["User Association"]
  E -->|Migrate| F["IpaySource with user_id"]
  C -->|Improve| G["Error Handling & Logging"]
  G -->|Use| H["Rails.logger"]

File Walkthrough

Relevant files

Enhancement

5 files

ipay_source.rb `Added user_id association and improved phone normalization`	+9/-2
ipay.rb `Refactored payment processing with extracted helper methods`	+224/-571
checkout_controller_decorator.rb `Extracted form generation to payment method class`	+1/-91
gateway_callbacks_controller.rb `Replaced custom logger with Rails.logger for consistency`	+1/-1
_ipay.html.erb `Added admin form for iPay payment source with phone and transaction` `fields`	+36/-0

Bug fix

2 files

ipay_controller.rb `Added conditional base controller inheritance and raise flags`	+4/-4
ipay_controller_decorator.rb `Added raise false flag to skip_before_action`	+1/-1

Configuration changes

2 files

spree_ipay.rb `Simplified initializer for Spree 5 with permitted attributes`	+13/-13
engine.rb `Simplified engine configuration for Spree 5 compatibility`	+14/-43

Database migration

1 files

20260327120000_add_user_id_to_spree_ipay_sources.rb `Added migration to add user_id foreign key to ipay sources`	+17/-0

Dependencies

2 files

spree_ipay.gemspec `Updated dependencies for Spree 5 and Ruby 3.1+`	+7/-20
Gemfile `Updated Spree version constraints to 5.0+`	+1/-6

qodo-code-review · 2026-03-27T14:06:02Z

PR Reviewer Guide 🔍

(Review updated until commit `6291fba`)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns Callback authentication bypass: `callback`/`return` skip user authentication (and related hooks). This is acceptable only if the callback verifies iPay authenticity robustly (HMAC/hash validation using a server-side secret, strict allow-listing of parameters, and replay protection / idempotency). Please double-check that the callback handler rejects requests with missing/invalid signatures and cannot be abused to mark orders as paid via forged requests.
⚡ Recommended focus areas for review Signature/fields The iPay form field construction and signature generation should be revalidated against iPay’s spec. The fields now include `rst` (return URL) and channel flags, but `signature_payload` only concatenates a fixed list and does not include `rst` or any channel parameters. If iPay expects the hash to cover additional fields (or a different `inv` strategy), callbacks/status checks may fail or be spoofable. Also `inv` is set equal to the order number, which may not be unique per attempt. def generate_ipay_form_html(payment, phone = nil) fields = build_form_fields(payment, phone) form_inputs = fields.map do \|key, value\| %(<input type='hidden' name='#{ERB::Util.html_escape(key.to_s)}' value='#{ERB::Util.html_escape(value.to_s)}'>) end.join("\n") <<~HTML <form id='ipay_form' action='#{ERB::Util.html_escape(api_endpoint)}' method='POST'> #{form_inputs} <input type='submit' value='Pay with iPay'> </form> <script>document.getElementById('ipay_form').submit();</script> HTML end def ipay_signature_hash(payment, phone = nil) signature_payload(build_form_fields(payment, phone)) end def confirm(payment, phone: nil) return success_response if payment.completed? response = process!(phone: phone \|\| payment.source&.phone, payment: payment, amount: payment.amount, options: {}) return response if response.success? failure_response(response.message) end def complete(payment) return success_response if payment.completed? status_response = check_payment_status(payment.response_code) return failure_response(status_response['message'] \|\| 'Payment completion failed') unless status_response['status'] == 'success' payment.complete! if payment.respond_to?(:can_complete?) ? payment.can_complete? : !payment.completed? success_response rescue StandardError => error failure_response("Payment completion failed: #{error.message}") end def check_payment_status(transaction_id) return error_hash('Missing transaction reference') if transaction_id.blank? response = self.class.post(status_endpoint, body: status_request_params(transaction_id)) JSON.parse(response.body) rescue StandardError error_hash('Failed to check payment status') end def cancel_payment(transaction_id) return error_hash('Missing transaction reference') if transaction_id.blank? response = self.class.post(status_endpoint, body: cancel_request_params(transaction_id)) JSON.parse(response.body) rescue StandardError error_hash('Failed to cancel payment') end def generate_hash(payment) fields = build_form_fields(payment, payment.source&.phone) signature_payload(fields.except(:hsh)) end def generate_status_hash(transaction_id) OpenSSL::HMAC.hexdigest('sha1', preferred_hash_key.to_s, [live_value, preferred_vendor_id.to_s, transaction_id.to_s].join) end def generate_cancel_hash(transaction_id) generate_status_hash(transaction_id) end def callback_url(_payment = nil) absolute_url(preferred_callback_url.presence \|\| '/ipay/confirm') end def return_url(payment = nil) fallback = payment ? "/orders/#{payment.order.number}?order_token=#{payment.order.guest_token}" : '/ipay/confirm' absolute_url(preferred_return_url.presence \|\| fallback) end def base_url default_host = Rails.application.routes.default_url_options[:host] return '' if default_host.blank? protocol = Rails.application.routes.default_url_options[:protocol].presence \|\| 'https' "#{protocol}://#{default_host}" end def test_mode? ActiveModel::Type::Boolean.new.cast(preferred_test_mode) end def api_endpoint test_mode? ? 'https://sandbox.ipayafrica.com/v3/ke' : 'https://payments.ipayafrica.com/v3/ke' end def success_response(message = 'Success', authorization: nil) ActiveMerchant::Billing::Response.new(true, message, {}, authorization: authorization, test: test_mode?) end def failure_response(message = 'Failed') ActiveMerchant::Billing::Response.new(false, message, {}, test: test_mode?) end private def build_form_fields(payment, phone = nil) normalized_phone = normalize_phone(phone \|\| payment.source&.phone \|\| payment.order.bill_address&.phone \|\| payment.order.billing_address&.phone) fields = { live: live_value, oid: payment.order.number.to_s, inv: payment.order.number.to_s, ttl: payment_amount_value(payment), tel: normalized_phone, eml: payment.order.email.to_s, vid: preferred_vendor_id.to_s, curr: preferred_currency.presence \|\| 'KES', p1: '', p2: '', p3: '', p4: '', cbk: callback_url(payment), rst: return_url(payment), cst: '1', crl: '2' } CHANNEL_PREFERENCES.each do \|channel\| fields[channel] = public_send("preferred_#{channel}") ? '1' : '0' end fields[:hsh] = signature_payload(fields) fields end def ensure_source(source, payment) ipay_source = source.presence \|\| payment.source return ipay_source if ipay_source.is_a?(Spree::IpaySource) && ipay_source.phone.present? phone = normalize_phone(payment.source&.phone \|\| payment.order&.bill_address&.phone \|\| payment.order&.billing_address&.phone) return if phone.blank? Spree::IpaySource.find_or_initialize_by(payment_method: self, phone: phone).tap do \|record\| record.save! if record.new_record? \|\| record.changed? end end def store_phone_in_session(options, phone) controller = options[:controller] return unless controller&.respond_to?(:session) && phone.present? controller.session[:ipay_phone_number] = phone end def normalize_phone(phone) digits = phone.to_s.gsub(/\D/, '') return if digits.blank? return "254#{digits[1..]}" if digits.length == 10 && digits.start_with?('0') return "254#{digits}" if digits.length == 9 digits end def payment_amount_value(payment) (payment.amount.to_f * 100).to_i.to_s end def signature_payload(fields) data_string = %i[live oid inv ttl tel eml vid curr p1 p2 p3 p4 cbk cst crl].map { \|key\| fields[key].to_s }.join OpenSSL::HMAC.hexdigest('sha1', preferred_hash_key.to_s, data_string) end Data consistency `user_id=` performs a DB lookup and assigns `user` inside the setter. This can introduce unexpected queries during mass-assignment, and can cause inconsistent state if `user` and `user_id` are set in different orders. Consider relying on ActiveRecord association handling (or a callback) rather than resolving the association inside the attribute writer. attribute :user_id, :integer belongs_to :payment_method, class_name: 'Spree::PaymentMethod::Ipay', optional: true belongs_to :user, class_name: Spree.user_class.to_s, optional: true has_many :payments, as: :source, class_name: 'Spree::Payment', dependent: :destroy # Validations validates :phone, presence: true def user_id=(value) super(value.presence) self.user = Spree.user_class.find_by(id: self[:user_id]) if self[:user_id].present? end # Callbacks before_validation :normalize_phone, if: :will_save_change_to_phone? Auth bypass The callback/return endpoints explicitly skip authentication and locale/user loading. Ensure the callback action has strong verification (HMAC/hash validation, replay protection, and strict parameter validation) since these endpoints are publicly accessible and now can be mounted under either API base or store controller depending on constants. class IpayController < (defined?(Spree::Api::V1::BaseController) ? Spree::Api::V1::BaseController : Spree::StoreController) # Only load payment for :return, not for :callback (GET/POST) before_action :load_payment, only: [:return] skip_before_action :load_payment, only: [:callback] # SKIP ALL USER-RELATED AUTH FOR CALLBACK (SECURITY BY HASH ONLY) skip_before_action :authenticate_user, only: %i[callback return], raise: false # skip_before_action :authenticate_spree_user, only: [:callback, :return] skip_before_action :load_user, only: %i[callback return], raise: false # If present in base skip_before_action :set_locale, only: %i[callback return], raise: false # Avoids user-locale issues

qodo-code-review · 2026-03-27T14:06:02Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns Authentication bypass: `callback`/`return` explicitly skip `authenticate_user` (and other filters) which is necessary for gateway callbacks, but it increases risk if request integrity is not strictly verified. Confirm the callback performs strong signature/HMAC validation against `preferred_hash_key`, rejects missing/invalid hashes, and includes replay protection (e.g., idempotency by transaction id, timestamp validation, or one-time use checks). Also ensure the public `return` endpoint does not expose sensitive order/payment data without validating the `order_token` or equivalent.
⚡ Recommended focus areas for review Signature Mismatch The form fields include `rst` (return URL) but `signature_payload` does not include it in the signed data string. If iPay expects `rst` to be part of the signature (or expects a specific parameter order), this can cause intermittent hash validation failures depending on gateway rules. Also validate that `inv` should be unique per attempt; it is currently set to the order number (same as `oid`), which may conflict with gateway expectations for invoice uniqueness. def generate_ipay_form_html(payment, phone = nil) fields = build_form_fields(payment, phone) form_inputs = fields.map do \|key, value\| %(<input type='hidden' name='#{ERB::Util.html_escape(key.to_s)}' value='#{ERB::Util.html_escape(value.to_s)}'>) end.join("\n") <<~HTML <form id='ipay_form' action='#{ERB::Util.html_escape(api_endpoint)}' method='POST'> #{form_inputs} <input type='submit' value='Pay with iPay'> </form> <script>document.getElementById('ipay_form').submit();</script> HTML end def ipay_signature_hash(payment, phone = nil) signature_payload(build_form_fields(payment, phone)) end def confirm(payment, phone: nil) return success_response if payment.completed? response = process!(phone: phone \|\| payment.source&.phone, payment: payment, amount: payment.amount, options: {}) return response if response.success? failure_response(response.message) end def complete(payment) return success_response if payment.completed? status_response = check_payment_status(payment.response_code) return failure_response(status_response['message'] \|\| 'Payment completion failed') unless status_response['status'] == 'success' payment.complete! if payment.respond_to?(:can_complete?) ? payment.can_complete? : !payment.completed? success_response rescue StandardError => error failure_response("Payment completion failed: #{error.message}") end def check_payment_status(transaction_id) return error_hash('Missing transaction reference') if transaction_id.blank? response = self.class.post(status_endpoint, body: status_request_params(transaction_id)) JSON.parse(response.body) rescue StandardError error_hash('Failed to check payment status') end def cancel_payment(transaction_id) return error_hash('Missing transaction reference') if transaction_id.blank? response = self.class.post(status_endpoint, body: cancel_request_params(transaction_id)) JSON.parse(response.body) rescue StandardError error_hash('Failed to cancel payment') end def generate_hash(payment) fields = build_form_fields(payment, payment.source&.phone) signature_payload(fields.except(:hsh)) end def generate_status_hash(transaction_id) OpenSSL::HMAC.hexdigest('sha1', preferred_hash_key.to_s, [live_value, preferred_vendor_id.to_s, transaction_id.to_s].join) end def generate_cancel_hash(transaction_id) generate_status_hash(transaction_id) end def callback_url(_payment = nil) absolute_url(preferred_callback_url.presence \|\| '/ipay/confirm') end def return_url(payment = nil) fallback = payment ? "/orders/#{payment.order.number}?order_token=#{payment.order.guest_token}" : '/ipay/confirm' absolute_url(preferred_return_url.presence \|\| fallback) end def base_url default_host = Rails.application.routes.default_url_options[:host] return '' if default_host.blank? protocol = Rails.application.routes.default_url_options[:protocol].presence \|\| 'https' "#{protocol}://#{default_host}" end def test_mode? ActiveModel::Type::Boolean.new.cast(preferred_test_mode) end def api_endpoint test_mode? ? 'https://sandbox.ipayafrica.com/v3/ke' : 'https://payments.ipayafrica.com/v3/ke' end def success_response(message = 'Success', authorization: nil) ActiveMerchant::Billing::Response.new(true, message, {}, authorization: authorization, test: test_mode?) end def failure_response(message = 'Failed') ActiveMerchant::Billing::Response.new(false, message, {}, test: test_mode?) end private def build_form_fields(payment, phone = nil) normalized_phone = normalize_phone(phone \|\| payment.source&.phone \|\| payment.order.bill_address&.phone \|\| payment.order.billing_address&.phone) fields = { live: live_value, oid: payment.order.number.to_s, inv: payment.order.number.to_s, ttl: payment_amount_value(payment), tel: normalized_phone, eml: payment.order.email.to_s, vid: preferred_vendor_id.to_s, curr: preferred_currency.presence \|\| 'KES', p1: '', p2: '', p3: '', p4: '', cbk: callback_url(payment), rst: return_url(payment), cst: '1', crl: '2' } CHANNEL_PREFERENCES.each do \|channel\| fields[channel] = public_send("preferred_#{channel}") ? '1' : '0' end fields[:hsh] = signature_payload(fields) fields end def ensure_source(source, payment) ipay_source = source.presence \|\| payment.source return ipay_source if ipay_source.is_a?(Spree::IpaySource) && ipay_source.phone.present? phone = normalize_phone(payment.source&.phone \|\| payment.order&.bill_address&.phone \|\| payment.order&.billing_address&.phone) return if phone.blank? Spree::IpaySource.find_or_initialize_by(payment_method: self, phone: phone).tap do \|record\| record.save! if record.new_record? \|\| record.changed? end end def store_phone_in_session(options, phone) controller = options[:controller] return unless controller&.respond_to?(:session) && phone.present? controller.session[:ipay_phone_number] = phone end def normalize_phone(phone) digits = phone.to_s.gsub(/\D/, '') return if digits.blank? return "254#{digits[1..]}" if digits.length == 10 && digits.start_with?('0') return "254#{digits}" if digits.length == 9 digits end def payment_amount_value(payment) (payment.amount.to_f * 100).to_i.to_s end def signature_payload(fields) data_string = %i[live oid inv ttl tel eml vid curr p1 p2 p3 p4 cbk cst crl].map { \|key\| fields[key].to_s }.join OpenSSL::HMAC.hexdigest('sha1', preferred_hash_key.to_s, data_string) end User Association `ensure_source` creates/fetches `Spree::IpaySource` by `payment_method` and `phone` only and does not associate the source to the order user. With the new `user_id` relationship, this may lead to sources not being linked to users (or being shared across multiple users who reuse a phone number), impacting reporting and ownership semantics. def ensure_source(source, payment) ipay_source = source.presence \|\| payment.source return ipay_source if ipay_source.is_a?(Spree::IpaySource) && ipay_source.phone.present? phone = normalize_phone(payment.source&.phone \|\| payment.order&.bill_address&.phone \|\| payment.order&.billing_address&.phone) return if phone.blank? Spree::IpaySource.find_or_initialize_by(payment_method: self, phone: phone).tap do \|record\| record.save! if record.new_record? \|\| record.changed? end end Public Endpoints Authentication is explicitly skipped for `callback` and `return`. Ensure the callback is strictly validated (e.g., HMAC/hash verification, transaction ownership checks, replay protection) and that the `return` endpoint cannot be used to leak order/payment state. Also confirm this behavior is consistent between the controller and its decorator to avoid unexpected filter ordering. class IpayController < (defined?(Spree::Api::V1::BaseController) ? Spree::Api::V1::BaseController : Spree::StoreController) # Only load payment for :return, not for :callback (GET/POST) before_action :load_payment, only: [:return] skip_before_action :load_payment, only: [:callback] # SKIP ALL USER-RELATED AUTH FOR CALLBACK (SECURITY BY HASH ONLY) skip_before_action :authenticate_user, only: %i[callback return], raise: false # skip_before_action :authenticate_spree_user, only: [:callback, :return] skip_before_action :load_user, only: %i[callback return], raise: false # If present in base skip_before_action :set_locale, only: %i[callback return], raise: false # Avoids user-locale issues

deepsource-io · 2026-03-27T14:06:17Z

DeepSource Code Review

We reviewed changes in 772020c...6291fba on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade	Security Reliability Complexity Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
JavaScript		Mar 28, 2026 6:12a.m.	Review ↗
Ruby		Mar 28, 2026 6:12a.m.	Review ↗

qodo-code-review · 2026-03-27T14:07:17Z

PR Code Suggestions ✨

Latest suggestions up to 6291fba

Category	Suggestion	Impact
Security	Avoid cross-user source reuse Update the `ensure_source` method to include `user_id` when finding or initializing a `Spree::IpaySource`. This prevents payment sources from being incorrectly shared between different users who use the same phone number. app/models/spree/payment_method/ipay.rb [295-305] def ensure_source(source, payment) ipay_source = source.presence \|\| payment.source return ipay_source if ipay_source.is_a?(Spree::IpaySource) && ipay_source.phone.present? phone = normalize_phone(payment.source&.phone \|\| payment.order&.bill_address&.phone \|\| payment.order&.billing_address&.phone) return if phone.blank? - Spree::IpaySource.find_or_initialize_by(payment_method: self, phone: phone).tap do \|record\| + user = payment.order&.user + + Spree::IpaySource.find_or_initialize_by(payment_method: self, phone: phone, user_id: user&.id).tap do \|record\| + record.user \|\|= user record.save! if record.new_record? \|\| record.changed? end end Apply / Chat Suggestion importance[1-10]: 9 __ Why: This suggestion points out a critical data integrity and potential security flaw where payment sources could be shared between different users. The proposed change correctly utilizes the `user_id` association added elsewhere in the PR to fix this issue.	High
Possible issue	Prevent association recursion on assignment Refactor the `user_id=` setter in `IpaySource` to use `association(:user).reset` instead of manually assigning `self.user`. This prevents potential recursion and avoids an unnecessary database query. app/models/spree/ipay_source.rb [12-15] -attribute :user_id, :integer - belongs_to :payment_method, class_name: 'Spree::PaymentMethod::Ipay', optional: true belongs_to :user, class_name: Spree.user_class.to_s, optional: true has_many :payments, as: :source, class_name: 'Spree::Payment', dependent: :destroy # Validations validates :phone, presence: true def user_id=(value) super(value.presence) - self.user = Spree.user_class.find_by(id: self[:user_id]) if self[:user_id].present? + association(:user).reset end `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential recursion and an unnecessary database query in the custom `user_id=` setter, proposing a more robust and idiomatic Rails solution that improves code quality and performance.	Medium
More

Previous suggestions

Suggestions up to commit 6291fba

Category	Suggestion	Impact
Security	Ensure sources are user-scoped Update the `ensure_source` method to scope the `Spree::IpaySource` lookup by user, preventing payment sources from being shared between different users who have the same phone number. app/models/spree/payment_method/ipay.rb [295-305] def ensure_source(source, payment) ipay_source = source.presence \|\| payment.source return ipay_source if ipay_source.is_a?(Spree::IpaySource) && ipay_source.phone.present? phone = normalize_phone(payment.source&.phone \|\| payment.order&.bill_address&.phone \|\| payment.order&.billing_address&.phone) return if phone.blank? - Spree::IpaySource.find_or_initialize_by(payment_method: self, phone: phone).tap do \|record\| + user = payment.order&.user + + Spree::IpaySource.find_or_initialize_by(payment_method: self, phone: phone, user: user).tap do \|record\| + record.user \|\|= user record.save! if record.new_record? \|\| record.changed? end end Suggestion importance[1-10]: 9 __ Why: This is a critical suggestion that fixes a data integrity and potential security issue where a payment source could be incorrectly shared between users with the same phone number, by correctly scoping the source to the order's user.	High
Possible issue	Guard optional method calls Guard the call to `payment.checkout?` with `respond_to?(:checkout?)` to prevent a `NoMethodError` and improve compatibility across different Spree versions. app/models/spree/payment_method/ipay.rb [137-156] def process!(phone: nil, payment: nil, amount: nil, options: {}) return failure_response('Missing required parameters') unless payment&.order && phone.present? && amount.to_f.positive? return failure_response('Payment configuration error') if preferred_vendor_id.blank? \|\| preferred_hash_key.blank? - payment.started_processing! if payment.respond_to?(:started_processing!) && payment.checkout? + if payment.respond_to?(:started_processing!) && (!payment.respond_to?(:checkout?) \|\| payment.checkout?) + payment.started_processing! + end form_html = generate_ipay_form_html(payment, phone) store_phone_in_session(options, phone) options[:controller]&.session&.[]=(:ipay_form_html, form_html) ActiveMerchant::Billing::Response.new( true, 'iPay payment initiated successfully', { form_html: form_html }, authorization: payment.number, test: test_mode? ) rescue StandardError => error failure_response("Payment processing failed: #{error.message}") end Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential `NoMethodError` if the `checkout?` method is not present on the `payment` object in some Spree versions, and the proposed change improves the code's robustness and compatibility.	Low
Possible issue	Make permitted attributes mutation safe Use the `\|=` operator instead of `push(...).uniq!` to safely add permitted attributes, preventing potential errors on frozen arrays and ensuring idempotency. config/initializers/spree_ipay.rb [7-15] -Spree::PermittedAttributes.source_attributes.push( - :phone, - :status, - :transaction_id, - :transaction_reference, - :transaction_amount, - :transaction_timestamp, - :metadata -).uniq! +Spree::PermittedAttributes.source_attributes \|= [ + :phone, + :status, + :transaction_id, + :transaction_reference, + :transaction_amount, + :transaction_timestamp, + :metadata +] Suggestion importance[1-10]: 4 __ Why: The suggestion improves code robustness by replacing a mutable operation (`push.uniq!`) with a safer, idempotent alternative (`\|=`), which prevents potential errors if the array is frozen.	Low

Suggestions up to commit 764147f

Category	Suggestion	Impact
Possible issue	Persist user on payment source In the `ensure_source` method, associate the order's user with the `Spree::IpaySource` record before saving. This ensures the `user_id` is correctly persisted. app/models/spree/payment_method/ipay.rb [295-305] def ensure_source(source, payment) ipay_source = source.presence \|\| payment.source return ipay_source if ipay_source.is_a?(Spree::IpaySource) && ipay_source.phone.present? phone = normalize_phone(payment.source&.phone \|\| payment.order&.bill_address&.phone \|\| payment.order&.billing_address&.phone) return if phone.blank? Spree::IpaySource.find_or_initialize_by(payment_method: self, phone: phone).tap do \|record\| + record.user \|\|= payment.order&.user record.save! if record.new_record? \|\| record.changed? end end Suggestion importance[1-10]: 8 __ Why: This suggestion fixes a significant logical omission in the PR. Without it, the new `user_id` column on `Spree::IpaySource` would not be populated, defeating the purpose of the user-association feature.	Medium
	Prevent association setter recursion Simplify the `user_id=` setter to prevent potential recursion and unnecessary database lookups. Let the `belongs_to` association manage the `user` object synchronization. app/models/spree/ipay_source.rb [12-15] -attribute :user_id, :integer - belongs_to :payment_method, class_name: 'Spree::PaymentMethod::Ipay', optional: true belongs_to :user, class_name: Spree.user_class.to_s, optional: true has_many :payments, as: :source, class_name: 'Spree::Payment', dependent: :destroy # Validations validates :phone, presence: true def user_id=(value) super(value.presence) - self.user = Spree.user_class.find_by(id: self[:user_id]) if self[:user_id].present? end Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the custom `user_id=` setter is redundant and can cause issues like recursion or unnecessary database queries, proposing a much safer and simpler implementation.	Low
	Make migration table reference stable In the migration file, use the static table name `:spree_users` for the foreign key reference instead of dynamically looking it up via `Spree.user_class.table_name` to improve stability. db/migrate/20260327120000_add_user_id_to_spree_ipay_sources.rb [5-9] add_reference :spree_ipay_sources, :user, - foreign_key: { to_table: spree_user_table_name }, + foreign_key: { to_table: :spree_users }, index: true, null: true Suggestion importance[1-10]: 5 __ Why: The suggestion improves the migration's robustness by replacing a dynamic table name lookup with a static symbol, which is a Rails best practice that prevents potential future migration failures.	Low

Suggestions up to commit 764147f

Category	Suggestion	Impact
Security	Prevent cross-user source reuse Update the `ensure_source` method to include the `user` when finding or creating a `Spree::IpaySource`. This prevents incorrectly reusing a payment source across different users who might share the same phone number. app/models/spree/payment_method/ipay.rb [295-305] def ensure_source(source, payment) ipay_source = source.presence \|\| payment.source return ipay_source if ipay_source.is_a?(Spree::IpaySource) && ipay_source.phone.present? phone = normalize_phone(payment.source&.phone \|\| payment.order&.bill_address&.phone \|\| payment.order&.billing_address&.phone) return if phone.blank? - Spree::IpaySource.find_or_initialize_by(payment_method: self, phone: phone).tap do \|record\| + user = payment.order&.user + + finder_attrs = { payment_method: self, phone: phone } + finder_attrs[:user] = user if user.present? + + Spree::IpaySource.find_or_initialize_by(finder_attrs).tap do \|record\| + record.user \|\|= user if user.present? record.save! if record.new_record? \|\| record.changed? end end Suggestion importance[1-10]: 9 __ Why: This suggestion addresses a significant data integrity and potential security issue where a payment source could be shared between different users based solely on a phone number. The proposed change correctly uses the new `user_id` column to properly scope the payment source to the user.	High
Possible issue	Avoid overriding association foreign keys Remove the redundant `attribute :user_id` declaration and the `user_id=` setter method. ActiveRecord automatically provides these for the `belongs_to :user` association. app/models/spree/ipay_source.rb [3-15] -attribute :user_id, :integer - belongs_to :payment_method, class_name: 'Spree::PaymentMethod::Ipay', optional: true belongs_to :user, class_name: Spree.user_class.to_s, optional: true has_many :payments, as: :source, class_name: 'Spree::Payment', dependent: :destroy # Validations validates :phone, presence: true -def user_id=(value) - super(value.presence) - self.user = Spree.user_class.find_by(id: self[:user_id]) if self[:user_id].present? -end - Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the explicit `attribute` definition and `user_id=` setter are redundant and inefficient given the `belongs_to :user` association, which ActiveRecord handles automatically. Removing this code improves quality and aligns with Rails conventions.	Low

Suggestions up to commit 552090c

Category	Suggestion	Impact
General	Associate payment source to user Update the `ensure_source` method to associate the newly created `Spree::IpaySource` record with the order's user. This completes the feature of linking payment sources to users. app/models/spree/payment_method/ipay.rb [295-305] def ensure_source(source, payment) ipay_source = source.presence \|\| payment.source return ipay_source if ipay_source.is_a?(Spree::IpaySource) && ipay_source.phone.present? - phone = normalize_phone(payment.source&.phone \|\| payment.order&.bill_address&.phone \|\| payment.order&.billing_address&.phone) + phone = normalize_phone( + payment.source&.phone \|\| payment.order&.bill_address&.phone \|\| payment.order&.billing_address&.phone + ) return if phone.blank? Spree::IpaySource.find_or_initialize_by(payment_method: self, phone: phone).tap do \|record\| + record.user \|\|= payment.order.user if payment.order&.user record.save! if record.new_record? \|\| record.changed? end end Suggestion importance[1-10]: 8 __ Why: This is an excellent catch. The PR adds a `user_id` to the `IpaySource` model but fails to populate it when creating a new source. The suggestion correctly fixes this omission, ensuring the new feature for associating a user with a payment source is fully functional.	Medium
Possible issue	Validate phone before signing In `build_form_fields`, add validation to ensure the normalized phone number is not blank and is in the correct format before it is used to generate the iPay signature. This prevents payment failures from invalid phone numbers. app/models/spree/payment_method/ipay.rb [265-293] def build_form_fields(payment, phone = nil) - normalized_phone = normalize_phone(phone \|\| payment.source&.phone \|\| payment.order.bill_address&.phone \|\| payment.order.billing_address&.phone) + normalized_phone = normalize_phone( + phone \|\| payment.source&.phone \|\| payment.order.bill_address&.phone \|\| payment.order.billing_address&.phone + ) + + if normalized_phone.blank? \|\| !normalized_phone.match?(/\A\d{12}\z/) + raise ArgumentError, 'Invalid phone number for iPay' + end fields = { live: live_value, oid: payment.order.number.to_s, inv: payment.order.number.to_s, ttl: payment_amount_value(payment), tel: normalized_phone, eml: payment.order.email.to_s, vid: preferred_vendor_id.to_s, curr: preferred_currency.presence \|\| 'KES', p1: '', p2: '', p3: '', p4: '', cbk: callback_url(payment), rst: return_url(payment), cst: '1', crl: '2' } ... fields[:hsh] = signature_payload(fields) fields end Suggestion importance[1-10]: 7 __ Why: This suggestion correctly identifies a potential bug where a malformed phone number could be normalized to `nil`, leading to an incorrect payment signature and subsequent failure. Adding validation for the normalized phone number before it's used to generate the signature is a valid improvement for correctness and error handling.	Medium
Possible issue	Remove risky foreign key override Refactor the `Spree::IpaySource` model to avoid overriding the `user_id=` setter. Instead, use a `before_validation` callback to associate the user object from the `user_id`. app/models/spree/ipay_source.rb [3-15] -attribute :user_id, :integer - belongs_to :payment_method, class_name: 'Spree::PaymentMethod::Ipay', optional: true belongs_to :user, class_name: Spree.user_class.to_s, optional: true has_many :payments, as: :source, class_name: 'Spree::Payment', dependent: :destroy +before_validation :set_user_from_user_id + ... -def user_id=(value) - super(value.presence) - self.user = Spree.user_class.find_by(id: self[:user_id]) if self[:user_id].present? +def set_user_from_user_id + return if user.present? \|\| self[:user_id].blank? + + self.user = Spree.user_class.find_by(id: self[:user_id]) end Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that overriding the `user_id=` setter is poor practice and can lead to redundant operations or bugs. The proposed solution using a `before_validation` callback is a much cleaner and more idiomatic Rails approach, improving code quality and robustness.	Low

Suggestions up to commit 552090c

Category	Suggestion	Impact
General	Scope sources to the user Update the `ensure_source` method to correctly associate the `IpaySource` with the order's user, scoping the source by user in addition to the phone number. app/models/spree/payment_method/ipay.rb [295-305] def ensure_source(source, payment) ipay_source = source.presence \|\| payment.source return ipay_source if ipay_source.is_a?(Spree::IpaySource) && ipay_source.phone.present? - phone = normalize_phone(payment.source&.phone \|\| payment.order&.bill_address&.phone \|\| payment.order&.billing_address&.phone) + phone = normalize_phone( + payment.source&.phone \|\| payment.order&.bill_address&.phone \|\| payment.order&.billing_address&.phone + ) return if phone.blank? - Spree::IpaySource.find_or_initialize_by(payment_method: self, phone: phone).tap do \|record\| + user = payment.order&.user + + finder = { payment_method: self, phone: phone } + finder[:user] = user if user.present? + + Spree::IpaySource.find_or_initialize_by(finder).tap do \|record\| + record.user \|\|= user if user.present? record.save! if record.new_record? \|\| record.changed? end end Suggestion importance[1-10]: 9 __ Why: This suggestion points out a critical flaw where the new `user_id` association, a key feature of the PR, is not being used when creating payment sources. This fix is essential for the feature to work as intended and to prevent data integrity issues.	High
Possible issue	Enforce required phone before hashing Add a check in `build_form_fields` to ensure the normalized phone number is not blank before generating the payment form, preventing the creation of an invalid iPay signature. app/models/spree/payment_method/ipay.rb [265-293] def build_form_fields(payment, phone = nil) - normalized_phone = normalize_phone(phone \|\| payment.source&.phone \|\| payment.order.bill_address&.phone \|\| payment.order.billing_address&.phone) + normalized_phone = normalize_phone( + phone \|\| payment.source&.phone \|\| payment.order.bill_address&.phone \|\| payment.order.billing_address&.phone + ) + raise 'Phone number is required for iPay payments' if normalized_phone.blank? fields = { live: live_value, oid: payment.order.number.to_s, inv: payment.order.number.to_s, ttl: payment_amount_value(payment), tel: normalized_phone, eml: payment.order.email.to_s, vid: preferred_vendor_id.to_s, curr: preferred_currency.presence \|\| 'KES', p1: '', p2: '', p3: '', p4: '', cbk: callback_url(payment), rst: return_url(payment), cst: '1', crl: '2' } ... fields[:hsh] = signature_payload(fields) fields end Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a bug where an invalid phone number could lead to a `nil` value after normalization, causing an incorrect payment hash and a broken payment flow. Adding a validation to fail fast is a crucial fix for correctness.	Medium
Possible issue	Prevent boot-time constant errors Wrap the modification of `Spree::PermittedAttributes` in a `Rails.application.config.to_prepare` block to prevent a potential `NameError` during application boot. config/initializers/spree_ipay.rb [7-15] -Spree::PermittedAttributes.source_attributes.push( - :phone, - :status, - :transaction_id, - :transaction_reference, - :transaction_amount, - :transaction_timestamp, - :metadata -).uniq! +Rails.application.config.to_prepare do + next unless defined?(Spree::PermittedAttributes) + Spree::PermittedAttributes.source_attributes \|= [ + :phone, + :status, + :transaction_id, + :transaction_reference, + :transaction_amount, + :transaction_timestamp, + :metadata + ] +end + Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential `NameError` due to Rails' load order and proposes a robust, standard solution using `to_prepare`, which prevents the application from failing to boot.	Medium

qodo-code-review · 2026-03-27T14:07:32Z

Persistent suggestions updated to latest commit 552090c

…ruby-version

willymwai · 2026-03-28T06:10:25Z

/describe

willymwai · 2026-03-28T06:10:25Z

/improve

willymwai · 2026-03-28T06:10:25Z

/review

willymwai · 2026-03-28T06:10:26Z

/review

willymwai · 2026-03-28T06:10:26Z

/improve

qodo-code-review · 2026-03-28T06:10:42Z

User description

Updated logging in gateway callbacks to use Rails.logger for better integration.
Added user_id association to Spree::IpaySource model and updated user_id setter method.
Refactored Spree::PaymentMethod::Ipay to streamline payment processing and improve error handling.
Introduced new form fields for phone number and transaction ID in the iPay payment source form.
Updated Spree initializer to accommodate new attributes for iPay sources.
Added migration to include user_id in spree_ipay_sources table.
Enhanced engine configuration for better autoloading and initialization.
Updated gemspec to reflect new dependencies and Ruby version requirements.

PR Type

Enhancement, Bug fix

Description

Refactored iPay payment method to streamline code and improve maintainability
- Consolidated preference definitions using CHANNEL_PREFERENCES constant
- Simplified form generation logic by extracting to dedicated methods
- Improved error handling with consistent response patterns
Enhanced user association in iPay payment sources
- Added user_id attribute and association to Spree::IpaySource model
- Created migration to add user_id column with foreign key constraint
- Implemented custom setter for automatic user lookup
Updated authentication handling in API controllers
- Added raise: false flag to skip_before_action calls for graceful degradation
- Made base controller inheritance conditional for better compatibility
Upgraded dependencies to Spree 5.x and Rails 7.2
- Updated gemspec, Gemfile, and Ruby version requirements
- Simplified engine configuration for Spree 5 Zeitwerk autoloading
- Updated initializer to use new permitted attributes API

Diagram Walkthrough

flowchart LR
  A["iPay Payment Method"] -->|"Refactored"| B["Streamlined Code Structure"]
  A -->|"Enhanced"| C["User Association"]
  D["API Controllers"] -->|"Updated Auth"| E["Graceful Degradation"]
  F["Dependencies"] -->|"Upgraded"| G["Spree 5.x & Rails 7.2"]
  B -->|"Includes"| H["Form Generation Methods"]
  B -->|"Includes"| I["Preference Management"]
  C -->|"Includes"| J["User ID Migration"]
  C -->|"Includes"| K["IpaySource Model"]

File Walkthrough

Relevant files

Enhancement

8 files

ipay.rb `Major refactoring of payment method logic and structure`	+224/-571
ipay_source.rb `Added user association and custom setter logic`	+9/-2
checkout_controller_decorator.rb `Extracted form generation to payment method`	+1/-91
gateway_callbacks_controller.rb `Updated logging to use Rails.logger`	+1/-1
_ipay.html.erb `New admin form for iPay payment source`	+36/-0
spree_ipay.rb `Simplified initializer for Spree 5 compatibility`	+13/-13
20260327120000_add_user_id_to_spree_ipay_sources.rb `Migration to add user_id foreign key column`	+17/-0
engine.rb `Simplified engine configuration for Zeitwerk autoloading`	+14/-43

Bug fix

2 files

ipay_controller.rb `Updated authentication handling with raise flag`	+4/-4
ipay_controller_decorator.rb `Added raise false to skip_before_action calls`	+1/-1

Dependencies

2 files

spree_ipay.gemspec `Updated dependencies to Spree 5 and Rails 7.2`	+7/-21
Gemfile `Updated Ruby and Rails version requirements`	+3/-8

Configuration changes

1 files

.ruby-version `Downgraded Ruby version from 3.3.8 to 3.3.6`	+1/-1

qodo-code-review · 2026-03-28T06:10:43Z

Persistent review updated to latest commit 764147f

qodo-code-review · 2026-03-28T06:10:49Z

Persistent review updated to latest commit 764147f

qodo-code-review · 2026-03-28T06:11:32Z

Persistent suggestions updated to latest commit 764147f

qodo-code-review · 2026-03-28T06:11:45Z

Persistent suggestions updated to latest commit 764147f

…ty with earlier versions

willymwai · 2026-03-28T06:12:37Z

/describe

willymwai · 2026-03-28T06:12:37Z

/improve

willymwai · 2026-03-28T06:12:37Z

/review

willymwai · 2026-03-28T06:12:38Z

/review

willymwai · 2026-03-28T06:12:38Z

/improve

qodo-code-review · 2026-03-28T06:12:54Z

Persistent review updated to latest commit 6291fba

qodo-code-review · 2026-03-28T06:12:54Z

User description

Updated logging in gateway callbacks to use Rails.logger for better integration.
Added user_id association to Spree::IpaySource model and updated user_id setter method.
Refactored Spree::PaymentMethod::Ipay to streamline payment processing and improve error handling.
Introduced new form fields for phone number and transaction ID in the iPay payment source form.
Updated Spree initializer to accommodate new attributes for iPay sources.
Added migration to include user_id in spree_ipay_sources table.
Enhanced engine configuration for better autoloading and initialization.
Updated gemspec to reflect new dependencies and Ruby version requirements.

PR Type

Enhancement, Bug fix

Description

Refactored iPay payment method to simplify form generation and improve code maintainability
Added user_id association to IpaySource model with proper user lookup in setter method
Simplified controller inheritance with fallback to StoreController for compatibility
Updated logging to use Rails.logger instead of custom Spree::Ipay::Logger
Upgraded Spree version requirement from 4.5.x to 5.0.x with Rails 7.2.0
Streamlined payment processing with extracted form field building and signature generation
Added migration to support user_id foreign key in spree_ipay_sources table
Enhanced engine configuration with simplified autoloading and decorator activation

Diagram Walkthrough

flowchart LR
  A["IpaySource Model"] -->|"user_id association"| B["User Lookup"]
  C["PaymentMethod::Ipay"] -->|"build_form_fields"| D["Form HTML Generation"]
  C -->|"signature_payload"| E["Hash Generation"]
  F["Controller"] -->|"authorize/capture"| C
  C -->|"process!"| D
  G["Engine Config"] -->|"decorator loading"| H["Controller Decorators"]
  I["Gemspec"] -->|"Spree 5.0+"| J["Rails 7.2.0"]

File Walkthrough

Relevant files

Enhancement

7 files

ipay_source.rb `Added user_id association and custom setter`	+9/-2
ipay.rb `Refactored payment processing with extracted helpers`	+224/-571
checkout_controller_decorator.rb `Extracted form generation to payment method`	+1/-91
_ipay.html.erb `New admin form for iPay payment source`	+36/-0
spree_ipay.rb `Simplified initializer for Spree 5 compatibility`	+13/-13
20260327120000_add_user_id_to_spree_ipay_sources.rb `Migration to add user_id foreign key column`	+17/-0
engine.rb `Simplified engine configuration and decorator loading`	+14/-43

Bug fix

3 files

ipay_controller.rb `Added fallback controller inheritance and raise: false`	+4/-4
ipay_controller_decorator.rb `Added raise: false to skip_before_action calls`	+1/-1
gateway_callbacks_controller.rb `Replaced custom logger with Rails.logger`	+1/-1

Dependencies

2 files

spree_ipay.gemspec `Updated Spree to 5.0+ and Rails to 7.2.0`	+7/-21
Gemfile `Updated Ruby and dependency versions`	+3/-8

Configuration changes

1 files

.ruby-version `Updated Ruby version to 3.3.6`	+1/-1

qodo-code-review · 2026-03-28T06:12:58Z

Persistent review updated to latest commit 6291fba

qodo-code-review · 2026-03-28T06:13:50Z

Persistent suggestions updated to latest commit 6291fba

qodo-code-review · 2026-03-28T06:13:53Z

Persistent suggestions updated to latest commit 6291fba

qodo-code-review Bot added Possible security concern Review effort 4/5 labels Mar 27, 2026

Update Ruby and Rails version requirements in gemspec, Gemfile, and .…

764147f

…ruby-version

Update spree version constraints in Gemfile.lock to allow compatibili…

6291fba

…ty with earlier versions

willymwai approved these changes Mar 28, 2026

View reviewed changes

willymwai merged commit 83bde25 into main Mar 28, 2026
5 checks passed

willymwai deleted the upgrade-spree branch March 28, 2026 06:18

Uh oh!

Conversation

kipsang01 commented Mar 27, 2026

Uh oh!

willymwai commented Mar 27, 2026

Uh oh!

willymwai commented Mar 27, 2026

Uh oh!

willymwai commented Mar 27, 2026

Uh oh!

willymwai commented Mar 27, 2026

Uh oh!

willymwai commented Mar 27, 2026

Uh oh!

qodo-code-review Bot commented Mar 27, 2026

User description

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-code-review Bot commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 6291fba)

Uh oh!

qodo-code-review Bot commented Mar 27, 2026

PR Reviewer Guide 🔍

Uh oh!

deepsource-io Bot commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DeepSource Code Review

PR Report Card

Code Review Summary

Uh oh!

qodo-code-review Bot commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

qodo-code-review Bot commented Mar 27, 2026

Uh oh!

willymwai commented Mar 28, 2026

Uh oh!

willymwai commented Mar 28, 2026

Uh oh!

willymwai commented Mar 28, 2026

Uh oh!

willymwai commented Mar 28, 2026

Uh oh!

willymwai commented Mar 28, 2026

Uh oh!

qodo-code-review Bot commented Mar 28, 2026

User description

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-code-review Bot commented Mar 28, 2026

Uh oh!

qodo-code-review Bot commented Mar 28, 2026

Uh oh!

qodo-code-review Bot commented Mar 28, 2026

Uh oh!

qodo-code-review Bot commented Mar 28, 2026

Uh oh!

willymwai commented Mar 28, 2026

Uh oh!

willymwai commented Mar 28, 2026

Uh oh!

willymwai commented Mar 28, 2026

Uh oh!

willymwai commented Mar 28, 2026

Uh oh!

willymwai commented Mar 28, 2026

Uh oh!

qodo-code-review Bot commented Mar 28, 2026

Uh oh!

qodo-code-review Bot commented Mar 28, 2026

qodo-code-review Bot commented Mar 27, 2026 •

edited

Loading

(Review updated until commit `6291fba`)

deepsource-io Bot commented Mar 27, 2026 •

edited

Loading

qodo-code-review Bot commented Mar 27, 2026 •

edited

Loading