Draft
Conversation
ehuelsmann
reviewed
Jun 12, 2025
This mostly changes the way that certain data structures are interacted with, avoiding long calls when shorter refereces are possible. This commit does not yet fix any bugs and behaves identical to the code before this commit. However, it ensures that bugs can be fixed after.
The code is extremely complex for what it does. PLEASE NOTE: this does mess with localization with two strings in warnings in the use overpayment workflow. However not only was this workflow broken but the use of the translation API was also broken and therefore I do not believe this should prevent backporting.
Prior to this, the repeatedly updating the overpayment screen would eventually pass in an array as invoice date, and the amount due would be lost.
This fixes an unexpected blank line in the invoice search workflow.
This involved some further simplifying of the code in areas where there were some past bugs. Not sure exactly what the problem was, but it is now fixed. Now Cucumber tests are needed.
einhverfr
force-pushed
the
fix_overpayments
branch
from
4f2dbd9 to
44431d6
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issues with overpayments processing by fixing broken translation API calls and modifying how overpayments are applied and displayed. The key changes include:
- Adding a new helper method (script) in Payment.pm to select transaction scripts based on invoice status.
- Refactoring and simplifying the overpayment logic in the payment processing script, including renaming variables and adjusting conditions.
- Updating the UI template to accommodate new hidden fields and revised default values.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| old/lib/LedgerSMB/DBObject/Payment.pm | Added a new method (script) with inline documentation for determining the script. |
| lib/LedgerSMB/Scripts/payment.pm | Refactored overpayment logic, improved handling of invoice processing, and updated variable usage. |
| UI/payments/use_overpayment2.html | Updated the form inputs by adding a hidden field for entity name and adjusting default values. |
Comments suppressed due to low confidence (1)
lib/LedgerSMB/Scripts/payment.pm:1690
- [nitpick] The input field 'checkbox_$count' is being used to skip processing; consider renaming it (e.g., to 'ignore_checkbox_$count') to clarify its purpose.
if ($Payment->{"checkbox_$count"} or not $Payment->{"entity_id_$count"})
| options = vc_list | ||
| default_values = [ request.new_entity_id ] | ||
| default_blank = 1 | ||
| default_values = [] |
There was a problem hiding this comment.
Changing the default_values to an empty list instead of [ request.new_entity_id ] may affect the pre-selected value; please confirm that this change is intentional.
Suggested change
| default_values = [] | |
| default_values = [selected_value] |
| name => $name, | ||
| name => $vcname, | ||
| vc_discount_accno => $vc_info[$ref]->{discount}}; | ||
| $n_name = $vcname if $vc_info[$ref]->{id} == $Payment->{new_entity_id}; |
There was a problem hiding this comment.
[nitpick] Ensure that $n_name is assigned a meaningful default value in cases where no vendor/customer id matches $Payment->{new_entity_id} to prevent potential issues later in the processing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This branch and (initially draft) Pull Request fixes Overpayments issues and allows overpayments to be posted on the master branch (tests forthcoming while in draft mode).
Please note that while I intend to backport this, I have also fixed two fundamentally broken uses of the translation API (which lead to untranslated strings in 1.12. I have left several other bad calls which will be a separate bug fix for master after this gets merged.