Conversations Feature #1597

begedin · 2017-12-11T13:25:56Z

The API changes required for this to work are in PR code-corps/code-corps-api#1301

Closes #1592
Closes #1593
Closes #1594
Closes #1596

begedin · 2017-12-15T13:01:52Z

I pushed a big commit, because I'm afraid there was a bit of a snowball effect of things that needed to work to make the PR self-contained. Will add comments to the code where applicable.

begedin · 2017-12-15T13:02:32Z

app/abilities/project.js

+   * Returns true if the user is the owner of the project.
+   * @type {Boolean}
+   */
+  canAdminister: or('{userIsAdmin,userIsOwner}'),


Until this PR, all of the bigger actions on a project were accessible by owners only. The only thing admins could do was approve pending members.

begedin · 2017-12-15T13:03:22Z

app/controllers/project/messages/message.js

+import { alias } from '@ember/object/computed';
+
+export default Controller.extend({
+  conversation: alias('model')


ember inspector expects a model property on the controller, so I started using this alias instead of setupController we usually do. If we don't want this, I'm fine with reverting to setupController

begedin · 2017-12-15T13:04:04Z

app/controllers/project/messages/new.js

+
+export default Controller.extend({
+  message: alias('model'),
+  user: alias('model.conversations.firstObject.user')


This, like many other places in the code, assumes a single user per message, meaning a single conversation as well.

As we add UI support for multiple users, we will need to update this.

begedin · 2017-12-15T13:06:25Z

app/router.js

@@ -65,9 +65,13 @@ Router.map(function() {

  this.route('project', { path: '/:slugged_route_slug/:project_slug' }, function() {
    this.route('checkout'); // Where you enter your credit card details
+    this.route('contributors');


contributors was under settings, meaning, since settings is owner only, contributors was also owner only. To make it admin or owner, I had to move it.

If we move it back, each settings subroute will have to implement it's own CanMixin, while the base settings route should not have it.

If we want it here, but keep the old /:orgslug/slug/settings/contributors url, we can just set the path here.

Personally, I prefer the new placement and the new :orgslug/:slug/contributors url. I would even move donations in the same way

I agree with this move.

begedin · 2017-12-15T13:06:51Z

app/routes/project/contributors.js

+    // 1. Sideload project user records
+    // 2. Have the server compute an ability table for the current user
+    return get(project, 'projectUsers').then(() => {
+      if (this.cannot('administer project', project)) {


used to be cannot('manage project', project)

begedin · 2017-12-15T13:11:11Z

app/routes/project/messages.js

+    // peekAll returns a live array, so it will alse render any newly created
+    // messages, causing the list to update properly on project.messages.new
+    await store.query('conversation', { project_id: projectId });
+    return store.peekAll('conversation');


We have a list-detail UI here

a list of conversations in the sidebar on /messages/

clicking one of the items navigates to /messages/:id, and renders the sidebar in the outlet of the main area, while still displaying the list

navigating to /messages/new does the same

This model hook allows the store to detect a new conversation record and show it in the list (styled differently).

It's hacky, but it's the only way I could find to do this.

An alternative would be to clone the query result of the project.messages model into an array, then render that array in controller, instead of the query result directly.

That would allow us to push the new conversation into that array during the messages.new afterModel hook.

Pushing into the query result directly, which is what I initially tried, does not seem to be possible without resorting to using private functions and properties of DS.model

begedin · 2017-12-15T13:11:47Z

app/routes/project/messages/new.js

+    });
+
+    let conversation = store.createRecord('conversation', { user });
+    get(message, 'conversations').pushObject(conversation);


This causes the conversation to appear in the list of conversations in the sidebar.

begedin · 2017-12-15T13:13:04Z

app/routes/project/messages/new.js

+      // Since it's created without an ID, the store will not detect the
+      // association was saved and will keep it around.
+      // We need to store a reference to it, so we can unload it on save
+      let unsavedConversation = get(message, 'conversations.firstObject');


Comment explains it. Since we store save the two records (message and conversation) together as one, ember does not detect the conversation child has changed, so we need to unload it manually so it gets replaced with the saved record.

begedin · 2017-12-15T13:13:21Z

app/routes/project/messages/new.js

+
+        if (get(conversation, 'isNew')) {
+          conversation.destroyRecord();
+        }


This will, unfortunately, mess with our proposed mixin.

begedin · 2017-12-15T13:14:59Z

app/serializers/message.js

+    // support saving child conversations alongside record
+    if (!json.id) {
+      json.included = snapshot.hasMany('conversations').map((c) => c.serialize());
+    }


This is how we include the conversation alongside the message in our payload when creating a message.

Apparently, ember data does not do this automatically and provides no configuration in order to do it, so I had to provide a manual implementation. It's according to spec, as far as I know, but the sad reality is, this whole process involves custom code both API and client side.

This looks about right!

snewcomer

Wow. This is phenomenal work!

snewcomer · 2017-12-15T18:37:08Z

app/models/conversation-part.js

+  body: attr(),
+  readAt: attr(),
+
+  author: belongsTo('user', { async: true }),


I think async: true is default...but I also like the explicitness...

It wasn't always, and I was about to ask if it is now finally the default.

snewcomer · 2017-12-15T18:40:57Z

app/routes/project/messages.js

+  },
+
+  async model() {
+    console.log('model');


joshsmith

Some updates on the changes I made today:

removed skills from the user list
redesigned the users list some
other general design changes
fixed the test failures
notes below

joshsmith · 2017-12-16T01:14:43Z

app/router.js

      this.route('new');
    });
+    this.route('donate'); // Where you choose your donation amount
+    this.route('people');


I've reworked the conventions to be:

/contributors => /people

/messages => /conversations

The latter is more appropriate because /messages would actually be a different route entirely. These would be the automated/manual messages the project has sent/is actively sending to users. These would be more like /messages/auto and /messages/manual, anyway.

/converations serves more as a unified inbox for the project's conversations with users and replies to messages.

joshsmith · 2017-12-16T01:17:14Z

app/templates/project/conversations.hbs

+<div class="container">
+  <div class="conversations">
+    <section>
+      <aside>


I have yet to add the styling for .conversations but it will differ from the .panel styling. The left sidebar will be a scrolling list of messages and the right side will contain two panes: left for the message thread, right for the information about the target user or project. In total, then, the UI will appear to have three panes, but the right two panes are basically the outlet for the individual conversation.

joshsmith · 2017-12-16T01:41:44Z

Also note that creating a message in the UI prompts you to confirm save of the record before redirecting, even though we're redirecting after saving the record.

joshsmith · 2017-12-17T01:32:40Z

We should have a follow-up issue to change the includes serializer behavior introduced here (and in our API) whenever JSON API 1.1 is released and available in ember-data.

joshsmith · 2017-12-17T01:36:03Z

I don't want to spend a bunch of time writing acceptance tests for logic that's going to change (in terms of new message creation), so I think we should instead:

write a component for a new message modal on the /project/people page
move the logic of /conversations/new to either /project/people or the new modal, depending on what makes sense
remove the /conversations/new
write a component test
fix acceptance tests

joshsmith · 2017-12-17T10:14:55Z

app/routes/project/conversations/first.js

+    let { conversations } = this.modelFor('project.conversations');
+    if (get(conversations, 'length') > 0) {
+      this.transitionTo('project.conversations.conversation', get(conversations, 'firstObject'));
+    }


I don't have a better way to handle this. Could use thoughts @begedin. The downside here is not getting the .active class on the project menu at all, which we want. We could force it some other way, but it's not ideal.

Fixed by using conversations.index redirect.

joshsmith · 2017-12-17T10:19:38Z

I think it's possible we're missing a message.readAt. I don't see any way to get the time the first message was read otherwise.

joshsmith · 2017-12-18T01:48:12Z

In response to my above comment, I think we might be able to infer whether the conversation was read based on whether the conversation has a readAt, but I'm not positive about this.

joshsmith · 2017-12-18T05:08:06Z

A lot of changes above:

redirects to firstObject (the first open conversation) from the conversations.index route
sockets fully working
adds avatars to conversation parts
adds tooltips to those avatars
adds conversation-thread component that has some automagical scrolling behavior on first load / posting a new message
adds new message composer for the conversation

The only things half-implemented in the project messages UI are:

read status (can be implemented later)
open/closed filtering
opening/closing conversation

To sum up what's missing outside the scope of this specific PR, but which will need to be added before anything here is merged into develop (using this as the base branch, once things are deemed passing here):

channel subscriptions to add new conversations into the sidebar list
notification badge in the project menu for project messages
user-facing messages UI
channels subscriptions for the user
sending a message when joining a project
sending a message when accepting/denying a contributor
"X is typing" channel events
online presence for each user in the conversation (can be shown in the /projects/people UI, too)
notification badge in the header for user messages
notifications in the favicon
audible notifications

begedin · 2017-12-18T06:32:08Z

app/services/conversation-channel.js

+  _onNewMessage(conversation) {
+    let store = get(this, 'store');
+    let id = get(conversation, 'id');
+    store.findRecord('conversation', id);


Could we not send the json payload for the conversation part with the event and then simply do store.pushPayload?

This would save us two queries at least.

We cannot do that because the user who sent the message would have the payload pushed twice, and ember-data would rightly reject it.

There also may be data that has changed on the backend since the payload was sent, so best to simply update the parent model, IMO.

joshsmith · 2017-12-18T20:54:34Z

tests/integration/components/conversations/conversation-thread-test.js

+  assert.expect(1);
+
+  set(this, 'onSend', (body) => {
+    assert.equal(body, 'foo', 'Correct value was send with action.');


joshsmith · 2017-12-20T01:23:50Z

Wow, 3,333 lines of code exactly. 👏

begedin commented Dec 15, 2017

View reviewed changes

begedin mentioned this pull request Dec 15, 2017

Add support for creation of associated conversations with message via params code-corps/code-corps-api#1301

Merged

3 tasks

snewcomer reviewed Dec 15, 2017

View reviewed changes

joshsmith reviewed Dec 16, 2017

View reviewed changes

joshsmith added the in progress label Dec 16, 2017

joshsmith assigned begedin Dec 16, 2017

joshsmith changed the title ~~Project messages page~~ Project conversations route Dec 16, 2017

joshsmith unassigned begedin Dec 17, 2017

joshsmith reviewed Dec 17, 2017

View reviewed changes

joshsmith assigned begedin Dec 18, 2017

begedin commented Dec 18, 2017

View reviewed changes

begedin mentioned this pull request Dec 18, 2017

Modify Conversation serializer to handle includes based on JSON API sideposting draft #1624

Open

joshsmith reviewed Dec 18, 2017

View reviewed changes

joshsmith force-pushed the 1592-project-messages branch from 9529832 to c24b18c Compare December 18, 2017 22:47

joshsmith added this to the Messages milestone Dec 19, 2017

joshsmith added blocked and removed in progress labels Dec 19, 2017

begedin changed the title ~~Project conversations route~~ Conversations Feature Dec 19, 2017

begedin changed the title ~~Conversations Feature~~ Project Messages Feature Dec 19, 2017

begedin mentioned this pull request Dec 19, 2017

Update socket connection based on session state #1627

Merged

joshsmith changed the title ~~Project Messages Feature~~ Conversations Feature Dec 19, 2017

Add project and user conversations

608b0f0

joshsmith force-pushed the 1592-project-messages branch from b3bb2e2 to 608b0f0 Compare December 20, 2017 01:23

joshsmith added ready to merge and removed blocked labels Dec 20, 2017

joshsmith approved these changes Dec 20, 2017

View reviewed changes

joshsmith merged commit f2bbebf into develop Dec 20, 2017

joshsmith deleted the 1592-project-messages branch December 20, 2017 01:35

daveconnis mentioned this pull request Dec 20, 2017

Add conversation-part-closed component #1638

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conversations Feature #1597

Conversations Feature #1597

begedin commented Dec 11, 2017 •

edited

Loading

begedin commented Dec 15, 2017

begedin Dec 15, 2017

begedin Dec 15, 2017

begedin Dec 15, 2017

begedin Dec 15, 2017

joshsmith Dec 15, 2017

begedin Dec 15, 2017

begedin Dec 15, 2017

begedin Dec 15, 2017

begedin Dec 15, 2017

begedin Dec 15, 2017

begedin Dec 15, 2017

snewcomer Dec 15, 2017 •

edited

Loading

snewcomer left a comment

snewcomer Dec 15, 2017

joshsmith Dec 15, 2017

snewcomer Dec 15, 2017

joshsmith left a comment •

edited

Loading

joshsmith Dec 16, 2017

joshsmith Dec 16, 2017

joshsmith commented Dec 16, 2017

joshsmith commented Dec 17, 2017 •

edited

Loading

joshsmith commented Dec 17, 2017 •

edited

Loading

joshsmith Dec 17, 2017

joshsmith Dec 17, 2017

joshsmith commented Dec 17, 2017

joshsmith commented Dec 18, 2017

joshsmith commented Dec 18, 2017 •

edited

Loading

begedin Dec 18, 2017

joshsmith Dec 18, 2017

joshsmith Dec 18, 2017

joshsmith commented Dec 20, 2017

Conversations Feature #1597

Conversations Feature #1597

Conversation

begedin commented Dec 11, 2017 • edited Loading

begedin commented Dec 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

snewcomer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshsmith left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshsmith commented Dec 16, 2017

joshsmith commented Dec 17, 2017 • edited Loading

joshsmith commented Dec 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshsmith commented Dec 17, 2017

joshsmith commented Dec 18, 2017

joshsmith commented Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshsmith commented Dec 20, 2017

begedin commented Dec 11, 2017 •

edited

Loading

snewcomer Dec 15, 2017 •

edited

Loading

joshsmith left a comment •

edited

Loading

joshsmith commented Dec 17, 2017 •

edited

Loading

joshsmith commented Dec 17, 2017 •

edited

Loading

joshsmith commented Dec 18, 2017 •

edited

Loading