Skip to content

Conversation

@begedin
Copy link
Contributor

@begedin begedin commented Dec 13, 2017

What's in this PR?

This PR basess off of #1295 to facilitate review. Should be rebased to develop once #1295 is merged.

This PR adds a ConversationController with an :index endpoint as well as all associated infrastructure

  • Messages.list_conversations/2
  • ConversationView
  • Policy.Conversation.scope/2

Tests have also been added.

References

Fixes #1288

Progress on: https://github.com/code-corps/code-corps-api/milestone/22

@begedin begedin requested a review from joshsmith December 13, 2017 10:53
@begedin begedin changed the base branch from develop to 1286-conversation-models December 13, 2017 10:53
"""
@spec status_filter(Queryable.t, map) :: Queryable.t
def status_filter(queryable, %{"status" => "active"}) do
prefiltered_ids = queryable |> select([c], c.id) |> Repo.all
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to additional joins below, we cannot just pipe the existing query due to potentially applying the project filter above as well. The SQL query ecto builds get's all garbled up with multiple joins and is no longer applying conditions to correct schemas and fields. Unfortunately, this is the only way I could find to support flexibility of multiple queries.

|> join(:left, [c, _m], cp in ConversationPart, c.id == cp.conversation_id)
|> group_by([c, m, _cp], [c.id, m.initiated_by])
|> having([_c, m, _cp], m.initiated_by == "user")
|> or_having([c, m, cp], m.initiated_by == "admin" and count(cp.id) > 0)
Copy link
Contributor Author

@begedin begedin Dec 13, 2017

Choose a reason for hiding this comment

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

Adding a reply_count counter cache field to Conversation might speed things up a bit here and it would certainly replace the having/or_having with where/or_where, but I would do it as part of a separate issue, since updating it would involve ConversationPart changesets.

My vote is, separate issue for consideration, and mark it low priority for now. If we find we need it for performance reasons, we can do it then.

@joshsmith joshsmith force-pushed the 1286-conversation-models branch from 51b35b4 to 30c9b07 Compare December 13, 2017 19:17
@joshsmith joshsmith force-pushed the 1288-conversation-index branch from e5d4759 to 270f0e2 Compare December 13, 2017 19:18
@joshsmith joshsmith changed the base branch from 1286-conversation-models to develop December 13, 2017 19:34
@joshsmith joshsmith force-pushed the 1288-conversation-index branch 2 times, most recently from 3877f5b to e5d4759 Compare December 13, 2017 19:43
@joshsmith joshsmith changed the base branch from develop to 1286-conversation-models December 13, 2017 19:46
@joshsmith joshsmith changed the base branch from 1286-conversation-models to develop December 13, 2017 19:47
@begedin begedin force-pushed the 1288-conversation-index branch from e5d4759 to 2ca0b0a Compare December 14, 2017 06:18
- Policy.Conversation.scope/2 with tests
- Messages.list_conversations/2 with tests
- ConversationView and tests
- ConversationController :index endpoint and tests
@begedin begedin force-pushed the 1288-conversation-index branch from 2ca0b0a to 21293b0 Compare December 14, 2017 06:21
@begedin begedin changed the title 1288 conversation index Conversation index endpoint with supporting infrastructure Dec 14, 2017
@joshsmith joshsmith merged commit a494af2 into develop Dec 14, 2017
@joshsmith joshsmith deleted the 1288-conversation-index branch December 14, 2017 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants