Project

General

Profile

Support #13441

ongoing user problems with daily digests

Added by Marilyn Weber about 1 year ago. Updated about 1 year ago.

Status:
Resolved
Priority name:
Normal
Assignee:
-
Category name:
-
Target version:
Start date:
2020-10-07
Due date:
% Done:

0%

Estimated time:

Description

Lisa Kofod reported:

I am not getting the daily digests for the "MA in Digital Humanities, CUNY Graduate Center" group on the Commons. I do have that radio button set (see screenshot). I do receive emails generated by other groups. Is there an issue with my account, or a problem with this group?

Later she added:
When I visit the Profile > My Settings > Notifications view, I see that the GROUP NOTIFICATIONS section keeps defaulting to NO EMAIL no matter how often I try to apply DAILY DIGEST as a setting (see screenshot).

However, when I check the Profile > My Groups > Memberships view, I have a DAILY DIGEST organized for each of my groups (see screenshot). From what I can see, it reads like the main settings are over-riding the group settings.

I have manually reset the Profile > My Groups > Memberships for the MA in DH group to ALL EMAIL. Let's see how it goes.

And today she writes:
Daily digest fails for all groups. I have had to set permissions to all emails to receive traffic.
This, combined with the fact that the main setting view defaults to no email regardless of the user's attempts to change it, tells me there are either issues with how the permissions system is applied to my account or the daily digest function has been nerfed.

LMK what else I should ask. Thanks!

Kofod 1.PNG (113 KB) Kofod 1.PNG Marilyn Weber, 2020-10-07 03:41 PM
Outlook-4vrfdx2a.png (7.91 KB) Outlook-4vrfdx2a.png Marilyn Weber, 2020-10-07 03:43 PM
Outlook-egieoask.png (50.6 KB) Outlook-egieoask.png Marilyn Weber, 2020-10-07 03:43 PM

Related issues

Related to CUNY Academic Commons - Bug #13442: Group-specific settings at member Settings > Notifications don't work after BPGES updateResolved2020-10-07

Related to CUNY Academic Commons - Bug #13429: Multiple forum notificationsResolved2020-10-05

History

#1 Updated by Boone Gorges about 1 year ago

  • Related to Bug #13442: Group-specific settings at member Settings > Notifications don't work after BPGES update added

#2 Updated by Boone Gorges about 1 year ago

Thanks for the detailed report.

