Proposed solution

rename conversations/conversation-part to conversations/conversation-part-comment

All related files

app/components/conversations/conversation-part.js
app/templates/components/conversations/conversation-part.hbs
tests/pages/components/conversations/conversation-part.js
tests/integration/components/conversations/conversation-part-test.js

All references to those files

• page object import in -test.js
• renderPage in tests
• conversation-thread.hbs

add a conversations/conversation-part-closed.hbs

The component needs to inject the currentUser service, to compare the part.author.id with currentUser.user.id and decide if it should render You closed this at... or username closed this at....

add tests for this new component

• Renders "You closed this at..." if current user equals part author
• Renders "username closed this at..." if current user is different from part author

add a conversations/conversation-part-reopened.hbs

The component needs to inject the currentUser service, to compare the part.author.id with currentUser.user.id and decide if it should render You reopened this at... or username reopened this at....

add tests for this new component

• Renders "You reopened this at..." if current user equals part author
• Renders "username reopened this at..." if current user is different from part author

modify conversation thread

• We assume the API will return only those conversation parts we are allowed to see.
• We modify components/conversations/conversation-thread.hbs so that, for each conversation part, it renders a different component
  • if part_type is comment, render the usual conversations/conversation-part.hbs. Might want to rename it to conversation-part-comment
  • if part_type is closed, render conversation-part-closed
  • if part_type is reopened, render conversation-part-reopened, etc.

add tests to support this change

• renders proper component type for comment part type
• renders proper component type for closed part type
• renders proper component type for reopened part type

Add client-filtering of parts based on authorizaton

Wrap non-comment parts in the conversation thread under a {{#if canAdminister project}} helper, to hide non-comment parts from users who aren't admins on project.

Add integration tests to support this change

• Render a thread with one of each supported part type, register a fake project ability with canAdminister: true
• Render a thread with one of each supported part type ,register a fake project ability with canAdminister: false, see if just the comment part is rendered

See tests/integration/components/task-assignment-test.js on how to register a fake ability

from code-corps-ember.

As I said in chat, I like the idea of specifying thread type. It could even be used for other potential features.

There is an argument to be made, however, that if we're rendering a conversation thread of type "project" and another one of type "user", why not just call them project-conversation-thread and user-conversation-thread respectively and be done with it?

No {{#if}} switching within the template, no component logic to test. We just test that the project conv page renders a proper thread of x parts and that the user conv page randers a user thread of x parts.

If you think there's no risk of the user seeing a part that's not intended for them, then fine.

In my view, If a user is able to, in any way, ever, receive data from the API which they should not be able to see, then our API is unsafe and we need to deal with it immediately.

The ember app rendering or not rendering that data, while adding a minor failsafe, is in no way a fix.

So what I'm saying is, if there is a risk, that means we have an API problem.

from code-corps-ember.

On the topic of authorization filtering on the client

If it were a simple, quick check, I'd be fine with this.

However, we are scoping our API. The API should not return information the client should not see, no exceptions.

Checking for the ability from the client means, (remember, we are in /conversations/:id, not /:org/:project/conversations/:id

• we fetch the conversation.message asynchronously
• we fetch the message.project asynchronously
• we fetch each project-user asynchronously
• we check if currentUser.user is within the project's users, with a role of admin or higher

That's a lot of requests before the UI can be rendered safely, for something our API should be doing and we're just double-checking, just in case.

If we did some sideloading here, I'd be fine.
If our abilities were some sort of hash we get from the API in a single request, it would be great.
As is, it's a lot.

For a moment, I thought there may be value in this check allowing the admin to see all parts regardless of which route they are on, but really, that should work with just scoping.

It's possible I'm missing some key aspect here, would not be the first time 😬 , but outside of that, I don't think the benefits outweigh the cost.

from code-corps-ember.

You will also need to modify the conversation-part model to add a part_type attr and an associated test.

from code-corps-ember.

This goes in conversation part and would if else on the part_type.A cleaner solution would be to have the thread decide which component to render based on part type, then conversation-part becomes conversation-part-comment, Then, at least, you get just one authorization check, in the thread. not one for each part

from code-corps-ember.

If you think there's no risk of the user seeing a part that's not intended for them, then fine.

The other approach that I could suggest is that we have the conversation threads have type switching on them. So a conversation thread that we render in the project/conversations would show all conversation parts, whereas a thread we render in the /conversations would show only comment parts.

This avoids the issue of the request and still scopes the parts, plus allows us to be more explicit in the client.

from code-corps-ember.

I think I actually prefer my last comment now.

from code-corps-ember.

My last comment would involve taking the conversation-thread and type switching in the each based on the context. We'd need to define an attribute on the conversation-thread that identifies it as being for a project or a user, and use that to determine which conversation-part components we show.

from code-corps-ember.

I agree with this.

• conversations/project/conversation-thread
• conversations/user/conversation-thread

We could maybe refactor some things like:

• conversations/conversation-list-item-with-user => conversations/project/conversation-list-item
• conversations/conversation-list-item-with-project => conversations/user/conversation-list-item
• conversations/project-details => conversations/user/details-pane
• conversations/user-details => conversations/project/details-pane
• conversations/new-conversation-modal =>
  • conversations/project/composer-modal
  • conversations/user/composer-modal

What do you think of these proposed changes? That would also make things like:

• app/styles/components/conversations/shared/conversation-list-item.scss

...seem even clearer, since each conversation-list-item obviously shares styles.

from code-corps-ember.