Skip to content
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

Fix #9880 - Error when importing currency fields with a decimal separator #9881

Merged
merged 1 commit into from Oct 30, 2023

Conversation

SinergiaCRM
Copy link
Contributor

resolve #9880

Description

If the decimal separator is set to a comma "," in the user's settings, an error occurs when importing currency fields that include decimals because the decimal separator is removed and the result is the value of the field multiplied by 100.
The error was caused by the filter set for importing currency fields being incorrect, as it passes the same filter as float fields, when a currency field, after the currency symbol is removed, should follow the same treatment as a decimal field. This is what has been implemented in the solution.

Motivation and Context

Prevent errors when importing currency fields with different types of decimal separators.

How To Test

  1. Set the decimal separator to a comma “,” in the user's settings.
  2. Import a record from the Opportunities module with the Opportunity Amount set to a decimal value, for example 1,12.
  3. Verify that after the import, the Opportunity Amount field has been set to 1,12.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Final checklist

  • My code follows the code style of this project found here.
  • My change requires a change to the documentation.
    (It would be nice to specify in the documentation that the inheritance of security groups is applied exclusively at record creation time.)
  • I have read the How to Contribute guidelines.

@serhiisamko091184
Copy link
Contributor

Hello, @SinergiaCRM ,
Your change appears to be defining $value twice, on line 96 and then on 98.

Thanks,
Serhii

@juanSTIC
Copy link

Hello, @SinergiaCRM , Your change appears to be defining $value twice, on line 96 and then on 98.

Thanks, Serhii

Yes, the assignment of $value on line 98 is modifying the value obtained on line 96, which in turn modifies the value of the argument received in the function, I think that should not be a problem.

@serhiisamko091184 serhiisamko091184 added Status: Requires Code Review Needs the core team to code review Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member Branch:Hotfix labels Feb 1, 2023
$value = sugar_substr($value, $vardef['len']);
}

return $value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SinergiaCRM,

Thank you for your PR.

Don't we also want to make sure the decimal separator is a "."? or is that what causes the problem on currencies at the moment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @clemente-raposo,

The code should work the same with any decimal separator. The importSanitize() function returns the field value in the format defined by the user, so we only need to remove the thousands separator symbol set in the configuration.

Changing the values of the thousands separator and decimal separator with different characters, such as ",", ".", "@", "#", etc., and testing the import process could be a useful test. In the tests performed, it has worked correctly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SinergiaCRM, thank you. Makes sense.

Copy link
Contributor

@clemente-raposo clemente-raposo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@clemente-raposo clemente-raposo added Status: Requires Testing Requires Manual Testing Status: Passed Code Review Mark issue has passed code review reviewed and removed Status: Requires Code Review Needs the core team to code review labels Feb 6, 2023
@johnM2401 johnM2401 added Status:Requires Updates Issues & PRs which requires input or update from the author and removed Status: Requires Testing Requires Manual Testing labels Mar 31, 2023
@johnM2401
Copy link
Contributor

Hey!

I've given this some testing, and it seems like the fix does indeed resolve issues when importing new records.

However, I seem to hit an error when trying to Import & Update records.

I've set my User's Thousands Operator to "."
And decimal operator to ","
image


Then, Importing to Update two Opportunities, I received the error:
"A non-numeric value encountered on line 3093 in file [crm]/include/database/DBManager.php"
"A non-numeric value encountered on line 3094 in file [crm]/include/database/DBManager.php"

image


This doesn't seem to happen when importing NEW records, only when updating existing records.

CSV Used attached, for reference:
Opportunitiesz.csv


What are your thoughts?

@SinergiaCRM
Copy link
Contributor Author

Hey!

I've given this some testing, and it seems like the fix does indeed resolve issues when importing new records.

However, I seem to hit an error when trying to Import & Update records.

I've set my User's Thousands Operator to "." And decimal operator to "," image

Then, Importing to Update two Opportunities, I received the error: "A non-numeric value encountered on line 3093 in file [crm]/include/database/DBManager.php" "A non-numeric value encountered on line 3094 in file [crm]/include/database/DBManager.php"

image

This doesn't seem to happen when importing NEW records, only when updating existing records.

CSV Used attached, for reference: Opportunitiesz.csv

What are your thoughts?

Hi @johnM2401,

What versión of PHP, SuiteCRM, MySQL/MariaDB are you using?

@SuiteBot
Copy link

SuiteBot commented Jul 5, 2023

This pull request has been mentioned on SuiteCRM. There might be relevant details there:

https://community.suitecrm.com/t/import-currency-field/34603/5

@jack7anderson7 jack7anderson7 added the Area:Import Issues & PRs related to all things regarding Importing label Aug 31, 2023
@johnM2401 johnM2401 added Status: Requires Testing Requires Manual Testing and removed Status:Requires Updates Issues & PRs which requires input or update from the author labels Oct 16, 2023
Copy link
Contributor

@johnM2401 johnM2401 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay, @SinergiaCRM

I've retested this after merging into the most recent version locally, (7.14), and cannot seem to replicate the issue I've raised above.

After some testing with various combinations of special-character Decimal/Thousands separators, it appears to be working well.

LGTM!

@johnM2401 johnM2401 added Status: Passed Testing and removed Status: Requires Testing Requires Manual Testing labels Oct 16, 2023
@jack7anderson7 jack7anderson7 merged commit 711f8a0 into salesagility:hotfix Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area:Import Issues & PRs related to all things regarding Importing Branch:Hotfix Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member Status: Passed Code Review Mark issue has passed code review reviewed Status: Passed Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when importing currency fields with a decimal separator set to a comma
7 participants