Feat: email notifications aggregation

[chrismclarke]

PR Checklist

• - Commit messages are descriptive, it will be used in our Release Notes

PR Type

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Developer experience (improves developer workflows for contributing to the project)

Description

As a prerequisite for delivering email summaries of user notifications we need an aggregated list of users who have opted in to receive email notifications, and their current list of unread notifications. This PR does the following:

• Add an aggregation to create single list of all users with pending notifications for sending via email
• Add admin page to show list of pending emails (for use in debugging)

Not done (next steps)

The email list is not yet formatted as HTML nor are configured to actually send (just showing a list for testing purposes). So roughly next steps in the process would be

• Create frontend UI to allow users to opt in/out of email notifications
• Create formatted emails from the list of pending notification data (will need to create as raw HTML so can't use the rendered page display unfortunately - depending on email system integration this may include a list of email templates that can be edited by admins)
• Integrate email sender and attach to function that triggers daily/weekly to process list of pending emails

Testing Notes

Run platform with latest emulators backend (note, image recently updated so might take a couple minutes to pull and make ready)

yarn start:emulated:docker

Once the docker emulators are running you will want to sign in as the demo_admin user, and then trigger a notification, prompting the email summary list to populate for the first time.
The easiest way is to either vote useful or add a comment to any howto not owned by the demo_admin user (see video below)
username: demo_admin@example.com
password: demo_admin

A list of notifications marked for future send will be displayed in the admin panel. As there is currently no frontend UI for enabling notifications for a user, the demo_admin and demo_beta_tester users both have been preconfigured to receive weekly notifications

Git Issues

Closes #

Screenshots/Videos

Seeding initial list of user email notifications, watching list respond when notifications read

Note - by default both demo_admin and demo_beta_tester have one pending unread message, however the aggregation list will only populate after a new notification is triggered (hence why first empty, then has 3 items). They are also the only users that will appear in the list, because they have their email frequency hardcoded into the database (need frontend for other users)

How-to.webm

---

What happens next?

Thanks for the contribution! We try to make sure all PRs are reviewed ahead of a monthly dev call (first Monday of the month, open to all!).

If the PR is working as intended it'll be merged and included in the next platform release, if not changes will be requested and re-reviewed once updated.

If you need more immediate feedback you can try reaching out on Discord in the Community Platform development channel.

[cypress]

1 flaky tests on run #2907 ↗︎

0

52

0

0

1

Details:

Merge branch 'master' into feat/email-notifications-backend
Project: onearmy-community-platform	Commit: 846a8f99a1
Status: Passed	Duration: 02:59 💡
Started: Feb 13, 2023 4:25 PM	Ended: Feb 13, 2023 4:28 PM

  src/integration/common.spec.ts • 1 flaky test • ci-chrome

View Output Video

Test
[Common] > [User Menu] > [By Authenticated]		Screenshot

_{This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.}

[davehakkens]

Looks cool @chrismclarke !
But i've been trying a few times but I dont seem to manage to run this locally with the emulator..
latest message in terminal is
[platform] cross-env PORT=4000 yarn start:platform exited with code 129
I'll give a try with the preview link..

[chrismclarke]

Interesting, could you try run that one command as standalone and see if there's any more info output, i.e.

cross-env PORT=4000 yarn start:platform

I don't think the preview will do much unfortunately as that's not configured to work with the docker emulators (instead uses its firebase online). There's also a chance some of the related packages aren't quite updating correctly, one other thing you could try is to see whether it will actually build

yarn build

If the build succeeds then likely the full command should work afterwards. If not might have the same or different log messages

[davehakkens]

If I run cross-env PORT=4000 yarn start:platform i get zsh: command not found: cross-env
yarn-build seems to work though..

The project was built assuming it is hosted at /.
You can control this with the homepage field in your package.json.

The build folder is ready to be deployed.
You may serve it with a static server:

  yarn global add serve
  serve -s build

Find out more about deployment here:

  https://cra.link/deployment

If I run yarn start:emulated:docker after yarn-build i get this:

[chrismclarke]

sorry, should have made that first command

yarn cross-env PORT=4000 yarn start:platform

[chrismclarke]

oh, and actually just realised there are a few more lines above in the original error that would be useful to see, could you screenshot to include the output from the [emulators] part of the log as well?

With the way it needs to run 5 different commands, if any 1 fails it stops all the others. So quite possible that the final error message you shared is actually triggered by an earlier error

[davehakkens]

[chrismclarke]

ah, ok - looks like there's an issue with the docker image itself. Sorry about that, I'll take a quick look

[chrismclarke]

Docker image issue now resolved, was super simple, barely an inconvenience.

So hopefully testing will work for you this time :D

[davehakkens]

haha yes, works now! All how you described above :)
Cool to see those actions in the admin panel.

And these messages would dissapear when the email is send?

[chrismclarke]

And these messages would dissapear when the email is send?

That would be the plan. We could try wait for confirmation of delivery before removing, but could get a little complicated (probably easier to have some sort of retry system independent of the notification list, so that we don't check the list again when we retry but just try send the exact same message)

Otherwise it would be possible if we want to keep it as unread on the web page but not turn up in the email list anymore (there's two separate fields for read and notified), although I think currently the code assumes all notified messages to be read so might take some tweaks.

[davehakkens]

Just to double check if I get it correct. How it would work:

New Notification comes in

1. It's instantly visable on website
2. After a period (day/week) all notifications that are not cleared on website (clear notifications button) will be send as email summary.
3. When the summary mail is send we assume they see it and all those notifications are cleared?

Follow up question:
Are there atm (smart) actions for individual notifications?
For instance If I click a single notification, wheter its in mail or web. Is there something that marks that specific notification as unread or notified? Or is this only done in bluk with the "clear notifications" button?

Adrian talked about this topic, but not sure what he ended up making 😅

Happy both ways, just want to make sure I understand the flow

[chrismclarke]

Just to double check if I get it correct. How it would work:

New Notification comes in

1. It's instantly visable on website
2. After a period (day/week) all notifications that are not cleared on website (clear notifications button) will be send as email summary.
3. When the summary mail is send we assume they see it and all those notifications are cleared?

Definitely points 1 and 2 is what we've been working towards, I think point 3 is a question for how we want things to work. According to the google doc draft notifications are notified either in site or via email - I think currently whenever the user clicks the notifications icon that marks all as notified, and then on dismiss marks all as read. If we keep the proposed logic to mark all as notified when sending an email I think that should mean they stay in the list, but the icon will not be lit as there are no new notifications. This seems pretty sensible to me, although I would have to re-check whether that's how things are working currently or not.

Follow up question: Are there atm (smart) actions for individual notifications? For instance If I click a single notification, wheter its in mail or web. Is there something that marks that specific notification as unread or notified? Or is this only done in bluk with the "clear notifications" button?

Pretty sure its all or nothing, there's no methods in the code to mark a single notification as read/notified, just all in bulk

[davehakkens]

If we keep the proposed logic to mark all as notified when sending an email I think that should mean they stay in the list, but the icon will not be lit as there are no new notifications. This seems pretty sensible to me, although I would have to re-check whether that's how things are working currently or not.

I dont think notifications currently work like that.
The only way to have the icon not 'lit' is to 'clear all notifications'.
If there is a notification in the list, seen or unseen, the icon is lit.

Thats mainly because we currently dont use two states, only now we introduce notified + read.
Your proposed logic with that makes sense to me and would be more user friendly.
But not sure how much effort it would take to fully intergrate.

[chrismclarke]

@evakill would you be able to take a pass at reviewing this by any chance?

It updates backend code so will need to be run with the emulator setup. It also uses an intermediate aggregations system, which is a set of utilities to intercept firestore db changes and trigger functions only if specific fields have changed.

This is because firestore doesn't allow for fine-grained change detection and want to avoid having too many listeners for something like a user profile update.

Feel free to drop a comment in the code if you think anything could do with better commenting/clarity, or any other issues you come across.

[davehakkens]

Just checking how this one is going..
Waiting for review from @evakill?

[evakill]

Just checking how this one is going.. Waiting for review from @evakill?

ah yes - let me look at this tonight

[evakill]

should we be using the notification_settings.enabled field here instead/also to filter on type of notif?

[chrismclarke]

Thanks @evakill , I hadn't thought about how it relates to filtering the specific types of notifications users are subscribed to or not. I'm assuming it might be a future PR anyway that handles filtering, so for now I'd say probably better to keep this simple and let all notifications pass through, and then filter out at a later stage (likely before hitting the user profile notifications, so that they don't receive on web and before we reach this aggregation step)

[evakill]

one q but otherwise tested locally and is working well!

[davehakkens]

thanks @evakill!

[onearmy-bot]

🎉 This PR is included in version 1.37.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