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] Check subtype validity in validate rather than wait for 'import', test #23491

Merged
merged 1 commit into from
May 20, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

[Import] Check subtype validity in validate rather than wait for 'import', test

Before

Validate routine does not check for contact subtype validity - but it will (often) fail on import

After

getParams does the mapping to ensure we have a name & then this can be checked during validation

Technical Details

Test cover added

Comments

@civibot civibot bot added the master label May 18, 2022
@civibot
Copy link

civibot bot commented May 18, 2022

(Standard links)

@eileenmcnaughton eileenmcnaughton force-pushed the import_sub branch 7 times, most recently from 5b20d04 to 5f6ad41 Compare May 19, 2022 00:22
@eileenmcnaughton
Copy link
Contributor Author

test this please

* @throws \CRM_Core_Exception
* @throws \Civi\API\Exception\UnauthorizedException
*/
private function validateMultiRowCsv(string $csv, array $mapper, string $field): void {
Copy link
Member

Choose a reason for hiding this comment

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

Since the assertion here seems new+important, I figured I'd kick the tires on it. If I randomly throw in bad data, it complains. So that's good. :)

I'm not sure I understand the signature. $csv makes sense as filename. What are $mapper and $field? (Docblock would be good - but doesn't have to be on this PR specifically.)

Copy link
Contributor Author

@eileenmcnaughton eileenmcnaughton May 19, 2022

Choose a reason for hiding this comment

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

mapper is pretty integral to the concept of these import forms - as the array that is submitted as the mappings - it's documented in quite a few places - I can perhaps find a 'see'. I'm starting to think $field might get removed again in later tests as I start to build on it.

@eileenmcnaughton
Copy link
Contributor Author

@totten just checking back on this since you seem to be onto something new now - was anything blocking here?

@eileenmcnaughton
Copy link
Contributor Author

this is stale - rebasing now

@eileenmcnaughton
Copy link
Contributor Author

@totten I updated the comments about 'mapper' - I am surprised my comment updates got through jenkins first go - 1 to me

@eileenmcnaughton
Copy link
Contributor Author

This needs another rebase - doing now

@eileenmcnaughton eileenmcnaughton force-pushed the import_sub branch 2 times, most recently from 0dcdd7d to a165be6 Compare May 20, 2022 03:31
@eileenmcnaughton
Copy link
Contributor Author

Since this keeps going stale I've pulled out the comments fix here - #23517 in the hope I can get that merged quickly.

I'm also going to pull out some of the bits in this PR that keep going stale / interact with other PRs

@eileenmcnaughton eileenmcnaughton force-pushed the import_sub branch 4 times, most recently from 1c5a23d to 4eee6b7 Compare May 20, 2022 06:38
@eileenmcnaughton
Copy link
Contributor Author

still dealing with conflicts from other PRs - I split off another small chunk to #23523

@demeritcowboy demeritcowboy merged commit 5e71c13 into civicrm:master May 20, 2022
@eileenmcnaughton eileenmcnaughton deleted the import_sub branch May 20, 2022 19:14
@eileenmcnaughton
Copy link
Contributor Author

thanks @demeritcowboy this looks like the pr doesn't have all the stuff in it it used to - did I lose it in my rebasing

@demeritcowboy
Copy link
Contributor

Actually it wasn't this one I merged. Github is just displaying it that way because after other merges this must have been what was left, and then when I merged that other PR it knows there's nothing left here.

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy yeah - I was excited it was merged - but in all the rebasing I think the meaningful commits fell out - will re-do

@eileenmcnaughton
Copy link
Contributor Author

OK - I think this was the remaining commit #23526

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.

3 participants