search

twomice / com.joineryhq.jsumfields

Provides additional fields under the Summary Fields framework.
Other
1 stars 10 forks source link

Issue #20 - Add support for data_update_method #21

Open samuelsov opened 3 years ago

samuelsov commented 3 years ago

Before: In /civicrm/admin/setting/sumfields page, upon adding new summary fields from jsumfields, triggers are always created even if we check the box How often should summary data be updated? to When ever the cron job is run (increases performance on large installation) Screenshot 2021-09-02 at 14-02-04 Summary Fields Administration Fondation pour les générations futures

After: If we check the box How often should summary data be updated? to When ever the cron job is run (increases performance on large installation), the triggers are not created and the jsumfields are only updated when the SumFields.Gendata cron job is ran.

twomice commented 3 years ago

Thanks for the PR. @samuelsov can you tell about your "before" and "after" observations with this code change. I can guess already what you'll say, based on the ticket description, but I would value hearing it explicitly from you.

twomice commented 3 years ago

@samuelsov i.e., documenting your "before" and "after" observations would move this more quickly to a merge. Thanks!

samuelsov commented 3 years ago

@twomice yes sorry about that... It's fixed. Let me know if you need more clarifications.

samuelsov commented 3 years ago

@twomice Note that the PR doesn't solves the situation afterwards so you might need to disable the fields and re-enable them to see the difference.

twomice commented 3 years ago

@samuelsov

... PR doesn't solves the situation afterwards so you might need to disable the fields and re-enable them to see the difference.

This would be caused by the fact that the triggers were already created; so by disabling them we remove the triggers, and then when they are re-enabled (with this patch) the triggers are not created.

Right?

samuelsov commented 3 years ago

@twomice Exactly. Ideally, we should add an updater to remove the triggers if the setting is set to "When ever the cron job..."

twomice commented 3 years ago

Good point @samuelsov I'll put some thought into that before merging.