-
Notifications
You must be signed in to change notification settings - Fork 493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improved: convert FinAccount getArithmeticSettingsInline to groovy (OFBIZ-13074) #790
Conversation
…FBIZ-13074) modified: - ofbiz-component.xml: added loader for services.xml - FinAccountServices.xml removed simple-method getArithmeticSettingsInline, replaced ref in other simple-methods with new groovy service added: - services.xml: added groovy service getArithmeticSettingsFinAccount - AccountingService.groovy: having getArithmeticSettingsFinAccount
Map result = success() | ||
arithmeticSettings = [:] | ||
roundingDecimals = EntityUtilProperties.getPropertyValue('arithmetic', 'finaccount.decimals', '2', delegator) | ||
arithmeticSettings.put('roundingDecimals', roundingDecimals) | ||
roundingMode = EntityUtilProperties.getPropertyValue('arithmetic', 'finaccount.roundingSimpleMethod', 'roundingMode', 'HalfUp', delegator) | ||
arithmeticSettings.put('roundingMode', roundingMode) | ||
result.put('arithmeticSettings', arithmeticSettings) | ||
return success (arithmeticSettings: arithmeticSettings) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can replace this by :
Map arithmeticSettings = [roundingDecimals: EntityUtilProperties.getPropertyValue('arithmetic', 'finaccount.decimals', '2', delegator),
roundingMode: EntityUtilProperties.getPropertyValue('arithmetic', 'finaccount.roundingSimpleMethod', 'roundingMode', 'HalfUp', delegator)]
return success([arithmeticSettings: arithmeticSettings])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Nicolas, for your time and effort.
I will improve this piece of code.
@@ -31,6 +31,8 @@ under the License. | |||
<entity-resource type="data" reader-name="seed" loader="main" location="data/AccountingPortletData.xml"/> | |||
<entity-resource type="data" reader-name="seed-initial" loader="main" location="data/AccountingScheduledServiceData.xml"/> | |||
|
|||
<service-resource type="model" loader="main" location="servicedef/services.xml"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to use a current service file instead introduce a new one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Nicolas, for the suggestion.
However, there are more of those arithmetic functions to get such setting used in various xml, groovy and java files. I prefer to have those in a single services.xml file, instead having those spread across many files.
Plus, having a generic 'services.xml' in the accounting component brings the component more in line with others (and the templates for creating a new plugin). Such would enhance the developer (and thus the contributor) experience.
I trust you find my solution acceptable.
…FBIZ-13074) modified: - AccountingService.groovy: having getArithmeticSettingsFinAccount Aligning code to best practices. Thanks to Nicolas Malin for reviewing the pull request.
…FBIZ-13074) removing unused Map result
Quality Gate passedIssues Measures |
modified:
added: