Email event cluster's subscribed users when an event is created #3362
Conversation
There was a problem hiding this comment.
I still worry about misuse of this, we have to be pretty careful. But we do need to get somethign implemented. One option would be to only send out notifications if the organizer is a community builder, that way there is some vetting.
Another thing: are we sure we want to send out the notification when the event is created, or should there just be a button on the event saying "send email" when the organizer has finished setting it up and is ready for the info to go out?
|
@schradert could you rebase this onto |
schradert
force-pushed
the
backend/feature/event-email
branch
from
2bb0e0e
to
b84b9a7
Compare
Just rebased |
I like this idea, but it requires frontend work. Should we wait until that is implemented on the frontend? Will the frontend repo be merged into this one so that we can have the PR address this if we prefer that approach with no intermediate solution? |
Okay I can work on this right now. I was told that there are many events not made by community builders. Is the idea we want to encourage them to become community builders in order to get this kind of event exposure? |
There was a problem hiding this comment.
This is looking pretty good. A few things:
-
There really needs to be some way to unsubscribe from these; I suggest you make a simple boolean in the user model. I can probably try hacking this together.
-
I had a look at the DB, here are the biggest communities:
cluster_id | name | count
------------+---------------------------------+-------
2 | Global Community | 29076
11 | Europe, Africa, and Middle East | 18065
5 | Americas | 8419
43 | United States of America | 5873
8 | Asia-Pacific | 2906
126 | Germany | 2422
46 | Turkey | 2100
570 | United Kingdom | 1673
497 | France | 1400
374 | Italy | 1327
95 | Canada | 1225
49 | Istanbul | 1003
397 | Spain | 996
1019 | Poland | 887
734 | Netherlands | 797
55 | Russia | 765
14 | Australia | 670
17 | New York City area | 557
153 | Berlin | 548
180 | Brasil | 526
888 | Belgium | 503
491 | Portugal | 465
500 | Paris | 461
189 | Sweden | 397
I would say the best thing would be to send notifications only for events in leaf clusters; but the 1k user limit is OK too if it's too much effort.
- The email needs to be fleshed out and polished, and we need to double check users can't inject random links into it, etc!
|
Let me know by tagging here when you're ready for another review! |
…lready taken by the ControlCe service which autorestarts on this port
…ne handler. The handler includes some validation to ensure users are not spammed with event emails. For example, if the event creator has not completed their profile, we abort the event creation. It's probably a very small edge case that an email will be sent out for a created event. I also included a filter for the largest nodes, global and regional. I then grab all the users that are subscribed to a cluster and send an email to all of them for the event. If there are too many users, I limit the users emailed to those in a 20km radius of the event occurrence.
…t is called when enqueuing the background job\nI make sure to pass in the user and event information to the email
… right number of times
The logic around max_users and max_radius already handles this situation. We just keep the one for Global right now because that's what all virtual events are assigned to.
this is more expected because it matches with the container port. i originally did this because AirPlay runs on 5000 and i thought it made sense to make the code work more generally for people since many have macOS with this same issue, but it's probably better to not enforce local differences like this on the codebase, so i just turn AirPlay off.
this allows accessing them straight from the event object to avoid repetitive join logic in various places
it could be interesting to replace the timezone of the event time (defaults currently as UTC) to reflect the user's timezone, but this became complicated...
Now the logic for selecting subscribers to send emails is the following: 1. If the creator doesn't have a completed profile, abort 2. Go through all clusters associated with the event and only exclude Global Community 3. If a cluster is a leaf node or the creator is an admin, always consider all cluster subscribers 4. Otherwise, filter out people not in a 20km radius of the event (there is always a default first occurrence for an event, so we just choose that as the location)
schradert
force-pushed
the
backend/feature/event-email
branch
from
8739dd3
to
0e57859
Compare
schradert
changed the title
this is already defined in the clusters model with a backref. the redundant definition was creating an issue with starting up the database because it couldn't create the same relation twice
this follows the instructions at docs/database.md
|
@aapeliv I followed the instructions at |
|
@aapeliv also I'm thinking we probably want the unsubscribe feature to be more granular. I imagine people want to unsubscribe for a more specific reason than wanting to never receive an email for a new event. My thought is it could be just for the event in question. Maybe in the future it could have a cluster-level control too. In this case though, my idea would be to also add this boolean flag you introduced to the |
It seems it picked up some postgis system tables. It has some functionality built in for US geo things, called "tiger" and another one, I think. Yeah just try deleting all but the stuff you added. There should be a test to pick up differences too. |
| select(User) | ||
| .join(ClusterSubscription, ClusterSubscription.user_id == User.id) | ||
| .where(ClusterSubscription.cluster_id == cluster.id) | ||
| .where(func.ST_Contains(func.ST_Buffer(event.occurrences[0].geom, max_radius), User.geom)) |
There was a problem hiding this comment.
I think func.ST_DWithin(User.geom, event.occurrences[0].geom, max_radius / 111111) is preferred, according to the web. These geography fields use WGS84 coordinates, so distances are measured in degrees; a degree is ~111km at the equator.
| A new event was created in your area! | ||
|
|
||
| {% set clusters = event.clusters %} | ||
| {% set is_plural = event.clusters | length > 1 %} |
|
@schradert See if you can fix the tests, if not let me know and I can have a look |
Builds on some work from #3362 by @schradert
Builds on some work from #3362 by @schradert
Builds on some work from #3362 by @schradert
Builds on some work from #3362 by @schradert
Builds on some work from #3362 by @schradert
Builds on some work from #3362 by @schradert
Builds on some work from #3362 by @schradert
Builds on some work from #3362 by @schradert
Builds on some work from #3362 by @schradert
Builds on some work from #3362 by @schradert
|
@aapeliv should we close this now that you've reworked in elsewhere? |
Yes, let's do that! |
Here is an incomplete attempt at sending emails as a background job when an event is created. An email would be sent to any user that is subscribed to the cluster the event was created in. I include some filters to prevent spamming, such as excluding events in the four largest clusters, only allowing events created by users with completed profiles to send emails, and excluding subscribers too far away when the number notified would exceed the maximum.
Currently, the background job test is failing when during the first event/cluster query in the job handler can't find a matching event. Even without the association to a cluster in the query, no event matches the payload ID. Could use some help here @ItsiW @aapeliv, I could make a codepairing work too for running through it and debugging. I suspect I'm just calling the event testing functions like
create_eventandcreate_communitywrong, or in the wrong context. Similar tests are passing. Perhaps I should rewrite the test from basics if these aren't adaptable.Backend checklist
autoflake -r -i --remove-all-unused-imports src && isort . && black .inapp/backenddevelopif necessary for linear migration history