-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
dev/core#1725 Remove groups, tags and notes from the returned data fo… #17444
Conversation
(Standard links)
|
@seamuslee001 so the issues are
|
@eileenmcnaughton I'm not proposing to completely remove them from primary fields, I'm just proposing to essentially exclude them for the purposes of generating the Preview of data. The problem is that for sites such as AUG and maybe even Pete's sites having a large number of groups and or tags or notes really slows down the export process which is why AUG has actually disabled the export by primary fields option on exports. This retains the standard field set if someone does an export of primary fields but when generating the content for the preview it doesn't generate any content for the groups or tags or notes fields if they are selected. |
@seamuslee001 Ah ok - & what about when it 's not primary fields only - should we preview them then? |
@seamuslee001 FYI the sites @petednz had that I looked at had searchPrimaryOnly = FALSE |
@eileenmcnaughton this preview of data is only shown on the select fields screen when you have selected to not use the primary fields export, The problem is that because NULL is the 2nd param to the ExportProcessor constructor it defaults to using the default return params which for a Contact Export have groups and tags etc included. So all I am doing is manually manipulating the return params for the purpose of the preview to ensure that those fields are removed I know AUG have searchPrimaryOnly = TRUE and still ground to the knees |
@seamuslee001 @eileenmcnaughton Thanks for the PR. Have applied this on the site with search primary only = no. With the above setting unchanged, this improves the time involved in export when the option "select fields to export" is chosen.
I tried to force the primary option by applying #17458 PR to the site. The query formed do have the condition of table.is_primary = 1 but it still takes too long to complete the process. Full Query -
This is while searching for contacts with activity type = x on advanced search and then export. |
so @jitendrapurohit @eileenmcnaughton For the Australian Greens the situation is / was this : Prior: After: I would say given your report @jitendrapurohit that the time taken for the select fields is improved with this PR I would suggest merging this |
@colemanw any thoughts? I would note that the preview seems to be called in preProcess which cases it to be called an extra time on postProcess Note @jitendrapurohit's query is actually the full query - not the preview query |
The preview issue only came up since we merged in the new ExportUI. Its probably not hitting heaps of sites but just the ones where groups are a real performance point |
…r the usage in preview data to speed up loading of the select fields screen
ef25574
to
726c191
Compare
@eileenmcnaughton as discussed I have now put this against 5.26 |
Jenkins re test this please |
3 similar comments
Jenkins re test this please |
Jenkins re test this please |
Jenkins re test this please |
Test fail unrelated I am going to merge this based on @jitendrapurohit 's testing showing that this improves performance on the select fields for export screen which is what this is targeting and that discussion with Eileen https://chat.civicrm.org/civicrm/pl/aytebtxgti8y5851jkcrmdu1fa says that she is fine for this to be merged into the RC |
foreach ($defaultProperties as $key => $var) { | ||
if (in_array($key, ['groups', 'tags', 'notes'])) { | ||
unset($defaultProperties[$key]); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the purpose of this foreach loop. You are just unsetting 3 keys from an array. FYI unset()
does not raise any error if the key isn't there. I think this could be simplified to:
foreach ($defaultProperties as $key => $var) { | |
if (in_array($key, ['groups', 'tags', 'notes'])) { | |
unset($defaultProperties[$key]); | |
} | |
} | |
CRM_Utils_Array::remove($defaultProperties, 'groups', 'tags', 'notes'); |
@colemanw note we should re-visit this in master - I felt we needed a quick fix for now but I think at least a place holder should be present |
…r the usage in preview data to speed up loading of the select fields screen
Overview
This aims to speed up the generation of the select fields for export screen after doing an advanced search by removing the groups, tags and notes return properties
Before
slow to load the select fields for export screen due to slow joins / select query
After
Faster to load the screen
ping @eileenmcnaughton @jitendrapurohit