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

CRM-17647 fix for submitting payment with thousand separator #11548

Merged
merged 2 commits into from
Jan 22, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jan 18, 2018

Overview

rc version of #11539

Note this does not cover all instances of mishandling the thousand separator - I believe they are well identified in #18839 but I am working through them

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

@agh1 @colemanw @mlutfy test fail unrelated - fix required

@eileenmcnaughton
Copy link
Contributor Author

also #11549 - there are some other places that need fixing for this bug but depend on this being merged

@eileenmcnaughton eileenmcnaughton force-pushed the money_rc branch 2 times, most recently from 2ffa425 to 161d506 Compare January 19, 2018 01:58
@eileenmcnaughton
Copy link
Contributor Author

@agh1 @alifrumin you might like to give this a spin - it fixes some identified paths but there are more identified through the tests - although I probably won't work on those until this is merged

@eileenmcnaughton
Copy link
Contributor Author

test fail unrelated

@eileenmcnaughton
Copy link
Contributor Author

test this please

1 similar comment
@seamuslee001
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor Author

@agh1 @colemanw @bgm this has passed

@agh1
Copy link
Contributor

agh1 commented Jan 20, 2018

@eileenmcnaughton this is awesome--thanks! I was out sick and haven't had a chance to try it out, but @alifrumin and I will bang on it on Monday with partial payments, Spanish numbers, and anything else we can think of.

@eileenmcnaughton
Copy link
Contributor Author

@agh1 - it fixes one path but there are others that don't work so don't see 'no broken places' as the bar for this fix

@eileenmcnaughton
Copy link
Contributor Author

@agh1 are you looking at this today - just want to keep it moving because once this is merged I'm going to test for other places again (#11539 throws errors on 'risky paths' but then I have to dig into each one)

@abihazakariya
Copy link

@eileenmcnaughton, @agh1 and I tried the patch out and it is working as expected on both fronts: delimiters and partial payments.Thanks so much!

@eileenmcnaughton
Copy link
Contributor Author

OK - I'm going to merge based on that (thank you for testing)

@agh1 I will also merge up to master - would still like you to keep reviewing & kicking the cans more on this

@eileenmcnaughton eileenmcnaughton merged commit d25fed1 into civicrm:4.7.30-rc Jan 22, 2018
@eileenmcnaughton eileenmcnaughton deleted the money_rc branch January 22, 2018 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants