Add conversation-part-closed component #1638

daveconnis · 2017-12-20T20:59:42Z

What's in this PR?

Adding a 'conversation-part-closed' component.

References #1636

Progress on: #1597

daveconnis · 2017-12-20T21:02:34Z

Clocking out for the day. Working through this is hard, but I think i have a semi-direction? It could be/probably is wrong, but my plan when I start tomorrow is to write a computed object that gets the author (either part.author.id or currentUser.user.id) and then passes it to the hbs which is written-ish (I know I'm missing stuff.)

daveconnis · 2017-12-20T21:03:38Z

app/components/conversations/conversation-part-closed.js

+  isSelf: computed('author', 'user', function() {
+    return get(this, 'author.id') === get(this, 'user.id');
+
+    closedUser: computed ('isSelf', 'currentUser', function() {


This is the component I'm talking about. Hoping that this will compute what author will be passed into the HBS.

Your reasoning here is good, but remember, the component will spell out You closed this or {{username}} closed this, so you don't really care about the closedUser. Your isSelf is correct, so what you need now is to modify your template and replace {{#if currentUser}} with {{#if isSelf}}.

joshsmith · 2017-12-20T22:20:08Z

app/templates/components/conversations/conversation-part-closed.hbs

@@ -0,0 +1,9 @@
+<div class="conversation-part-closed">


This div is not necessary since the component itself creates a div with this class (using the classNames you defined).

begedin · 2017-12-21T16:00:58Z

app/components/conversations/conversation-part-closed.js

+  isSelf: computed('author', 'user', function() {
+    return get(this, 'author.id') === get(this, 'user.id');
+
+    closedUser: computed ('isSelf', 'currentUser', function() {


Your reasoning here is good, but remember, the component will spell out You closed this or {{username}} closed this, so you don't really care about the closedUser. Your isSelf is correct, so what you need now is to modify your template and replace {{#if currentUser}} with {{#if isSelf}}.

begedin · 2017-12-21T16:02:19Z

app/templates/components/conversations/conversation-part-closed.hbs

+    You closed this at...
+    {{else}}
+    <div class="conversation-part-closed__message-wrapper">
+      {{author.username}} closed this at...


See comment above on what to do here. Also, your syntax is incorrect. You use the if helper like this:

{{#if something}} Foo {{else}} Bar {{/if}} Note the indentations, the `#` and the `{{/if}}`

…mment

begedin · 2017-12-21T17:15:08Z

app/components/conversations/conversation-part-closed.js

+    return get(this, 'author.id') === get(this, 'user.id');
+
+
+  })


Blank lines need to go

daveconnis · 2018-01-01T21:19:40Z

tests/integration/components/conversations/conversation-part-closed-test.js

+      author=author
+      closedAt=closedAt
+    }}
+  `);

 moduleForComponent('conversations/conversation-part-closed', 'Integration | Component | conversations/conversation part closed', {
  integration: true
 });



Would love feedback on test statements on 21 + 25.

For future reference, you should comment below those lines so we can see them in this view. Also, would be helpful to have some direction in terms of what you want feedback on specifically?

daveconnis · 2018-01-01T21:20:22Z

tests/pages/components/conversations/conversation-part-closed.js

 } from 'ember-cli-page-object';

 export default {
-  title: text('h1')


Would also love feed back on page object. Think I have everything I need though, at least that I've found I need so far.

joshsmith · 2018-01-01T22:11:20Z

@daveconnis remember to set the label, too.

begedin

Commented on the component file. Also, not to forget, you need to rename the other conversation-part files - the .hbs, the integration/.../...-test.js and the pages/.../conversation-part.js

begedin · 2018-01-02T11:31:35Z

tests/integration/components/conversations/conversation-part-closed-test.js

+test('it renders', function(assert) {
+
+  // Set any properties with this.set('myProperty', 'value');
+  // Handle any actions with this.on('myAction', function(val) { ... });


Don't forget to remove this test.

begedin · 2018-01-02T11:32:15Z

tests/integration/components/conversations/conversation-part-closed-test.js

+
+test('if user closes message, "You closed this at" is rendered', function(assert){
+
+})


These blank tests seems like they would test correct behavior, so the thinking is probably sound. Just need to implement them.

begedin · 2018-01-02T11:48:05Z

app/components/conversations/conversation-part-closed.js

+
+  isSelf: computed('author', 'user', function() {
+    return get(this, 'author.id') === get(this, 'user.id');
+  })


Two problems here

It's not clear what author is

You specify the value of author through the .hbs, as exampled in the test module you wrote:

{{conversations/conversation-part-closed author=author closedAt=closedAt}}

When we do that, we also add that property into the .js and give it a default value, usually null, unless some other value makes sense. You already did this with closedAt: null, so you should do the same with author: null.

The code will work regardless, but we do this so that, when someone is reading the .js, they do not need to navigate back and forth between the .js and the .hbs and the component is relatively clear from the .hbs alone.

user is not defined at all

The author is whichever user is responsible for this specific conversation part being created.

The user is supposed to be whichever user is currently signed in to the app. You get it from the currentUser service as currentUser.user, so to have it available to you as just user, you need to define an alias. You can look at the basic conversation-part-comment component to see how that's done.

daveconnis · 2018-01-02T21:21:13Z

Clocking out for the day. Started working on tests for conversation-part-closed and realized I had a lot of tests failing. Went through the tests and got most passing by changing names from conversation-part to conversation-part-comment (had messed the switch up on the first go). The failing tests dipped into two or three different tests, all are passing except code-corps-ember/tests/acceptance/project-conversations-test.js.

test('System is notified of new conversation part', function(assert) {
  assert.expect(3);

  let { project, user } = server.create('project-user', { role: 'admin' });
  authenticateSession(this.application, { user_id: user.id });

  let [conversation] = createConversations(1, project, user);

  page.visit({
    organization: project.organization.slug,
    project: project.slug
  });

  andThen(() => {
    page.conversations(0).click();
  });

  andThen(() => {
    assert.equal(page.conversationThread.conversationParts().count, 1, 'Just the message head is rendered.');
    server.create('conversation-part', { conversation });
  });

  andThen(() => {
    assert.equal(page.conversationThread.conversationParts().count, 1, 'No notification yet, so new part was not rendered.');
    let conversationChannelService = this.application.__container__.lookup('service:conversation-channel');
    let socket = get(conversationChannelService, 'socket.socket');
    let [channel] = socket.channels;
    channel.trigger('new:conversation-part', {});
  });

  andThen(() => {
    assert.equal(page.conversationThread.conversationParts().count, 2, 'Notification was sent. New part is rendered.');
  });
});

This one is giving me the trouble, specifically

andThen(() => {
    assert.equal(page.conversationThread.conversationParts().count, 2, 'Notification was sent. New part is rendered.');
  });

I'm getting an expected result of 2, but only getting 1 on my test. Considering how were trying to create a second conversation part, I'm assuming that 'channel.trigger('new:conversation-part', {});' is somehow missing the mark. When I change channel.trigger('new:conversation-part', {}); to channel.trigger('new:conversation-part-comment', {}); the test error tells me it's not a model, which makes sense considering in our model app/models/conversation.js, conversationParts: hasMany('conversation-part', { async: true }),. Before I needed to wrap up, I changed this last part to conversationParts: hasMany('conversation-part-comment', { async: true }), and suddenly nothing was rendering and all my asserts for code-corps-ember/tests/acceptance/project-conversations-test.js were failing.

I'm planning on starting here in the morning, but if someone could give me some thoughts on if I'm heading in the right direction, that'd be swell.

begedin

Good progress, but a few things left to do

begedin · 2018-01-03T07:36:47Z

app/templates/components/conversations/conversation-thread.hbs

+          author=conversationPartComment.author
+          body=conversationPartComment.body
+          readAt=conversationPartComment.readAt
+          sentAt=conversationPartComment.insertedAt


So what you probably wanna do here is keep the {{#each conversation.conversationParts as |conversation_part|}} and then do a couple of if-else chains inside the {{#if conversationPart.isLoaded}}

Think of this way. A conversation has a bunch of conversation parts and we want to render all of them in the order we got them in.

Then, for each of them, if it's a comment-type conversation part, render it as a comment type. If it's a closed-type conversation part, render is as a closed type, etc.

{{#each conversation.conversationParts as |conversationPart|}} {{#if conversationPart.isLoaded}} {{#if (eq conversationPart.partType 'closed')}} {{conversations/conversation-part-closed ..}} {{else}} {{conversations/conversation-part-comment ...}} {{/if}} {{/if}} {{/each}}

Note that in the example, the comment type is sort of considered the default - the thing we render if it's not any other type. It's not perfect and really, if we do not support all the types our API supports, we have a bug, but at least this will render something, which is preferable to and easier to detect as an issue than nothing.

There may be more elegant ways to do this, but for now, this is good enough. Then, when we add another conversation part type, such as conversation-part-open, we would add an else if here, between the if and the else.

begedin · 2018-01-03T07:38:09Z

tests/integration/components/conversations/conversation-part-closed-test.js

+  renderPage();
+
+  assert.ok(page.closedAt.text, 'You closed this two days ago', 'The closed at timestamp is rendered');
+});


This one is good, but I would call it 'if the current user closes the message..., just to emphasize the current

The other test is 'if someone other than the current user closes the message...'

daveconnis · 2018-01-03T17:38:02Z

Okay, I'm stuck. It seems like I'm getting conflicting thoughts here. Right now, I'm working on failing tests for code-corps-ember/tests/acceptance/project-conversations-test.js. I'm getting a bunch of

Error: Element not found.

PageObject: 'page.conversationParts(0).body.text'

Which leads me to believe that my path to that part is wrong. This is probably the case considering I've renamed things to conversation-part-comment.

Considering Nikola's earlier comments about app/templates/components/conversations/conversation-thread.hbs

what you probably wanna do here is keep the {{#each conversation.conversationParts as |conversation_part|}} and then do a couple of if-else chains inside the {{#if conversationPart.isLoaded}}

I went back and fixed this, taking the comment out that I'd added yesterday when switching conversation-part to conversation-part-comment. However, app/templates/components/conversations/conversation-thread.hbs
still calls conversations/conversation-part-comment because this is where author, body, readAt, sentAt are all defined.
IE:

{{conversations/conversation-part-comment
        author=conversation.message.author
        body=conversation.message.body
        readAt=conversation.message.readAt
        sentAt=conversation.message.insertedAt

In [https://github.com//issues/1636], you guys stated that you wanted conversation-part to conversation-part-comment. So, I feel like, right now, in order to fix my path, I need to change the name of conversation-part-comment back to conversation-part in order to stay with Nikola's comments, however I'm sure I'm missing something. I'm getting confused that there's a model for Conversation part AND conversation as well as a component for conversation-part-comment and conversation-part-closed I think I'm missing a change in some of the ways the touch together somehow.

daveconnis · 2018-01-04T20:23:58Z

tests/integration/components/conversations/conversation-part-closed-test.js

+
+  set(this, 'author', { user1 });
+  set(this, 'closedAt', moment().from());
+  stubService(this, 'current-user', { user });


I also can't get the "author" nor the "time" to pass in here, either. Been trying for too long so moving onto something else.

There's a misunderstanding here with the author.

{ user } is an object shorthand. Basically, it's a shorter way of writing { user: user }

With that in mind

set(this, 'author', { user1 });

means you're actually doing

set(this, 'author', { user1: user1 });

which clearly wont work.

In your setup, there's no author.username, author.id, etc., there's 'author.user1.username, author.user1.id`, etc.

Thus, what you need to do instead is,

set(this, 'author', user1);

Meaning, the value of author in our component is the same as user1

What might be confusing you is the

stubService(this, 'current-user', { user });

but we actually do a different thing there.

If you take a look at how the actual current-user service is implemented, under app/services/current-user.js, you will see that it's an Ember.Service, with a user property in it, so what we're doing is exactly what we want to do - replace the actual service with a fake one, where the value of user is our user.

That means, we will have currentUser.user, currentUser.user.id, currentUser.user.username, etc., exactly as we want.

As for the time stamp, look for an example in admin-github-event-test.js

The general idea is to get some timestamp, either using Date.now() or moment(). Both will return the exact current time. The example above uses moment().

Then, store that time stamp into a variable. This is critical. You can't just go and do the following, for example:

let timestamp = moment(); assert.equal(timestamp, moment());

The simple reason you can't do this is that both Date.now() and moment() return a different value every time you call them - the current timestamp, meaning the exact time at the time the function was called.

So if we want to test our code here, we need to understand what our component is supposed to do - The intent of the test is to ensure that the component does what it supposed to do.

Our component is supposed to tell us who closed our conversation and when.

The who will be either the username of the author, or just "You" if the author is the current user, so we need two tests for that, which I think you'll know how to cover.

The "when" will be the time of the closing event, meaning the time the conversation part of "partType" "closed" was created (or, in terms of the code, the value of closedAt). We also want the when to nicely formatted in some manner, which is something I described in my comment above.

To give a few examples for the username part

If the current user is begedin, and the person who closed the conversation is daveconnis, we want the text to contain "daveconnis closed this"

If the current user is daveconnis and daveconnis closed the conversation, we want the text to say "You closed this"

As for the timestamp, it gets a bit more complicated because you sort of need to know how the friendly formatting works, but what you would do is something like

Create a couple of timestamps at varying moments in the past

For each of them

set the value of closedAt to that

check if the text contains the friendly version of that timestamp

due to some weird ember internals, you need to use the run loop to do that, otherwise it won't update

As a concrete example

// moment() returns the current time. `subtract(amount, type)` then modifies that let twoMinutesAgo = moment().subtract(2, 'minutes'); // from() is the friendly formatted version of the above, same as what the {{moment-from-now}} helper does let twoMinutesAgoFriendly = twoMinutesAgo.from(); // this is where we use the run loop to set the new value run(() => { set(this, 'closedAt', twoMinutesAgo) } // indexOf will return -1 if the text does not contain twoMinutesAgoFriendly // If it does, it will contain the position in the text where it starts, so 0 or higher // checking if it's higher than -1 means we check if the text contains it assert.ok(page.closedAt.text.indexOf(twoMinutesAgoFriendly) > -1)

Then you do the same thing with, for example, two seconds ago, 2 days ago, etc. Actually, just those 3 examples ought to be enough in total.

daveconnis · 2018-01-04T20:24:25Z

tests/integration/components/conversations/conversation-part-closed-test.js

  set(this, 'author', user);
-  set(this, 'closedAt', moment().subtract(2, 'days'));
+  set(this, 'closedAt', moment().fromNow());
  stubService(this, 'current-user', { user: { id: 1 } });



For the life of me, I can't figure out how to get the timestamp to pass in and have been working on it for an very inefficient length of time, by that I mean all day. This thing has kicked my ass to and fro and I'm sick of looking at it and am going to move to something else so the day isn't a complete wash. Nikola has stepped in a bunch with some guidance, but it didn't seem to help my stupidity.

daveconnis · 2018-01-04T20:24:43Z

tests/integration/components/conversations/conversation-part-closed-test.js

  set(this, 'author', user);
-  set(this, 'closedAt', moment().subtract(2, 'days'));
+  set(this, 'closedAt', moment().fromNow());


I found `fromNow()); from the moment docs here .

begedin · 2018-01-04T16:06:59Z

app/templates/components/conversations/conversation-part-closed.hbs

+    You closed this at {{moment-format conversationPart.updatedAt 'MM/DD/YYYY hh:mm:ss'}}
+  {{else}}
+    {{author.username}} closed this at {{moment-format conversationPart.updatedAt 'MM/DD/YYYY hh:mm:ss'}}
+  {{/if}}


Indentation error here.

Still have this indentation error, It starts at line 5, if it's not clear. The whole if-else chunk needs to be indented 3 levels deeper.

begedin · 2018-01-04T16:08:05Z

app/templates/components/conversations/conversation-thread.hbs

      {{#if conversationPart.isLoaded}}
-        {{conversations/conversation-part
+        {{#if (eq conversationPart.partType 'Closed')}}


I'm reasonably sure the type will be lowercased at all times, so this should probably be

{{#if (eq conversationPart.partType 'closed')}}

begedin · 2018-01-04T16:14:14Z

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

-    { isLoaded: true, body: 'foo 3' }
+    { isLoaded:true, body: 'foo 1' },
+    { isLoaded:true, body: 'foo 2' },
+    { isLoaded:true, body: 'foo 3' }


You removed the spaces between isLoaded: and true here. You should put them back. That is the preferred formatting.

begedin · 2018-01-05T08:20:41Z

tests/integration/components/conversations/conversation-part-closed-test.js

+
+  set(this, 'author', { user1 });
+  set(this, 'closedAt', moment().from());
+  stubService(this, 'current-user', { user });


There's a misunderstanding here with the author.

{ user } is an object shorthand. Basically, it's a shorter way of writing { user: user }

With that in mind

set(this, 'author', { user1 });

means you're actually doing

set(this, 'author', { user1: user1 });

which clearly wont work.

In your setup, there's no author.username, author.id, etc., there's 'author.user1.username, author.user1.id`, etc.

Thus, what you need to do instead is,

set(this, 'author', user1);

Meaning, the value of author in our component is the same as user1

What might be confusing you is the

stubService(this, 'current-user', { user });

but we actually do a different thing there.

If you take a look at how the actual current-user service is implemented, under app/services/current-user.js, you will see that it's an Ember.Service, with a user property in it, so what we're doing is exactly what we want to do - replace the actual service with a fake one, where the value of user is our user.

That means, we will have currentUser.user, currentUser.user.id, currentUser.user.username, etc., exactly as we want.

begedin · 2018-01-05T08:36:47Z

app/templates/components/conversations/conversation-part-closed.hbs

+  {{#if isSelf}}
+    You closed this on {{moment-format conversationPart.closedAt 'MM/DD/YYYY hh:mm:ss'}}
+  {{else}}
+    {{author.username}} closed this on {{moment-format conversationPart.closedAt 'MM/DD/YYYY hh:mm:ss'}}


First of all, even if you write the test correctly, it wont pass because your component has a bug here:

It's supposed to be

{{#if isSelf}} You closed this on {{moment-format closedAt 'MM/DD/YYYY hh:mm:ss'}} {{else}} {{author.username}} closed this on {{moment-format closedAt 'MM/DD/YYYY hh:mm:ss'}} {{/if}}

Secondly, I apologize for this, but the task was incorrectly written. We don't want a formatted timestamp, we want a "friendly" timestamp, which we get ,using moment-from instead of moment-format, so what you actually want here is

{{#if isSelf}} You closed this {{moment-from-now closedAt interval=60000}} {{else}} {{author.username}} closed this {{moment-from-now closedAt interval=60000}} {{/if}}

Similar to how the conversation-part-comment does it with its own timestamp.

This will cause the text to say something like

"You closed this a few seconds ago" "Joe closed this a minute ago" "James closed this a couple of hours ago" "etc."

begedin · 2018-01-05T08:52:09Z

tests/integration/components/conversations/conversation-part-closed-test.js

+
+  set(this, 'author', { user1 });
+  set(this, 'closedAt', moment().from());
+  stubService(this, 'current-user', { user });


As for the time stamp, look for an example in admin-github-event-test.js

The general idea is to get some timestamp, either using Date.now() or moment(). Both will return the exact current time. The example above uses moment().

Then, store that time stamp into a variable. This is critical. You can't just go and do the following, for example:

let timestamp = moment(); assert.equal(timestamp, moment());

The simple reason you can't do this is that both Date.now() and moment() return a different value every time you call them - the current timestamp, meaning the exact time at the time the function was called.

So if we want to test our code here, we need to understand what our component is supposed to do - The intent of the test is to ensure that the component does what it supposed to do.

Our component is supposed to tell us who closed our conversation and when.

The who will be either the username of the author, or just "You" if the author is the current user, so we need two tests for that, which I think you'll know how to cover.

The "when" will be the time of the closing event, meaning the time the conversation part of "partType" "closed" was created (or, in terms of the code, the value of closedAt). We also want the when to nicely formatted in some manner, which is something I described in my comment above.

To give a few examples for the username part

If the current user is begedin, and the person who closed the conversation is daveconnis, we want the text to contain "daveconnis closed this"

If the current user is daveconnis and daveconnis closed the conversation, we want the text to say "You closed this"

As for the timestamp, it gets a bit more complicated because you sort of need to know how the friendly formatting works, but what you would do is something like

Create a couple of timestamps at varying moments in the past

For each of them

set the value of closedAt to that

check if the text contains the friendly version of that timestamp

due to some weird ember internals, you need to use the run loop to do that, otherwise it won't update

As a concrete example

// moment() returns the current time. `subtract(amount, type)` then modifies that let twoMinutesAgo = moment().subtract(2, 'minutes'); // from() is the friendly formatted version of the above, same as what the {{moment-from-now}} helper does let twoMinutesAgoFriendly = twoMinutesAgo.from(); // this is where we use the run loop to set the new value run(() => { set(this, 'closedAt', twoMinutesAgo) } // indexOf will return -1 if the text does not contain twoMinutesAgoFriendly // If it does, it will contain the position in the text where it starts, so 0 or higher // checking if it's higher than -1 means we check if the text contains it assert.ok(page.closedAt.text.indexOf(twoMinutesAgoFriendly) > -1)

Then you do the same thing with, for example, two seconds ago, 2 days ago, etc. Actually, just those 3 examples ought to be enough in total.

begedin

Almost there. Only minor details left :)

begedin · 2018-01-09T06:14:27Z

tests/integration/components/conversations/conversation-part-closed-test.js


+  let text = `You closed this ${twoMinutesAgoFriendly}`;
+  assert.equal(page.closedAt.text, text, 'The closed at timestamp is rendered');


Good thinking with this. Easier to read than indexOf.

A further suggestion would be to name it expectedText, so you'd have

let expectedText = //... assert.equal(page.closedAt.text, expectedText, //...

begedin · 2018-01-09T06:15:25Z

app/templates/components/conversations/conversation-part-closed.hbs

+    You closed this at {{moment-format conversationPart.updatedAt 'MM/DD/YYYY hh:mm:ss'}}
+  {{else}}
+    {{author.username}} closed this at {{moment-format conversationPart.updatedAt 'MM/DD/YYYY hh:mm:ss'}}
+  {{/if}}


Still have this indentation error, It starts at line 5, if it's not clear. The whole if-else chunk needs to be indented 3 levels deeper.

daveconnis · 2018-01-10T19:29:45Z

Made the changes that Nikola laid out, got to the end where he said tests would break (they did) but I can't figure out what to do now.

We've changed a lot of things, mostly exactly what Nik asked to change, but what I'm not sure about his HOW tests are breaking due to renaming closedByCurrentUser and sentByCurrentUser into isByCurrentUser.

isComment: hasClass('conversation-part--comment'),
 isClosed: hasClass('conversation-part--closed'),
 isByCurrentUser: hasClass('conversation-part--is-self')

Does adding isComment and isClosed mean we have to add these attribute somewhere in the corresponding hbs files? Where? I can't find where closedByCurrentUserand sentByCurrentUser is defined. I found a few sentBy... and changed them to isByCurrentUser in the conversation part comment test, but I'm just missing how closedByCurrentUserand sentByCurrentUser were interacting with the code, and I feel stupid because Nik laid it all out super clearly and I can't even do that.

joshsmith

It would be much easier to help if you provided output of where the tests are failing and what you've done to diagnose.

joshsmith · 2018-01-10T19:32:10Z

app/components/conversations/conversation-part-comment.js

@@ -4,7 +4,7 @@ import { alias } from '@ember/object/computed';
 import { inject as service } from '@ember/service';

 export default Component.extend({
-  classNames: ['conversation-part'],
+  classNames: ['conversation-part, conversation-part-comment'],


I'm not sure precisely where your tests are breaking, but this is certainly a syntactical problem.

joshsmith · 2018-01-10T19:32:33Z

app/components/conversations/conversation-part-closed.js

+
+  author: null,
+
+  classNames: ['conversation-part-closed'],


This is missing a conversation-part class name.

joshsmith · 2018-01-10T19:32:58Z

app/components/conversations/conversation-part-closed.js

+  author: null,
+
+  classNames: ['conversation-part-closed'],
+  classNameBindings: ['isSelf:conversation-part-closed--is-self'],


Is this needed? I'm not sure what would differ stylistically here so it looks like it's just extra work.

Do you mean classNameBindings: ['isSelf:conversation-part-closed--is-self'],

Right, I'm not sure why it's needed here, since there's nothing special about you vs someone else closing (vs the chat bubble where it's gray vs blue).

joshsmith · 2018-01-10T19:33:12Z

app/models/conversation-part.js

@@ -7,6 +7,7 @@ export default Model.extend({
  insertedAt: attr(),
  readAt: attr(),
  updatedAt: attr(),
+  closedAt: attr(),


This needs alpha-ordered.

joshsmith · 2018-01-10T19:33:25Z

app/templates/components/conversations/conversation-part-closed.hbs

+        You closed this {{moment-from-now closedAt interval=60000}}
+        {{else}}
+        {{author.username}} closed this {{moment-from-now closedAt interval=60000}}
+        {{/if}}


You have indentation issues here.

joshsmith · 2018-01-10T19:34:04Z

app/templates/components/conversations/conversation-thread.hbs

+          {{conversation/conversation-part-closed
+          author=conversationPart.author
+          closedAt=conversationPart.insertedAt
+          }}


Indentation here is off.

joshsmith · 2018-01-10T19:34:09Z

app/templates/components/conversations/conversation-thread.hbs

          author=conversationPart.author
          body=conversationPart.body
          readAt=conversationPart.readAt
          sentAt=conversationPart.insertedAt
-        }}
+          }}


Indentation here is also off.

joshsmith · 2018-01-10T19:34:51Z

tests/pages/components/conversations/conversation-part.js

-  sentByCurrentUser: hasClass('conversation-part--is-self')
+  isComment: hasClass('conversation-part--comment'),
+  isClosed: hasClass('conversation-part--closed'),
+  isByCurrentUser: hasClass('conversation-part--is-self')


The alpha-ordering here is off.

begedin · 2018-01-11T06:10:53Z

app/components/conversations/conversation-part-closed.js

+  author: null,
+
+  classNames: ['conversation-part', 'conversation-part--closed'],
+  classNameBindings: ['isSelf:conversation-part-closed--is-self'],


@joshsmith Your comment got marked as outdated. I still think it's needed for the "displaying it left vs right" part of the style.

@daveconnis This needs to be

classNameBindings: ['isSelf:conversation-part--is-self'],

In case it's not clear, this tells Ember to add a conversation-part--is-self css class to the component's HTML element, whenever the isSelf property on the component is true (or truthy).

We then use that class to style component differently when it's displayed to the user. Like in any other chat interface, if it's your message, or your action, it's written on your side (left vs right), compared to when it's the other user's message, where it's written on their side.

begedin · 2018-01-11T06:16:56Z

app/templates/components/conversations/conversation-thread.hbs

+        readAt=conversationPart.readAt
+        sentAt=conversationPart.insertedAt
+      }}
+    {{/if}}


This whole {{else}} block needs to be indented one level deeper.

begedin · 2018-01-11T06:19:19Z

app/models/conversation-part.js

@@ -4,6 +4,7 @@ import { belongsTo } from 'ember-data/relationships';

 export default Model.extend({
  body: attr(),
+  closedAt: attr(),


There is no closedAt on the model

The component's closedAt property can be set to the model's insertedAt or updatedAt as we discussed. You already do this on line 16 of app/templates/components/conversations/conversation-thread.hbs.

begedin · 2018-01-11T06:38:17Z

tests/pages/components/conversations/conversation-part.js

+
+  isComment: hasClass('conversation-part--comment'),
+  isClosed: hasClass('conversation-part--closed'),
+  isByCurrentUser: hasClass('conversation-part--is-self'),


I saw a question that got marked as outdated, which caused me to think page objects might not be completely clear to you. They are only ever used in tests and they are defined in a way that provides us with information we need to answer questions in tests.

If a conversation thread lists multiple conversations of different partType values, we want to ask

is this part displayed as a comment

is this other part displayed as "user closed at"

We know that a comment has a conversation-part--coment class, and we know that the other one has a conversation-part--closed class, so we could ask

assert.ok($('conversation-part').classNames().indexOf('conversation-part--comment') > -1); assert.ok($('conversation-part').classNames().indexOf('conversation-part--closed') > -1);

and that works, except it's not always completely clear what it is we're actually asking. Also, in multiple tests, we tend to ask the same or similar questions, so we have to repeat all of that. Then, if, for example, the class name changes, we have to change it in all of those places.

What page objects allow us to do is to specify

isClosed: hasClass('conversation-part--closed'),

in just one place and use that.

assert.ok(conversationPart.isClosed); assert.ok(conversationPart.isComment);

That makes the question much clearer. Also, if the class changes, we change it in just that one place - in the page object.

joshsmith · 2018-01-11T20:48:22Z

app/templates/components/conversations/conversation-thread.hbs

+            closedAt=conversationPart.insertedAt
+          }}
+          {{else}}
+          {{conversations/conversation-part-comment


Your indentation here is wrong.

…tus-changed

begedin

Need to reorder properties in app/components/conversations/conversation-part-closed.js and then this is good to go.

daveconnis · 2018-01-15T19:29:07Z

Just realized I can't really finish the "client filtering of parts based on authorization" without making a feature branch off of https://github.com/code-corps/code-corps-ember/pull/1689 which is already a branch on top of https://github.com/code-corps/code-corps-ember/pull/1638. I don't want to have multiple branches contingent on the branch below it, so we need to merge #1638 , then change 1689 base to develop and merge, THEN I can write the filtering component. I'll make a issue for client filtering of parts based on authorization and label it blocked by 1638 and 1689.

joshsmith · 2018-01-15T20:31:46Z

Realistically we don’t want to be merging all these branches into develop when there’s authorization stuff that is unfinished. What this means is that #1689 will have to serve as the base branch and all PRs will be done to it and merged there.

We could also – and this might be easier – branch off develop with some conversation-part- branch (not sure naming) and then use that as the base for all these PRs. That way develop doesn't have stuff in it that's not ready to go live, and the existing PRs can just point at the new branch as their base, which itself serves as the "feature branch" containing the full feature.

Does that make sense?

daveconnis · 2018-01-15T20:45:22Z

I think that makes sense. So I'd create a Conversation-parts branch on develop
What would my base code be, here? Would I just set all my conversation part PRs using this as a base? Would that mean I'd have each PR to work with? My brain is saying I wouldn't because 'conversation-parts' wouldn't actually include the code for my other PRs. So I'd create conversation-parts then merge my existing PRs into conversation-parts, then finish authorization stuff on `conversation-parts'?

begedin · 2018-01-16T07:10:56Z

👍 merged. Good work.

Add conversation-part-closed component Rewrite existing conversation-part as conversation-part-comment

added conversation-part-closed component

c9115be

daveconnis commented Dec 20, 2017

View reviewed changes

joshsmith changed the title ~~added conversation-part-closed component~~ Add conversation-part-closed component Dec 20, 2017

joshsmith reviewed Dec 20, 2017

View reviewed changes

begedin reviewed Dec 21, 2017

View reviewed changes

changed component name from conversation-part to conversation-part-co…

bd476e4

…mment

begedin reviewed Dec 21, 2017

View reviewed changes

fix component-part naming

7da31f3

daveconnis force-pushed the conversation-part-for-open-and-closed-status-changed branch from 79b55de to 7da31f3 Compare December 21, 2017 19:35

daveconnis added the in progress label Dec 21, 2017

write page object and test for component

e5f797d

daveconnis commented Jan 1, 2018

View reviewed changes

daveconnis requested review from joshsmith and begedin January 1, 2018 21:20

begedin requested changes Jan 2, 2018

View reviewed changes

start tests for conversation-part-closed

600c518

begedin requested changes Jan 3, 2018

View reviewed changes

daveconnis added 2 commits January 4, 2018 11:04

work on getting tests to pass

368686b

write tests for conversation-part-closed

36220d5

daveconnis commented Jan 4, 2018

View reviewed changes

begedin requested changes Jan 5, 2018

View reviewed changes

Fix integration tests

f11705c

daveconnis requested a review from begedin January 8, 2018 20:39

daveconnis added the needs code review label Jan 8, 2018

begedin requested changes Jan 9, 2018

View reviewed changes

make changes to dry up code

955cea9

joshsmith reviewed Jan 10, 2018

View reviewed changes

daveconnis added 2 commits January 10, 2018 15:30

change conversation-part/conversation-part-comment with tests passing

27f5947

fix syntax and alpha-ordering

6de8ba1

begedin requested changes Jan 11, 2018

View reviewed changes

This was referenced Jan 11, 2018

Create conversation part reopened component #1684

Closed

Write conversation part reopened component for messages #1689

Merged

fix syntax and remove unnecessary code

372f93e

joshsmith reviewed Jan 11, 2018

View reviewed changes

fix indentation

4144f99

daveconnis requested review from joshsmith and begedin January 11, 2018 20:55

begedin added 2 commits January 15, 2018 18:03

Merge branch 'develop' into conversation-part-for-open-and-closed-sta…

9f5c0da

…tus-changed

Update conversation-thread.hbs

a3f219e

begedin approved these changes Jan 15, 2018

View reviewed changes

begedin requested changes Jan 15, 2018

View reviewed changes

reordered conversation-part-closed to match style guide

b18b605

daveconnis requested a review from begedin January 15, 2018 18:46

daveconnis mentioned this pull request Jan 15, 2018

Add client filtering of conversation part types for authorized users #1692

Open

Update conversation-part-closed.js

f03c50c

begedin approved these changes Jan 16, 2018

View reviewed changes

begedin merged commit 7bfd335 into develop Jan 16, 2018

begedin added ready to merge and removed needs code review labels Jan 16, 2018

jderr-mx pushed a commit to jderr-mx/code-corps-ember that referenced this pull request Mar 13, 2018

Add conversation-part-closed component (code-corps#1638)

27af56d

Add conversation-part-closed component Rewrite existing conversation-part as conversation-part-comment


		test('if user closes message, "You closed this at" is rendered', function(assert){

		})


		let text = `You closed this ${twoMinutesAgoFriendly}`;
		assert.equal(page.closedAt.text, text, 'The closed at timestamp is rendered');

Add conversation-part-closed component #1638

Add conversation-part-closed component #1638

Conversation

daveconnis commented Dec 20, 2017 • edited by joshsmith Loading

What's in this PR?

References #1636

daveconnis commented Dec 20, 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

joshsmith commented Jan 1, 2018

begedin 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

It's not clear what author is

user is not defined at all

daveconnis commented Jan 2, 2018 • edited Loading

begedin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daveconnis commented Jan 3, 2018 • edited Loading

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

Choose a reason for hiding this comment

begedin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daveconnis commented Jan 10, 2018 • edited Loading

joshsmith 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

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

Choose a reason for hiding this comment

begedin left a comment

Choose a reason for hiding this comment

daveconnis commented Jan 15, 2018 • edited Loading

joshsmith commented Jan 15, 2018

daveconnis commented Jan 15, 2018

begedin commented Jan 16, 2018

daveconnis commented Dec 20, 2017 •

edited by joshsmith

Loading

It's not clear what `author` is

`user` is not defined at all

daveconnis commented Jan 2, 2018 •

edited

Loading

daveconnis commented Jan 3, 2018 •

edited

Loading

daveconnis commented Jan 10, 2018 •

edited

Loading

daveconnis commented Jan 15, 2018 •

edited

Loading