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

[REF] Follow up fix to fixing note entity tables in views #144

Merged
merged 1 commit into from
Jul 7, 2021

Conversation

seamuslee001
Copy link
Contributor

@civibot
Copy link

civibot bot commented Jul 7, 2021

(Standard links)

@civibot civibot bot added the 1.x-master label Jul 7, 2021
Comment on lines 1191 to 1192
$noteEntityTables = civicrm_api3('OptionValue', 'get', ['option_group_id' => 'note_used_for'])['values'];
foreach (array_values(CRM_Utils_Array::collect('value', $noteEntityTables)) as $entityTable) {
Copy link
Member

Choose a reason for hiding this comment

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

I think both the before and after versions are incorrect because they access lower-level data instead of doing a pseudoconstant lookup and letting that layer worry about where the data is coming from. If it had been doing this from the beginning, we wouldn't need any change here:

Suggested change
$noteEntityTables = civicrm_api3('OptionValue', 'get', ['option_group_id' => 'note_used_for'])['values'];
foreach (array_values(CRM_Utils_Array::collect('value', $noteEntityTables)) as $entityTable) {
$noteEntities = civicrm_api3('Note', 'getoptions', ['field' => "entity_table"])['values'];
foreach (array_keys($noteEntities) as $entityTable) {

@seamuslee001 seamuslee001 force-pushed the fix_note_entity_table branch from 91e1d07 to fe0c92c Compare July 7, 2021 22:09
@seamuslee001
Copy link
Contributor Author

@colemanw I have updated this as per your suggestion now

@colemanw colemanw merged commit f7195c5 into civicrm:1.x-master Jul 7, 2021
@seamuslee001 seamuslee001 deleted the fix_note_entity_table branch September 27, 2021 21:46
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