The discrepancy between Profile > My Settings > Notifications and My Groups > Memberships is a red herring. The former (Notifications) was not working properly (and will be fixed when #13442 is deployed as part of the next release) but this bug is unrelated to the user's not receiving digest emails.

There does seem to be something strange happening with the specific user, as I see that even some of the user's immediate notifications ("All Email") are unsent in the queue. I'll see if I can figure it out.

#3 Updated by Boone Gorges about 1 year ago

  • Related to Bug #13429: Multiple forum notifications added

#4 Updated by Raymond Hoh about 1 year ago

I just checked Cavalcade and it appears that the hook for daily digests (ass_digest_event) was not running or queued up. Not sure how the hook got unscheduled or removed.

I've just queued the daily digest for tomorrow at 05:30 GMT.

#5 Updated by Raymond Hoh about 1 year ago

Boone, I ran two queries before the daily digest fired:

1. select count(id) from wp_bpges_queued_items where type = 'dig' and date_recorded < '2020-10-08 05:30:00' - 65526
2. select count(distinct user_id) from wp_bpges_queued_items where type = 'dig' and date_recorded < '2020-10-08 05:30:00' - 627

About an hour after the daily digest fired, I ran the same queries:

1. select count(id) from wp_bpges_queued_items where type = 'dig' and date_recorded < '2020-10-08 05:30:00' - 65420
2. select count(distinct user_id) from wp_bpges_queued_items where type = 'dig' and date_recorded < '2020-10-08 05:30:00' - 627

This means that the daily digest queue stalled and only processed 106 queued items out of 65526. The user count also didn't change so it looks like the digest for that one user did not complete either.

Next, I checked the GES debug log:

[08-Oct-2020 05:30:08] Beginning digest batch of type dig for timestamp 2020-10-08 05:30:05.

And the Cavalcade log:

$ wp cavalcade log --hook=ass_digest_event
+--------+------------------+---------------------+-----------+
| job    | hook             | timestamp           | status    |
+--------+------------------+---------------------+-----------+
| 625212 | ass_digest_event | 2020-10-08 05:30:16 | completed |
+--------+------------------+---------------------+-----------+

It appears that the digest blast ran for only 11 seconds.

Based on this, I started looking at the queue process code and have the following observations / suggestions:

1. BPGES_Async_Request_Send_Queue::time_exceeded() always returns true when run from CLI. Meaning after the first loop, the queue bails and doesn't run again. I've posted some thoughts here. Perhaps we can also add set_time_limit( 0 ) when the queue starts being processed around here if we're running from WP-CLI.

2. We should remove all items in wp_bpges_queued_items older than October 2020. This is to prevent the queued digest query from becoming too big and having too many older entries that are not applicable.

3. To prevent the queued digest query from becoming too big again, for BPGES_Queued_Item_Query::get_results() and BPGES_Queued_Item_Query::get_users_with_pending_digest(), we should do what you recommended here. Basically, setting a prior limit for daily digests and weekly summaries. You mention three days for daily digests and three weeks for weekly summaries. Perhaps set it to two days for daily digests and 1.5 weeks for weekly summaries?

#6 Updated by Boone Gorges about 1 year ago

Thanks for helping to debug this mess, Ray.

1. BPGES_Async_Request_Send_Queue::time_exceeded() always returns true when run from CLI. Meaning after the first loop, the queue bails and doesn't run again. I've posted some thoughts here. Perhaps we can also add set_time_limit( 0 ) when the queue starts being processed around here if we're running from WP-CLI.

Agreed. I've made the change in BPGES and pushed the latest to the production site. https://github.com/cuny-academic-commons/cac/commit/f638a18e04596adf8e2a176aa8126ec711391caa

2. We should remove all items in wp_bpges_queued_items older than October 2020. This is to prevent the queued digest query from becoming too big and having too many older entries that are not applicable.

Agreed. I've done this.

3. To prevent the queued digest query from becoming too big again, for BPGES_Queued_Item_Query::get_results() and BPGES_Queued_Item_Query::get_users_with_pending_digest(), we should do what you recommended here. Basically, setting a prior limit for daily digests and weekly summaries. You mention three days for daily digests and three weeks for weekly summaries. Perhaps set it to two days for daily digests and 1.5 weeks for weekly summaries?

Let's hold off on this for a moment while we see what happens with changes we've already put in place.

Marilyn, please let the user know that we're looking into digest issues across the system.

#7 Updated by Marilyn Weber about 1 year ago

Will do, thanks!

#8 Updated by Marilyn Weber about 1 year ago

The user (a very nice DH student) writes "I only tested the daily digests and all mail options. I hope they are
also testing to learn if weekly digests have been disabled."

#9 Updated by Boone Gorges about 1 year ago

Thanks for the update. Weekly digests work based on the same logic as daily digests, so what fixes one should fix the other.

#10 Updated by Raymond Hoh about 1 year ago

Thanks for making those changes, Boone.

I'll check in on the daily digest after it runs in the early morning and report back.

#11 Updated by Marilyn Weber about 1 year ago

Fantastic and thank you both! Please update when you can and I'll pass it on.

#12 Updated by Raymond Hoh about 1 year ago

Just checked the GES log.

[09-Oct-2020 05:30:06] Beginning digest batch of type dig for timestamp 2020-10-09 05:30:04.
[09-Oct-2020 05:30:24] Sent dig digests to 176 users as part of this batch. Launching another batch....
[09-Oct-2020 05:30:25] Beginning digest batch of type dig for timestamp 2020-10-09 05:30:04.
[09-Oct-2020 05:30:32] Finished digest run of type dig for timestamp 2020-10-09 05:30:04. Digests were sent to a total of 250 users.

Looks like we're good!

However, when I run the select count(id) from wp_bpges_queued_items where type = 'dig' and date_recorded < '2020-10-09 05:30:00' query, there are still a lot of entries. Before the digest ran, there were 6962 rows; after the digest completed, there were 4660 rows. The date_recorded rows for the remaining entries are from October 1 to 6 though, however I don't see where we are enforcing a prior limit in BPGES. Any ideas, Boone?

#13 Updated by Boone Gorges about 1 year ago

However, when I run the select count(id) from wp_bpges_queued_items where type = 'dig' and date_recorded < '2020-10-09 05:30:00' query, there are still a lot of entries. Before the digest ran, there were 6962 rows; after the digest completed, there were 4660 rows. The date_recorded rows for the remaining entries are from October 1 to 6 though, however I don't see where we are enforcing a prior limit in BPGES. Any ideas, Boone?

BPGES filters out "invalid" entries when assembling the digest. See https://github.com/boonebgorges/buddypress-group-email-subscription/blob/4.0.x/bp-activity-subscription-digest.php#L181. One of these validity checks is for staleness, which uses the logic we discussed earlier in this thread, but which I forgot already existed :) So these items will never be added to any future digests, and it should be safe to delete them. Does that seem right to you too?

#14 Updated by Raymond Hoh about 1 year ago

Fantastic and thank you both! Please update when you can and I'll pass it on.

Marilyn, can you ask Lisa if she received a daily digest over the past few days?

One of these validity checks is for staleness, which uses the logic we discussed earlier in this thread, but which I forgot already existed :) So these items will never be added to any future digests, and it should be safe to delete them. Does that seem right to you too?

