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

Fix master-only regression on showing fields for contact type #23329

Merged
merged 1 commit into from
May 2, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Apr 30, 2022

Overview

Fix master-only regression on showing fields for contact type

Before

Master-only regression whereby contact-type-specific fields were not being offered up for mapping for Organization and Household

After

Back to the future

Technical Details

The issue was the available fields were being calculated before the contact type was available. There is definitely an intent to be able to get this data withough going through the run function which is is a jack-of-all-trades-and-a-master-of-none so I added a funciton to do that - using the userJobID as the source of information for contactType

I also hit an uncaught exception - I added a catch for it but it is not quite being handled consistently with other errors as their handling will change in #23292

Comments

@demeritcowboy tthis is a fix for the issue you found

@civibot
Copy link

civibot bot commented Apr 30, 2022

(Standard links)

*/
protected function getContactType() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to the parent

protected function getContactType() {
return $this->_contactType ?? 'Individual';
public function getAvailableFields(): array {
$this->setFieldMetadata();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

setFieldMetadata means we don't need to call run or init first - but I think this will evolve as we clean up the inputs on construct

$importer->init();
$importer->_contactType = $this->getContactType();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that per class code comments - this function is only called from unit tests

@demeritcowboy
Copy link
Contributor

Thanks I'll try to take a look tomorrow. Or do you prefer the "names" one first?

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy I guess either is fine - but I'll put up a PR with both in case you want to test all together

@demeritcowboy
Copy link
Contributor

Noting separately that there's all kinds of "undefined offset" errors coming up, and that clicking cancel on the map fields page when doing contact import always takes you to Activity Import instead of back to Contact Import.

@eileenmcnaughton eileenmcnaughton deleted the import_getfields branch May 2, 2022 01:57
@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy I didn't replicate the activity import bounce just now - the undefined errors I'm seeing are mostly on the preview page when there is only one row - and the 'isCheked' one that I haven't thought of a good fix for

@demeritcowboy
Copy link
Contributor

Ok the bounce seems semi-random. Sometimes it goes to the home page, sometimes it goes somewhere else. It seems to depend where you've visited, but not the immediate page you visited before, just something chosen from your past. I'm thinking now it's not specific to this.

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy yeah I wound up at search -

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants