add counter cache #1343
add counter cache #1343
Conversation
snewcomer
force-pushed
the
counter-cache
branch
from
ba5d6b3
to
964dfc2
Compare
snewcomer
force-pushed
the
counter-cache
branch
from
964dfc2
to
dbc18a1
Compare
| from(p in Project, where: p.id == ^project_id) | ||
| |> repo.update_all(inc: [open_conversations_count: 1]) | ||
|
|
||
| prepared_changeset |
There was a problem hiding this comment.
@joshsmith @begedin Right now the project_id is not an association on the Conversation and I'm not totally sure we need to. What if we passed the project_id from the Messages.create_changeset?
There was a problem hiding this comment.
Another option is a db trigger. @begedin what do you think about this sol'n as well?
There was a problem hiding this comment.
@joshsmith To elaborate
This is a changeset used by cast_assoc during the create changeset for a Message, to insert a conversation alongside it.
The way cast_assoc works is, we do not associate the conversation with the message, and really, by default, there is no information about the parent Message at this point. Since that's where the project information is to, we don't get that either.
The way I see it, we have several choices, but I'm really not sure what the correct one is.
- We could specify an anonymous wrapping function as the Conversation changeset, which would call this function, while also currying the message/project information as an additional argument
- It would work, but it would be ugly and it would not solve a similar issue we'd likely have in the update case
- We could transform the
cast_associnto a multi, where the third step of the multi would be a counter cache update- steps would be - insert message, insert conversation, update project counter cache
- we'd need another multi for the update case
- We could handle the counter cache update as part of the
prepare_changesfor the parent, message changeset- This would quite elegantly solve this case, but it would not solve the update case
- A db trigger - would be the first one we did, I think
- A project association on the conversation - makes the relationships more complex, but would work and kind of makes sense.
I'm really not sure which way to go here. @joshsmith What do you think?
There was a problem hiding this comment.
My feeling - after having spent some time looking at this myself earlier - is that we should be adding a project relationship into conversations. It would simplify a lot of what’s going on in the conversations UI on the frontend as well. What do you think?
There was a problem hiding this comment.
It would make sense, if we look at the whole thing as the Message being a sort of conversation template, and the Conversation being an instance of that, targeted at a specific user.
There was a problem hiding this comment.
If the conversation part is created, then would the conversation itself not have a body anymore (the part would have that content?)
There was a problem hiding this comment.
@joshsmith @begedin I like all the answers above.
The db trigger is quite simple and the most efficient. We can always get the scope of the project through the message; however, in this case, it seems we are simply limited by function logistics.
Adding the project to the conversation seems to make everything more complex and "bulky", but I might be missing something as to why it may be a good idea in the grander scheme of things.
There was a problem hiding this comment.
@begedin right now the Conversation does not have a body – the Message does. The Message would still have a body going forward, but it would be copied over to the ConversationPart when the Conversation is created. The subject probably does not need to be copied over since it's ephemeral, but it might be a good idea to do so simply because we may need to do some weird parsing on inbound email replies to a message and retaining the original subject could help with that.
There was a problem hiding this comment.
@snewcomer really the need for a Project relationship on Conversation stems from something probably not immediately clear in the current implementation. There will be two types of Message in the future:
- manual messages (implemented now)
- sent to users on a one-off basis
- 1:1 between a message and conversation
- message details cannot change once sent
- auto messages
- sent to users based on triggers
- 1:n between a message and many conversations
- message details can be refined, but should not change previous conversations
Given that, it makes sense to have a relationship between Project and Conversation, and to copy some data from Message to ConversationPart, even though it is more implementation than necessary for just manual messages.
There was a problem hiding this comment.
Ok. So going forward, would it be best if I left this PR just to add the counter cache fields (w/o the prepare_changes) and open another issue based off this discussion?
| @@ -31,6 +31,8 @@ defmodule CodeCorps.Project do | |||
| field :title, :string | |||
| field :total_monthly_donated, :integer, default: 0 | |||
| field :website, :string | |||
| field :open_conversations_count, :integer, default: 0 | |||
| field :closed_conversations_count, :integer, default: 0 | |||
There was a problem hiding this comment.
Can you reorder these attributes by alpha in here?
| -- Dumped from database version 10.1 | ||
| -- Dumped by pg_dump version 10.1 | ||
| -- Dumped from database version 9.6.6 | ||
| -- Dumped by pg_dump version 9.6.6 |
There was a problem hiding this comment.
Would recommend upping your pg version when you have a chance.
begedin
force-pushed
the
develop
branch
17 times, most recently
from
8ed3069
to
e075407
Compare
References
Fixes #1313