Add messages

[joshsmith]

What's in this PR?

• Message
  • model
  • policy
  • controller
  • view

References

Progress on: #1234

[begedin]

If I'm understanding this correctly, this has no recipient yet? That goes into Conversation? The author is the user starting the message, the project is the project the user is a member/admin of.

If this is correct, or if it not correct and I'm misunderstanding it completely, I'm thinking it should be part of the
moduledoc either way, since it's not completely intuitive (albeit for good reasons).

[joshsmith]

The author is not necessarily a member of the project. The message could be to the project without the author being a member.

[begedin]

I would just call this chain require_subject_if_admin. Makes it clearer and foregoes the need for a comment

[joshsmith]

I didn't want to tie it to the admin specifically in the method signature, but probably fine for now to do that.

[begedin]

a guard clause would work better here

def create?(%User{id: id}, %{"initiated_by" => "user", "author_id" => author_id}) 
  when id == author_id, do: true

[joshsmith]

Only because the fallback is false. I'll make the change, but I'm not sure if I understand the argument.

[begedin]

Using a guard may be faster (not sure if it actually is here) and, additionally, stresses that there are no side effects to the check since only kernel functions can be used in guards.

It seems arbitrary, and probably doesn't affect much, if anything here, but generally it's preferred to avoid entering the function at all if it's easy to do, which it is here.

[begedin]

@joshsmith I've moved the list subclause logic into a separate Query module and made it so multiple filters can be applied at once. I think we should do this with most other endpoints, and here's my reasoning.

• Any client hitting the API, including our own will probably expect to have multiple filters being applied
• For example, if we do GET /messages?project_id=1&author_id=2, we expect messages from project 1, by author 2, not just all messages from project 1, ignore the author
• If a client request an array of ids, the client should worry about not additionally filtering by author or project. If they do specify, we should not assume they made a mistake and just give them those ids, ignoring the rest of the query, because we then effectively disable the ability to do that "mistake" intentionally.

[begedin]

@joshsmith I couldn't find an authorization approach for index that wouldn't completely kill our performance, so instead, I pushed up a commit which offers an alternative - scope

With this, our index route would not return a 403 and instead just return the data the user is authorized to view. That would mean anything ranging from no data, to some data, to all the data in the case of admin users.

The idea is that we apply the scope first, then apply the filters, although technically, any order would work.

Looking at authorization libraries, scopes are a common solution to the problem of index authorization.

For endpoints where we discriminate purely on user type (admin or not admin) returning a 403 would still be an option, but I really don't think returning a 403 when only some of the results are inaccessible is intuitive, nor is it manageable performance-wise.

However, if you disagree, I tried to write this without affecting the rest of the PR, so it can be reversible.

Even pundit, probably one of the better developed authorization libraries out there seems to encourage scopes for index actions. Yes, that's Ruby, not Elixir, but considering how Ecto fetches data (we build the query first, then explicitly execute it using Repo.all), it seems even more suited for this approach.

[begedin]

To make my previous commit stick, what we need to do is

• remove index? clauses from Policy.Message
• remove index? clause for Message from Policy
• remove index? tests
• consider creating an issue to switch to a scope approach on the GithubEvent index endpoint to, albeit that one is clearer on who gets to access it, regardless of data, so it may make sense to keep it around

[begedin]

While working on this, I found a bug, or at the very least, an inconsistency we need to deal with. I created an issue for it in #1282

[begedin]

@joshsmith You're the author, so I can't request your review, but this checks all the boxes in the PR description, so it should be good to review/merge

[begedin]

@joshsmith Due to findings from other issues I've created, it may be that

• we don't really need an index endpoint for messages, other than maybe for coalesced id requests.
• we do need a show endpoint
• we do need a create endpoint
• the create endpoint needs to create a conversation record alongside the message record

That means good chunks of this PR will get removed. However, I would still like to get it merged, since it's possible to reappropriate most of the code for reuse in issues linked above - #1286, #1287, #1288, #1289, #1290, #1291, #1292

I've created #1294 and #1293 to deal with remaining problems once this is merged

[snewcomer]

@joshsmith @begedin This looks great! So a conversation has many messages? Is the reason to move the index route to a Conversation due to the desire to create threads? If so, what role does Messages play in the UI? Trying to draw out a map of Conversations and Messages and how it is painted in the UI.

[begedin]

@snewcomer Actually, it's the other way around.

A project admin could send out a message to multiple users. That would create a single message, belonging to the project and authored by the admin, as well as multiple conversations - one for each user.

Once replies to a conversation start going in, conversation parts get created.

That means you can run separate conversations with different users from one starting message.

On the other side, a user could also message a project. This would create a Message authored by that user, belonging to that project, as well as a single Conversation belonging to that user and that message.

Once replies start going in, it works pretty much the same as above.

As I said in my previous comment, this PR is sort of wrong, and needs to be corrected post merge, but the work done here is useful for solving other problems, so we want to get it in.

[joshsmith]

I think we do need an index for messages because we'd still want to show which manual messages were created by a project in some other yet-to-be-determined UI.

[begedin]

@joshsmith As I said, we might need it anyway, which is one of several reasons why I'd prefer to merge this as is, barring any serious oversights, and move on from there.

[joshsmith]

I can't review myself but this is good to merge.