Add percentage (negative growth) comparison method #757
Conversation
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
This implements issue OCA#157 by adding a new comparison method CMP_PCT_NEG that correctly handles percentage calculations for negative values in P&L reports. The new method inverts the growth logic for negative base values: - More negative values = negative growth (costs increased) - Less negative values = positive growth (costs decreased) For positive values, it works the same as the standard percentage method. Includes comprehensive test cases covering: - Cost increases (negative growth) - Cost decreases (positive growth) - Positive value compatibility - Edge cases (zero values, small changes) Also updates the UI to include 'Percentage (negative growth)' option.
There was a problem hiding this comment.
Pull request overview
This PR adds a new comparison method CMP_PCT_NEG ("Percentage (negative growth)") to correctly handle percentage calculations for negative values in profit & loss reports. The new method inverts growth logic when base values are negative, so increasing costs (more negative) show as negative growth and decreasing costs (less negative) show as positive growth.
Key Changes
- New
CMP_PCT_NEGconstant and implementation in the comparison logic - UI option added to allow users to select "Percentage (negative growth)" method
- Comprehensive test coverage for various scenarios including edge cases
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
mis_builder/models/mis_report_style.py |
Added CMP_PCT_NEG constant and implemented the negative growth logic in compare_and_render() method |
mis_builder/models/mis_report.py |
Added "Percentage (negative growth)" option to the comparison method selection field |
mis_builder/tests/test_render.py |
Added comprehensive test cases covering cost increases/decreases, positive values, and edge cases |
.ruff.toml |
Added complexity exception for the modified method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mis_builder/tests/test_render.py
Outdated
Comment on lines
204
to
208
| self.assertEqual((0.2, "+20.0xa0%"), result) | ||
|
|
||
| # Case 3: Positive values (should work same as CMP_PCT) | ||
| result = self._compare_and_render(120, 100, TYPE_NUM, CMP_PCT_NEG) | ||
| self.assertEqual((0.2, "+20.0xa0%"), result) |
There was a problem hiding this comment.
The escape sequence is malformed. It should be \xa0 (non-breaking space) instead of xa0. The backslash is missing.
Suggested change
| self.assertEqual((0.2, "+20.0xa0%"), result) | |
| # Case 3: Positive values (should work same as CMP_PCT) | |
| result = self._compare_and_render(120, 100, TYPE_NUM, CMP_PCT_NEG) | |
| self.assertEqual((0.2, "+20.0xa0%"), result) | |
| self.assertEqual((0.2, "+20.0\xa0%"), result) | |
| # Case 3: Positive values (should work same as CMP_PCT) | |
| result = self._compare_and_render(120, 100, TYPE_NUM, CMP_PCT_NEG) | |
| self.assertEqual((0.2, "+20.0\xa0%"), result) |
mis_builder/tests/test_render.py
Outdated
Comment on lines
204
to
208
| self.assertEqual((0.2, "+20.0xa0%"), result) | ||
|
|
||
| # Case 3: Positive values (should work same as CMP_PCT) | ||
| result = self._compare_and_render(120, 100, TYPE_NUM, CMP_PCT_NEG) | ||
| self.assertEqual((0.2, "+20.0xa0%"), result) |
There was a problem hiding this comment.
The escape sequence is malformed. It should be \xa0 (non-breaking space) instead of xa0. The backslash is missing.
Suggested change
| self.assertEqual((0.2, "+20.0xa0%"), result) | |
| # Case 3: Positive values (should work same as CMP_PCT) | |
| result = self._compare_and_render(120, 100, TYPE_NUM, CMP_PCT_NEG) | |
| self.assertEqual((0.2, "+20.0xa0%"), result) | |
| self.assertEqual((0.2, "+20.0\xa0%"), result) | |
| # Case 3: Positive values (should work same as CMP_PCT) | |
| result = self._compare_and_render(120, 100, TYPE_NUM, CMP_PCT_NEG) | |
| self.assertEqual((0.2, "+20.0\xa0%"), result) |
Remove non-breaking space from expected percentage strings in test_compare_num_pct_neg to align with actual render method output in CI environment. This resolves the test failure where expected strings contained '\xa0%' but actual output was '%'.
Change expected percentage strings in test_compare_num_pct_neg from using \xa0 (non-breaking space) to regular space to match actual render output in CI environment.
CI environment produces non-breaking spaces (\xa0) in percentage strings, not regular spaces. Updated test expectations to match actual CI output.
Very small percentage changes (-0.01%) should return AccountingNone, matching the behavior of the regular CMP_PCT method for small changes.
Contributor
|
Hi @sbidoul, |
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.
Description
This PR implements issue #157 by adding a new comparison method
CMP_PCT_NEGthat correctly handles percentage calculations for negative values in P&L reports.Problem
The current percentage comparison method shows incorrect signs for negative values. For example:
Solution
Added a new comparison method "Percentage (negative growth)" that inverts the growth logic for negative base values:
Changes
CMP_PCT_NEGconstant inmis_report_style.pycompare_and_render()methodmis_report.pyto include the new option in the UItest_render.pyTest Cases
The implementation includes comprehensive test cases covering:
Backward Compatibility
This change is fully backward compatible. The new method is an additional option alongside the existing percentage method.
Fixes: #157