Skip to content

Conversation

lastcanal
Copy link
Contributor

@lastcanal lastcanal commented Jan 13, 2025

This feature adds activity notifications and optional change tracking to the app. It works by creating a 'abstract' activities table for each table we want activities to be associated with, as described here. I have added an activity table for almost every table in the database, although some some might no make sense and can be removed later.

The main entry point for adding activities is through Algora.Repo.insert/update/delete_with_activity. It creates an Ecto.Multi transaction for inserting associated activities and any Oban jobs. A more ergonomic option not requiring a Multi transaction is Activity.put_activity/3, but that requires adding a new on_remove: :ignore option to Ecto, otherwise we will have to load all associated activities for the target object prior to updating. I have removed this fork of Ecto for now and we are still using the default on_remove: :raise, as I am not not sure if they will adopt :ignore.

Querying this setup is accomplished by "unioning" all the tables that you want activities from, instead of filtering out the uninteresting assoc_type as you would with one giant 'activities' table. It also makes it possible to "union" across tables using associations, for example, getting all contract activities for a user. This even works with nested associations, for example getting all contract activities for a user, including associated transaction activities. Querying for all activities of one type is very simple.

My plan is to have all a user's 'activities' displayed in a 'feed' somewhere, and also emailed to them if they are not online, depending on the activity.

See this graph for a basic visual description:

graph
     TargetChangeset --> |Multi.new| InsertWithActivity
    InsertWithActivity --> InsertTarget
    InsertActivity --> InsertNotifierJob
    InsertWithActivity --> |with target changes| InsertActivity
    InsertTarget --> |with target id| InsertActivity
    InsertNotifierJob --> | Multi.transaction| NotifierJob
    NotifierJob --> |online| LiveUpdate
    NotifierJob --> |offline / forced| SendNotification
    SendNotification --> SendEmailJob
    SendNotification --> SendSNSJob

  
Loading

@lastcanal lastcanal force-pushed the activity-notifications branch from 0572de1 to 3295e78 Compare January 13, 2025 17:01
@lastcanal
Copy link
Contributor Author

I have also added ExCoveralls and started building out some coverage; I am hoping to have all the notifications covered as some of them are tough to exercise. I've copied some of the mocks into test/support/mock for use in other tests.

@lastcanal
Copy link
Contributor Author

Frustratingly, it seems as though it's impossible with this setup to load the "associated" object from the Activity without adding either a assoc_type field and suffering N+1, or having to use many_to_many relationships with every associated object in a join table. Ecto appears to be not as good at polymorphism as Ruby's ActiveRecord: https://elixirforum.com/t/multiple-polymorphic-associations-in-ecto/983/5

Would it be possible for you guys to give me a list of "Activities" you are interested in displaying to the user? I kept this implementation very broad because it would give me flexibility while figuring that out. Now that a join table with a foreign key per association is needed, it's probably best to figure out what we want ahead of time.

@zcesur you mentioned this was more granular than you would have done it; what level of granularity are you looking for? Also, it might be helpful to get me a list of all the emails that are sent from the old code base, along with their content.

FYI The reason Ecto doesn't do Rails style polymorphism is because it's unable to retain referential integrity on delete when using assoc_type and assoc_id on one big activities table. I'm going to send a PR to Ecto in the coming weeks, clarifying their documentation and providing the drawbacks to using their 'second approach' to polymorphism. It seems as though Ash is able to handle polymorphic unioning, perhaps the team at Ecto can help me figure out how to add something like has_one :assoc union: [table1, ...], but that will take some time that I don't think we have right now.

drop transaction activities and 'second' level activities associated
with a user
@zcesur
Copy link
Member

zcesur commented Jan 16, 2025

That's a bummer, this looked very promising. What kind of error/result is your union_all reducer returning? While it's all fresh in your mind, I suggest writing up a quick question about it on the Elixir Forum before ditching it - I see lots of active senior Elixir folks there including the core Ash devs

In any case, the many-to-many approach should be perfectly fine!

In terms of granularity we'd probably wanna track most changes on most schemas at some point to have a proper audit log, but for the time being let's focus on main business capabilities & events, starting with things like:

  • bounty.awarded
  • bounty.posted
  • bounty.repriced
  • claim.submitted
  • claim.approved
  • tip.awarded

Let's do emails only for {bounty,tip}.awarded and bounty.posted for now - the former sent immediately and the latter sent as a digest once every N days based on user preference. Basic copy without any fluff or fancy formatting, e.g.

Subject: 🎉 $75 bounty awarded by ZIO

Congratulations, you've been awarded a bounty by ZIO! [View balance]

© 2025 Algora, Public Benefit Corporation
You are subscribed to Bounty awarded emails. Manage your communication preferences here.

@lastcanal
Copy link
Contributor Author

All is not lost: I managed to get the associations loading using a virtual assoc_type field on Activity and a N+1 query. It's pretty un-optimized right now but can be made N+1 where N is the number of distinct 'activity tables' you need to access, which will only be 3 at most, at first. This think this might be possible to do in one query, but it would need to be done in Ecto as there is no way to configure a has_one association that is polymorphic.

Thanks for the extra info! I'm starting to get the hang of this app, should be easy to get those in there.

@lastcanal lastcanal changed the title [WIP] feat: activity notifications feat: activity notifications Jan 23, 2025
@zcesur
Copy link
Member

zcesur commented Jan 23, 2025

Amazing 🫡

@zcesur zcesur merged commit 9e6b3ac into main Jan 23, 2025
1 check passed
zcesur pushed a commit that referenced this pull request Apr 15, 2025
* activities (with has_many on_replace: :ignore)

* add more tables, test helpers

add accounts test

* add more user activity attributes

* add Repo.insert_with_activity/2

* add notifier

* rename

* Repo.update/delete_with_activity

* test register github user activities

* add coveralls

* add basic coverage for bounties, chat and reviews and coveralls.json

* remove redundant

* remove ecto fork

* Setup notifier and signal module

Send error signals when failing to link stripe accounts

* account coverage

* update coveralls

* rename

* fetch user activities from nested children

* add notifier job template

* satisfy the dialyzer

format

* filter out bounties without solver when onboarding dev, fix KeyError

* lower Drawer z-index; it was hiding flash messages

* load association for activities

drop transaction activities and 'second' level activities associated
with a user

* use Dataloader for loading activity associations

* add Mox to dev

* add create_session to Stripe SeedImpl

* format

* use map for activity table lookups

* add owned|created_tips to User

* add bounty and tip activities

* test

* add activities dropdown in main nav and on company admin page

* implement Algora.Analytics.get_company_analytics/0-2

* remove transaction activities

* zero not implemented demo data

* add activity redirect controller

* add User#received_tips

* cleanup

* typo

* load target association with Activities.get

* show activities in header

broadcast from Oban job

enqueue email deilvery

add mail templates

* fix merge

* Protocol.UndefinedError for claims when creating tips

* adapt bounty testing to claims

* add on true to inner_lateral_join

* add redirect url to email body

* remove vars from email subjects

* use schema.preload/1

* add claim_submitted and bounty_awarded activities

* add utility for making admins

* remove unused alias

* add mix algora.create_tip

* send email job

* rework activity views

* assert mail job enqueued

* add todos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants