USGS-WiM / whispersservices

Django services for WHISPers
2 stars 1 forks source link

"All Events" updated option not triggering notifications #524

Closed JChipault closed 2 years ago

JChipault commented 2 years ago

I've been noticing for the past few weeks that I don't seem to be getting all the standard notifications that I expect and Neil helped me figure out the likely pattern - looks like notifications for updates on "All Events" aren't firing. Here's my notification preferences:

image

Per Neil "The last time you were sent a notification (or at least a notification was generated) for the 'All events' updates was on 11/30, however at least 42 events have been updated since (see attached). The standard notifications you did get were for 'Your organization'."

I do seem to be getting "All Events" notifications when a new event is created.

Update_events_20211207.xlsx

aaronstephenson commented 2 years ago

Fixed in 6daff97. The problem was that I accidentally used modified_date instead of created_date in the all events (updated) query (line 893). Note that this only affected users with admin or superadmin roles.

I just pushed the fix to the test environment. @JChipault Let me know when you want it pushed to the production environment.

JChipault commented 2 years ago

Sweet! By "Note that this only affected users with admin or superadmin roles."... do you mean those are the only users that had the "updates" for "All Events" set to trigger notifications? If so, I follow. But in the future (now that this bug is fixed) if a partner user, say, set their preferences to receive updates on "All Events", they would get these notifications, correct? Put another way, it's not that only admins and superadmins can get this type of notification, it's just by chance that those are the only roles that had this type of notification turned on.

aaronstephenson commented 2 years ago

Let me restate: This bug only affected admins and superadmins. Other users were still getting the All Updated Events notifications.

JChipault commented 2 years ago

I see. Thanks! Yea, let's go ahead and push it to production

aaronstephenson commented 2 years ago

Release 2.1.10 d754b41 pushed to production.

JChipault commented 2 years ago

Got some weirdness overnight. I received both the "new" and "updated" All Events notifications for 201704 that was created yesterday but I only got the "new" notification for 201705 - no "updated" for All Events for 201705.

aaronstephenson commented 2 years ago

This is a tricky one. I've been debugging it a while and have a lead, but not a solution yet.

In my IDE debugger as I step thru the code, I see the history records for Event 201705 have the wrong date. Here's what it looks like in the debugger: image Here's what it looks like in the DB table: image

Notice that for history_id 37552, the DB table has a history_date of 2021-12-20 21:04:01.232116-06, while the debugger has a history_date of 2021-12-21 03:04:01.232116+00:00. At the moment I have no idea why the debugger has a history_date 6 hours ahead of the DB value, but I suspect that the code is attempting to adjust the timezone to get the UTC date value, and because the edits were done after 9pm local time, they appear as the next day in UTC, and that is causing the "get changes made yesterday" section of the code to be skipped, which results in no email about updates for Event 201705, and only the email about the create.

I'll keep digging to figure out why this is happening, and how to fix it.

aaronstephenson commented 2 years ago

I looked into this some more today, and I still can't explain why the simple_history module is ignoring the stored time zone information for datetimes. There was an issue about this in their github repo 6 years ago that was closed immediately, with the maintainer saying that the module should handle time zone correctly if the Django project settings have USE_TZ=True and a valid value for TIME_ZONE, which we have been doing since early in this project (see here and here).

So, I have implemented a workaround in 368eaca, where I use the time zone value from the settings to adjust the datetime that the simple_history module is reading incorrectly (see here). I pushed this workaround to the test environment, so give it a look and let me know if you want to promote it to production @JChipault .

We could also try reaching out to the simple_history module maintainer, if you're interested. I'd be in favor of doing that, because I would like an acknowledgement of this bug in their code and an official fix instead of a workaround.

JChipault commented 2 years ago

@nbaertlein is on leave for the holidays but I'll let him weigh in on next steps here. Thanks, Aaron!

nbaertlein commented 2 years ago

I'm trying wrap my head around the issue and started digging into from the data side. So far a couple questions/comments I have: 1.) Is 201705 the error, or is 201704? Did we need 2 emails (new & update) for 201704 when 'updated' is checked for all notifications? Seems just one (new) would suffice and I thought we had programmed to send just one in these cases. I need to do some digging to see if one or two notifications are sent for new events when 'Your events', 'Your org events', and 'Your collaborator events' notifications are checked as 'updated' as the logic should be consistent.

2.) If the system truly viewed 201705 as being updated the following day (and subsequently excluded from notification), wouldn't it get picked up on the next evening's scheduled check?

3.) I'd like to understand a little better before we expend a lot of time fixing. In addition to answering the above, I'd like to monitor the data a bit to see if notifications are being generated when expected. There may be some broader patterns there that could be helpful.

Will talk more in January. Thank you!

aaronstephenson commented 2 years ago

I'm away from my computer, for your question #2, yes, the updates would get picked up for the next day in a new notification.

JChipault commented 2 years ago

Regarding 1, I'm pretty sure that they do always come in a pair now - i.e., if someone creates an event and box "new" and "updated" are checked on notification settings, then you get both a New and Update notification, separately. For example, for 201702 in my org, I got a new and updated notification when Shelby first created the event and also an update notification from that same day because Dan updated the event that Shelby created. I haven't combed through other data though. I think we decided to do that on purpose, at least until we get some sort of digest going, because otherwise you might question if you're missing an update. Also, the "new" notifications just say that somethings was created, while the "updated" notifications tell you what data were plugged in... so if we were only to get one or the other, I'd want the "updated" but I'd want it labeled in some way within that email to indicate that this is a new event.

new doesn't give me much info, but does flag that there's a whole new event logged: image

this updated email gives me more information about the event Shelby created, but the "An Event was created" is pretty buried: image

nbaertlein commented 2 years ago

For Question 1: I think I understand better now. -If 'updated' is checked AND 'new' is checked, the user will get BOTH update and new notifications for a newly created event. -If 'updated' is checked and 'new' IS NOT checked, the user will receive ONLY an update notification for a newly created event.

This sounds fine to me. I was thinking there was a possibility of getting both the update and new notifications if only 'updated' was checked, but I don't think this is the case.

For Question 2: We received a new notification for 201705 as expected (event created 12/20, notification generated 12/21), but no update notification (assuming Jenny had 'updated' checked) at any time. If the system was misinterpreting time as the next day, Jenny should had have at least received notification the day after expected (12/22). I suspect however, the time interpretation is still the cause of the issue as it looks like late event creations are not producing update notifications for newly created events as seen for 201705 and 201698 here:

image

Disclaimer: I have only checked Jenny's notifications for 'All events' under the assumption that both 'new' and 'updated' were checked. I have not checked other users and other notifications (e.g., Your Org's events) as this gets a little more complicated to see which boxes were checked (update, new, none) at the time of the notification generation.

Reference: (sql for the above: select event_id, event_creation_date, event_creation_time, subject, count(*) as notification_count, sum(case when notification_type = 'create' then 1 else 0 end) as create_notification_count, sum(case when notification_type = 'update: event creation' then 1 else 0 end) as update_create_notification_count from (select a.id, a.event_id, case when body like '%was updated on%An Event was created%' then 'update: event creation' when body like '%was created on%' then 'create' else 'update: event updated' end as notification_type, a.recipient_id, to_char(a.history_date, 'yyyy-mm-dd') as notification_creation_date, to_char(b.history_date, 'yyyy-mm-dd') as event_creation_date, to_char(b.history_date,'hh12:mm AM') AS event_creation_time, a.subject, a.body from public.whispershistory_notification a left join public.whispershistory_event b on a.event_id = b.id where b.history_type = '+' and a.history_type = '+' and a.recipient_id = 6 and a.subject like '%All Events%') z where notification_type <> 'update: event updated' group by event_id, event_creation_date, event_creation_time, subject order by event_id desc;)

JChipault commented 2 years ago

I do believe that my preferences have been set to "new" and "updated" for "All Events" throughout that time.

aaronstephenson commented 2 years ago

I suspect however, the time interpretation is still the cause of the issue as it looks like late event creations are not producing update notifications for newly created events

This is indeed the case @nbaertlein . My assumption here was wrong. However, the fix I made here will correct for this problem (which continues to crop up as seen in Jenny's latest issue), I just need a greenlight to promote it.

JChipault commented 2 years ago

There was some reason why Neil thought that #534 was not related to this ticket, but I can't recall why. I'm fine with these things being pushed to production, but maybe touch base with @nbaertlein at your meeting tomorrow to make sure I'm not missing something?

JChipault commented 2 years ago

Also, I have a new potential hiccup related to notifications this morning..... seems unrelated but I'll throw it here for now... yesterday (Tues) I made event comments on 4 events. For the event that NWHC owns, I got a "Your Organization Events" notification with the text of the comment (as expected): image

However, I did NOT get "All Events" notifications for the event comment that I made on the other three events (listed in image above), which are events owned by a different org but I was still expecting notifications since I have privileges that allow me to see those event comments.

nbaertlein commented 2 years ago

Hey @aaronstephenson, yes. let's proceed with the promotion of your fix.
I was mistakenly thinking that changes prior to 9pm were not affected by this as it was > 6hrs from the 3am run, but I see now it needs to be 6 prior to midnight (thus the day before). Thanks!

aaronstephenson commented 2 years ago

Thanks Neil, in retrospect I can see what I wrote last month was a bit ambiguous about the time.

Jenny, I'll take a look at your latest find right now.

aaronstephenson commented 2 years ago

I fixed the latest problem you reported @JChipault. The notifications permissions validations for comments and contacts were just checking for event owner/org/collab (here and here); it turns out I hadn't included checks for admin and superadmin roles there. Fixed in 93b7a2e7 and will be included in next release.

aaronstephenson commented 2 years ago

Fix deployed in test and production environments as v2.1.11 0e85d7f