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

Import locations - code cleanup, add tests, fix bugs #23563

Merged
merged 8 commits into from
May 27, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented May 24, 2022

Overview

Import locations - code cleanup, add tests, fix bugs

Before

After

Getting closer to the goal of

  • transformations & validity checks in getParams
  • validation bounces in validateValues
  • formatting done in only one function.... early on. Less formatting needed as getParams is doing a lot of it
  • consistency between main contact & related contacts
  • Country fixed - state & county need this merged before I can work on them

Technical Details

Comments

In order to help with testing there are a bunch of csvs that I use over here - https://github.com/eileenmcnaughton/testdata/tree/master/csvs

Specifically useful to this PR is https://github.com/eileenmcnaughton/testdata/blob/master/csvs/individual_locations_with_related.csv

Along with javascript to create a mapping to get started....

https://github.com/eileenmcnaughton/testdata/blob/master/csvs/individual_locations_with_related.csv.js

(The js can be pasted directly into a Chrome or Firefox console - ie right-click inspect to get the console)

@civibot
Copy link

civibot bot commented May 24, 2022

(Standard links)

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

last fails were unrelated (concurrency tests) - now falling over due to guzzle? test this please

This fixes a couple of bugs I found writing tests & adds
test cover for website & phone imports + cleanup

Notably - creating a phone with a phone_ext or an email
with signature_text doesn't work prior to this
@colemanw
Copy link
Member

OK, this is tested and fixes a regression.

@colemanw colemanw merged commit 8465545 into civicrm:master May 27, 2022
@colemanw colemanw deleted the import_location_all branch May 27, 2022 20:05
@eileenmcnaughton
Copy link
Contributor Author

thanks @colemanw - that unblocks me! I'll work on the states

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.

2 participants