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

Allow configuring required address comonents #48

Merged
merged 8 commits into from May 2, 2022

Conversation

MarcMichalsky
Copy link
Contributor

This is a real KISS solution. And to be honest, I am not exceedingly proud of it. But it works and I think its simple nature makes it easily understandable for everyone.

I understand this PR as a voluntary act of mine to improve this cool open source software. It's just an offer and I do not demand you to pull it. So, there's no need to send a bill. 😬

fix #47

@MarcMichalsky
Copy link
Contributor Author

What's that part for exactly?

if ($address_param != $address_component) {
unset($params[$address_param]);
}

@jensschuppe
Copy link
Collaborator

This removes e.g. the user_street element from the $params array, since its value is being set to the internal CiviCRM parameter name (street_address) and the original parameter (user_street) is not needed anymore.

@MarcMichalsky
Copy link
Contributor Author

MarcMichalsky commented Mar 10, 2021

This removes e.g. the user_street element from the $params array, since its value is being set to the internal CiviCRM parameter name (street_address) and the original parameter (user_street) is not needed anymore.

But the condition is always true because the array is hard coded.

EDIT: Okay, I got it, it's always true but for the gender_id.

@jensschuppe
Copy link
Collaborator

But the condition is always true because the array is hard coded.

It is. I think I imagined new parameters could have the same name and the code would be prepared for it. Consider it a foresighted clean-up ;-)

@jensschuppe jensschuppe added the status:needs review Code needs review and testing label Mar 25, 2021
@bjendres
Copy link
Member

@jensschuppe, any objections to pulling this? It looks sensible and @mariav0 confirmed that it works for her... And afaik there's no other address parameters submitted by twingle.

@bjendres bjendres added the bug Something isn't working label May 19, 2021
@jensschuppe
Copy link
Collaborator

@bjendres I haven't merged it yet, because

  • it changes current behavior (someone might rely on addresses containing country only)
  • we should therefore think about making this configurable per profile
  • we should maybe flip the logic (if nothing else than country instead of not everything but country - with "everything" being hard-coded; i.e. use an array_diff() or similar)
  • it does not check for empty address parameters (only for whether they exist with array_key_exists()

@MarcMichalsky
Copy link
Contributor Author

@jensschuppe

it does not check for empty address parameters (only for whether they exist with array_key_exists()

As far as I know, Twingle does not submit empty fields. But I can check this, if you want.

@MarcMichalsky
Copy link
Contributor Author

Okay, I checked it: Twingle does submit empty address data fields. So we should include another check

@jensschuppe
Copy link
Collaborator

@MarcMichalsky Could you rebase this on the current master since #53 changed some code in the TwingleDonation.submit API code and I'd like that to not interfere with your work.

@MarcMichalsky
Copy link
Contributor Author

Okay, that should be it.
I have made the required address components configurable in the profile.

The check for empty address parameters is already done here:

foreach (array(
'user_street' => 'street_address',
'user_postal_code' => 'postal_code',
'user_city' => 'city',
'user_country' => 'country',
) as $address_param => $address_component) {
if (!empty($params[$address_param])) {
$params[$address_component] = $params[$address_param];
if ($address_param != $address_component) {
unset($params[$address_param]);
}
}
}

@@ -347,7 +347,12 @@ function civicrm_api3_twingle_donation_Submit($params) {
}
}
if ($unset_address_params) {
foreach($required_address_components as $req) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I had a knot in my brain here. 🧠

@jensschuppe jensschuppe changed the title Don't create new address only from user_country Allow configuring required address comonents May 2, 2022
@jensschuppe jensschuppe merged commit d95813d into systopia:master May 2, 2022
@jensschuppe
Copy link
Collaborator

I did some slight changes in 6779349 and merged to master. This should be backwards-compatible due to the new configuration option. However, it defaults to requiring street, postal code, city and country for new profiles, but that seems reasonable to me.

@MarcMichalsky you might want to re-run your tests on that, but the code looks good to me. I went ahead merging it in order to move on towards a pre-release.

@jensschuppe jensschuppe added this to the 1.3 milestone Apr 5, 2024
@jensschuppe jensschuppe added status:fixed The issue has been resolved (usually by committing/merging code) and removed status:needs review Code needs review and testing labels Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working status:fixed The issue has been resolved (usually by committing/merging code)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not use user_country as the only address parameter for XCM
3 participants