CRM-19621 fix address field to show country & state. #9466
Conversation
eileenmcnaughton
force-pushed
the
address
branch
from
b5b3d8c to
07df8b0
Compare
|
@eileenmcnaughton this produces the following output on the confirm page Seamus lee |
|
@seamuslee001 can you tell why - it is converting for me ... although on the last page I am getting abbreviations rather than full |
eileenmcnaughton
force-pushed
the
address
branch
2 times, most recently
from
64c5997 to
3ef64c9
Compare
| $addressFields[$name] = CRM_Core_PseudoConstant::stateProvince($value); | ||
| } | ||
| if ($name == 'country') { | ||
| $addressFields[$name] = CRM_Core_PseudoConstant::country($value); |
There was a problem hiding this comment.
This displays name instead of abbreviations for me too, maybe it should be stateProvinceAbbreviation() and countryIsoCode() call ?
There was a problem hiding this comment.
The test fail relates to the same behavour as I and @seamuslee001 are encountering?
|
@eileenmcnaughton While working on QA, I see that names don't get displayed on the Confirm page if Anyway, I agree with the concern that this may be more safer than the original PR as it works for Seamus too. If the above can be considered as trivial to be ignored, we can go ahead in merging this PR :-) |
eileenmcnaughton
force-pushed
the
address
branch
2 times, most recently
from
3e91c07 to
3986ed4
Compare
|
Tested latest update from Eileen and fixes my issue with address state not being converted to abbreviation :-) |
|
Jenkins re test this please |
| @@ -360,6 +360,15 @@ public static function validateCreditCard($values, &$errors, $processorID = NULL | |||
| * @param bool $reverse | |||
| */ | |||
| public static function mapParams($id, $src, &$dst, $reverse = FALSE) { | |||
| // Set text version of state & country if present. | |||
There was a problem hiding this comment.
@seamuslee001 can you check if it still works if I remove these lines again - my theory is they can go again now...
There was a problem hiding this comment.
hmm now I'm a bit torn - do I introduce more risk if I remove them now? Probably not since @jitendrapurohit was ready to remove them before. Will wait on Jitendra's opinion
There was a problem hiding this comment.
@eileenmcnaughton Per my comments on #9399 (comment), I don't really think that removing this part will introduce any risk as it was moved from Confirm.php to here fixing some e-notices for country and state.
What makes me think is it has also been removed from Confirm.php now. But I've tested on the confirm and thankyou page and I didn't notice any errors. Also it has been tested by @seamuslee001, so I think we are good to go with the removal.
|
@jitendrapurohit can you confirm whether my latest still fixes "Most things but not Payment Express". Depending on how thoroughly you test you might be able to confirm my theory that we no longer need those lines in the mapParams function. There is a general need to keep testing different scenarios in 4.7 due to the changes in #9390 - I'm about to work on a bug in the receipt Karin identified. I think maybe variants on recurring membership & separate payment probably still need testing. Just mentioning all of this in case you can tie it into working on related issues.... Also, do you think we should put that price set js fix you did for Karin into the rc? |
|
@eileenmcnaughton I've tested this using the latest changes and it seems to be working fine except PayPal Express. As we see the This can be fixed in mapParams(which seems to be riskier as it gets called on multiple places), can we add an extra check of |
eileenmcnaughton
force-pushed
the
address
branch
from
3986ed4 to
9c86599
Compare
|
@jitendrapurohit I have altered the patch per your previous suggestion - I also removed the lines from mapParams - but maybe they DO need to be there? |
|
@eileenmcnaughton I think this condition should be enclosed within brackets as it always evaluates to I've updated the PR with the commit. Can you please verify if it seems fine to you ? |
|
test this please |
| @@ -359,7 +359,7 @@ public static function getFormattedBillingAddressFieldsFromParameters($params, $ | |||
| $value = $params[$alternateName]; | |||
| } | |||
| } | |||
| if (is_numeric($value) && $name == 'state_province' || $name == 'country') { | |||
| if (is_numeric($value) && ($name == 'state_province' || $name == 'country')) { | |||
There was a problem hiding this comment.
ah yes! My bad
|
Jenkins re test this please |
|
Unrelated fail |
@jitendrapurohit what about this - it reverses the form changes & moves all the handling to the format function (which we would ideally add a unit test for since that one is unit-testable)