I missed the bp_ges_activity_is_valid_for_digest() function, Boone! Should be safe to delete the stale items.

Is there a reason for having the stale check in that function rather than in BPGES_Queued_Item_Query::get_results() and BPGES_Queued_Item_Query::get_users_with_pending_digest()? Is it for individual activity filtering?

#15 Updated by Marilyn Weber about 1 year ago

I asked yesterday, no reply yet but am checking.

I got a digest for a site I'd forgotten I signed up for, so I guess it's working!

#16 Updated by Matt Gold about 1 year ago

Marilyn Weber wrote:

I got a digest for a site I'd forgotten I signed up for, so I guess it's working!

Same here!

#17 Updated by Boone Gorges about 1 year ago

  • Status changed from New to Resolved
  • Target version set to 1.17.5

Is there a reason for having the stale check in that function rather than in BPGES_Queued_Item_Query::get_results() and BPGES_Queued_Item_Query::get_users_with_pending_digest()? Is it for individual activity filtering?

Yes, I believe so. If the timestamp filtering happened as part of the query, then items that didn't show up in query results (because they were too old) would never hit the _valid_ filter, which would make it more complicated for plugins to override the default BPGES logic for those items. This way, all filtering happens in a single function, so it's easy to read and modify.

However, it certainly wouldn't be impossible to customize the logic if we were to move the timestamp checks. It would look something like:

1. Add an `after` param (or something similar) to BPGES_Queued_Item_Query
2. In get_users_with_pending_digest(), pass 'after' to get_results(). 'after' would be calculated using similar logic to what's currently at https://github.com/boonebgorges/buddypress-group-email-subscription/blob/4.0.x/bp-activity-subscription-digest.php#L978 (and the corresponding logic in bp_ges_activity_is_valid_for_digest() would be removed)
3. Then, in a plugin, if you find that certain items aren't turning up in the query because of the staleness, you'd have to filter bp_ges_stale_activity_period before the items would be included in the bp_ges_activity_is_valid_for_digest filter.

The main advantage, as you note, would be that very old items would never be pulled during get_results(), reducing the overhead needed to run batch operations. My hunch is that this overhead, as things stand, is not very significant, especially as other bugs related to digests are resolved (since they result in fewer items remaining unsent), so it's not worth the effort to make the change. But I could be persuaded otherwise :-D

For the purposes of this ticket, it seems as if our changes have had the desired effect, so I'm going to mark Resolved.

Also available in: Atom PDF