Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add counter cache #1343

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

add counter cache #1343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dbc18a1
add counter cache
snewcomer Dec 26, 2017
ee4524b
wip for callback to update counter
snewcomer Dec 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion lib/code_corps/messages/conversations.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ defmodule CodeCorps.Messages.Conversations do
specific user belonging to a `CodeCorps.Message`.
"""

import Ecto.Query
alias Ecto.Changeset

alias CodeCorps.{Conversation}
alias CodeCorps.{Conversation, Project}

@doc false
@spec create_changeset(Conversation.t, map) :: Ecto.Changeset.t
Expand All @@ -15,6 +16,15 @@ defmodule CodeCorps.Messages.Conversations do
|> Changeset.cast(attrs, [:user_id])
|> Changeset.validate_required([:user_id])
|> Changeset.assoc_constraint(:user)
|> Changeset.prepare_changes(fn prepared_changeset ->
repo = prepared_changeset.repo
project_id = prepared_changeset.project_id

from(p in Project, where: p.id == ^project_id)
|> repo.update_all(inc: [open_conversations_count: 1])

prepared_changeset
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is a db trigger. @begedin what do you think about this sol'n as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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_assoc into 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_changes for 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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the conversation part is created, then would the conversation itself not have a body anymore (the part would have that content?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

end)
end

@doc false
Expand Down
2 changes: 2 additions & 0 deletions lib/code_corps/model/project.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you reorder these attributes by alpha in here?


belongs_to :organization, CodeCorps.Organization

Expand Down
10 changes: 10 additions & 0 deletions priv/repo/migrations/20171226203914_add_counter_cache_to_project.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
defmodule CodeCorps.Repo.Migrations.AddCounterCacheToProject do
use Ecto.Migration

def change do
alter table(:projects) do
add :open_conversations_count, :integer, null: false, default: 0
add :closed_conversations_count, :integer, null: false, default: 0
end
end
end
8 changes: 5 additions & 3 deletions priv/repo/structure.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
-- PostgreSQL database dump
--

-- 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would recommend upping your pg version when you have a chance.


SET statement_timeout = 0;
SET lock_timeout = 0;
Expand Down Expand Up @@ -884,6 +884,8 @@ CREATE TABLE projects (
website character varying(255),
should_link_externally boolean DEFAULT false,
approval_requested boolean DEFAULT false,
open_conversations_count integer,
closed_conversations_count integer,
CONSTRAINT set_long_description_markdown_if_approved CHECK (((long_description_markdown IS NOT NULL) OR (approved = false)))
);

Expand Down Expand Up @@ -4159,5 +4161,5 @@ ALTER TABLE ONLY users
-- PostgreSQL database dump complete
--

INSERT INTO "schema_migrations" (version) VALUES (20160723215749), (20160804000000), (20160804001111), (20160805132301), (20160805203929), (20160808143454), (20160809214736), (20160810124357), (20160815125009), (20160815143002), (20160816020347), (20160816034021), (20160817220118), (20160818000944), (20160818132546), (20160820113856), (20160820164905), (20160822002438), (20160822004056), (20160822011624), (20160822020401), (20160822044612), (20160830081224), (20160830224802), (20160911233738), (20160912002705), (20160912145957), (20160918003206), (20160928232404), (20161003185918), (20161019090945), (20161019110737), (20161020144622), (20161021131026), (20161031001615), (20161121005339), (20161121014050), (20161121043941), (20161121045709), (20161122015942), (20161123081114), (20161123150943), (20161124085742), (20161125200620), (20161126045705), (20161127054559), (20161205024856), (20161207112519), (20161209192504), (20161212005641), (20161214005935), (20161215052051), (20161216051447), (20161218005913), (20161219160401), (20161219163909), (20161220141753), (20161221085759), (20161226213600), (20161231063614), (20170102130055), (20170102181053), (20170104113708), (20170104212623), (20170104235423), (20170106013143), (20170115035159), (20170115230549), (20170121014100), (20170131234029), (20170201014901), (20170201025454), (20170201035458), (20170201183258), (20170220032224), (20170224233516), (20170226050552), (20170228085250), (20170308214128), (20170308220713), (20170308222552), (20170313130611), (20170318032449), (20170318082740), (20170324194827), (20170424215355), (20170501225441), (20170505224222), (20170526095401), (20170602000208), (20170622205732), (20170626231059), (20170628092119), (20170628213609), (20170629183404), (20170630140136), (20170706132431), (20170707213648), (20170711122252), (20170717092127), (20170725060612), (20170727052644), (20170731130121), (20170814131722), (20170913114958), (20170921014405), (20170925214512), (20170925230419), (20170926134646), (20170927100300), (20170928234412), (20171003134956), (20171003225853), (20171006063358), (20171006161407), (20171012215106), (20171012221231), (20171016125229), (20171016125516), (20171016223356), (20171016235656), (20171017235433), (20171019191035), (20171025184225), (20171026010933), (20171027061833), (20171028011642), (20171028173508), (20171030182857), (20171031232023), (20171031234356), (20171101023309), (20171104013543), (20171106045740), (20171106050209), (20171106103153), (20171106200036), (20171109231538), (20171110001134), (20171114010851), (20171114033357), (20171114225214), (20171114225713), (20171114232534), (20171115201624), (20171115225358), (20171119004204), (20171121075226), (20171121144138), (20171123065902), (20171127215847), (20171201073818), (20171205161052), (20171213062707), (20171220154922);
INSERT INTO "schema_migrations" (version) VALUES (20160723215749), (20160804000000), (20160804001111), (20160805132301), (20160805203929), (20160808143454), (20160809214736), (20160810124357), (20160815125009), (20160815143002), (20160816020347), (20160816034021), (20160817220118), (20160818000944), (20160818132546), (20160820113856), (20160820164905), (20160822002438), (20160822004056), (20160822011624), (20160822020401), (20160822044612), (20160830081224), (20160830224802), (20160911233738), (20160912002705), (20160912145957), (20160918003206), (20160928232404), (20161003185918), (20161019090945), (20161019110737), (20161020144622), (20161021131026), (20161031001615), (20161121005339), (20161121014050), (20161121043941), (20161121045709), (20161122015942), (20161123081114), (20161123150943), (20161124085742), (20161125200620), (20161126045705), (20161127054559), (20161205024856), (20161207112519), (20161209192504), (20161212005641), (20161214005935), (20161215052051), (20161216051447), (20161218005913), (20161219160401), (20161219163909), (20161220141753), (20161221085759), (20161226213600), (20161231063614), (20170102130055), (20170102181053), (20170104113708), (20170104212623), (20170104235423), (20170106013143), (20170115035159), (20170115230549), (20170121014100), (20170131234029), (20170201014901), (20170201025454), (20170201035458), (20170201183258), (20170220032224), (20170224233516), (20170226050552), (20170228085250), (20170308214128), (20170308220713), (20170308222552), (20170313130611), (20170318032449), (20170318082740), (20170324194827), (20170424215355), (20170501225441), (20170505224222), (20170526095401), (20170602000208), (20170622205732), (20170626231059), (20170628092119), (20170628213609), (20170629183404), (20170630140136), (20170706132431), (20170707213648), (20170711122252), (20170717092127), (20170725060612), (20170727052644), (20170731130121), (20170814131722), (20170913114958), (20170921014405), (20170925214512), (20170925230419), (20170926134646), (20170927100300), (20170928234412), (20171003134956), (20171003225853), (20171006063358), (20171006161407), (20171012215106), (20171012221231), (20171016125229), (20171016125516), (20171016223356), (20171016235656), (20171017235433), (20171019191035), (20171025184225), (20171026010933), (20171027061833), (20171028011642), (20171028173508), (20171030182857), (20171031232023), (20171031234356), (20171101023309), (20171104013543), (20171106045740), (20171106050209), (20171106103153), (20171106200036), (20171109231538), (20171110001134), (20171114010851), (20171114033357), (20171114225214), (20171114225713), (20171114232534), (20171115201624), (20171115225358), (20171119004204), (20171121075226), (20171121144138), (20171123065902), (20171127215847), (20171201073818), (20171205161052), (20171213062707), (20171220154922), (20171226203914);