-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Projectors refactor #1624
Projectors refactor #1624
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CDimonaco I still see some Repo.queries
in the after_updates
that shouldn't be there:
- Cluster projection,
ClusterDeregistered
event. This is the unique real change i ask you XD - Host projection,
HostAddedToCluster
, you could add thereturning: true
value to theinsert
, so you get the data from the database, and you don't need to query in theafter_update
. Anywayinsert
firstquery
later is accepted as a good solution as well.
Ref
@@ -63,8 +63,10 @@ defmodule Trento.ClusterProjector do | |||
deregistered_at: deregistered_at | |||
}, | |||
fn multi -> | |||
cluster = Repo.get!(ClusterReadModel, cluster_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as a comment, in other project
functions, we have this pipelined:
changeset =
ClusterReadModel
|> Repo.get!(cluster_id)
|> ClusterReadModel.changeset(%{
deregistered_at: deregistered_at
})
Just in case if you prefer it 😝
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You triggered my ocd 👁️
Everything refactored to this style
@CDimonaco Besides the review, can you make me a small favor? XD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
Everything done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 🥇
Description
This pr refactor the usage of projectors, using the changesets with the current entity as the base entity for the changes.
Removes the queries from
after_update
callbacks, only the necessary association preloads for broadcasts are present.How was this tested?
Automated tests