Update conversation route #1309
Conversation
| # conversation | ||
| # |> where(user_id: ^id) | ||
| # |> or_where([c], c.message_id in ^scoped_message_ids) | ||
| true |
There was a problem hiding this comment.
@joshsmith what are the rules here when updating a conversations?
There was a problem hiding this comment.
Good question. I think it should be updated really only by someone who's an admin or the message's project is administered by a user.
The read_at in a conversation is going to be set by an update to the conversation_part, so it won't happen explicitly. The only explicit update to a conversation should be the status, which should be admin-only.
There was a problem hiding this comment.
lmk what you think! updated everything.
joshsmith
force-pushed
the
update-conversation
branch
from
7b143ca to
198e3ed
Compare
|
Actually not sure why my commits were changed but tons of merge conflicts |
snewcomer
force-pushed
the
update-conversation
branch
from
198e3ed to
146da87
Compare
|
@snewcomer I had rebased for you. Sorry about that. Was trying to be helpful but was not. |
|
@joshsmith ah no worries! I thought I did something. Does this need tracking? Not totally sure what the strategy as it relates to tracking.... |
|
We can circle back to figure out the tracking/analytics side of this later. I haven't really formed a coherent strategy around it yet for this feature set. |
joshsmith
left a comment
joshsmith
left a comment
There was a problem hiding this comment.
I think I was unclear about the policy earlier. My more detailed notes here should help.
| with %Conversation{} = conversation <- Messages.get_conversation(id) |> preload(), | ||
| %User{} = current_user <- conn |> CodeCorps.Guardian.Plug.current_resource, | ||
| {:ok, :authorized} <- current_user |> Policy.authorize(:update, conversation), | ||
| {:ok, %Conversation{} = updated_conversation} <- conversation |> Conversation.update_changeset(params) |> Repo.update |
There was a problem hiding this comment.
We should make use of the context here, so instead use Messages.update_conversation(conversation, params).
| def show?(_, _), do: false | ||
|
|
||
| def update?(%User{admin: true}, _conversation), do: true | ||
| def update?(_, _), do: false |
There was a problem hiding this comment.
This also needs the following clause:
def update(%User{} = user, %Conversation{} = conversation) do
conversation |> get_message() |> get_project() |> administered_by?(user)
end| conversation = insert(:conversation, message: message, user: user) | ||
|
|
||
| refute update?(user, conversation) | ||
| end |
There was a problem hiding this comment.
This will need tests added like in the show? tests above once the policy is updated per the above comment.
| @tag authenticated: :admin | ||
| test "updates and renders chosen resource when data is valid", %{conn: conn, current_user: current_user} do | ||
| message = insert(:message) | ||
| conversation = insert(:conversation, user: current_user, message: message) |
There was a problem hiding this comment.
This test will need modified using logic similar to the index's conversation_on_user_administered_project when the policy is changed, rather than relying on admin here.
|
@joshsmith fixed! Thanks for reviewing it all! |
|
|
||
| def update_conversation(conversation, params) do | ||
| Conversation.update_changeset(conversation, params) |> Repo.update | ||
| end |
There was a problem hiding this comment.
Style thing but I would consider leading with the pipe, so conversation |> Conversation.update_changeset...
| message_on_user_administered_project = insert(:message, project: project) | ||
| conversation_on_user_administered_project = | ||
| insert(:conversation, message: message_on_user_administered_project) | ||
| assert conn |> request_update(conversation_on_user_administered_project, %{status: "wat"}) |> json_response(422) |
There was a problem hiding this comment.
It looks like you're doing a 422 assert in here. This line probably needs removed.
snewcomer
force-pushed
the
update-conversation
branch
from
e1fea5b to
829c4a7
Compare
snewcomer
force-pushed
the
update-conversation
branch
from
829c4a7 to
c1c15fa
Compare
|
🙌 |
Fixes #1308