Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add conversation-part-closed component #1638

Merged
merged 18 commits into from
Jan 16, 2018
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions app/components/conversations/conversation-part-closed.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import Component from '@ember/component';
import { inject as service } from '@ember/service';
import { computed, get } from '@ember/object';
import { alias } from '@ember/object/computed';

export default Component.extend({

author: null,

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

closedAt: null,

currentUser: service(),

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

user: alias('currentUser.user')
});
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
classNameBindings: ['isSelf:conversation-part--is-self'],

author: null,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<div class="coversation-part__message-wrapper">
<div class="conversation-part__timestamps-wrapper">
<div class="conversation-part__timestamps">
<span data-test-closed-at>
{{#if isSelf}}
You closed this {{moment-from-now closedAt interval=60000}}
{{else}}
{{author.username}} closed this {{moment-from-now closedAt interval=60000}}
{{/if}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have indentation issues here.

</span>
</div>
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In line with my comment above, you can remove all the -closed parts from all the classes here.

23 changes: 15 additions & 8 deletions app/templates/components/conversations/conversation-thread.hbs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<div class="conversation-thread-inner">
<div class="conversation-thread__messages">
{{#if conversation.message.isLoaded}}
{{conversations/conversation-part
{{conversations/conversation-part-comment
author=conversation.message.author
body=conversation.message.body
readAt=conversation.message.readAt
Expand All @@ -10,16 +10,23 @@
{{/if}}
{{#each conversation.conversationParts as |conversationPart|}}
{{#if conversationPart.isLoaded}}
{{conversations/conversation-part
author=conversationPart.author
body=conversationPart.body
readAt=conversationPart.readAt
sentAt=conversationPart.insertedAt
}}
{{#if (eq conversationPart.partType 'isClosed')}}
{{conversation/conversation-part-closed
author=conversationPart.author
closedAt=conversationPart.insertedAt
}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation here is off.

{{else}}
{{conversations/conversation-part-comment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your indentation here is wrong.

author=conversationPart.author
body=conversationPart.body
readAt=conversationPart.readAt
sentAt=conversationPart.insertedAt
}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation here is also off.

{{/if}}
{{/if}}
{{/each}}
</div>
<div class="conversation-thread__composer">
<div class="conversation-thread__composer">
{{conversations/conversation-composer
send=(pipe (action send) (action scrollBottom))
}}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { moduleForComponent, test } from 'ember-qunit';
import hbs from 'htmlbars-inline-precompile';
import { set } from '@ember/object';
import PageObject from 'ember-cli-page-object';
import component from 'code-corps-ember/tests/pages/components/conversations/conversation-part';
import stubService from 'code-corps-ember/tests/helpers/stub-service';
import moment from 'moment';

let page = PageObject.create(component);

function renderPage() {
page.render(hbs`
{{conversations/conversation-part-closed
author=author
closedAt=closedAt
}}
`);
}

moduleForComponent('conversations/conversation-part-closed', 'Integration | Component | conversations/conversation part closed', {
integration: true,
beforeEach() {
page.setContext(this);
},
afterEach() {
page.removeContext();
}
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love feedback on test statements on 21 + 25.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

test('if current user closes message, "You closed this" is rendered', function(assert) {
assert.expect(1);

let user = {
id: 1,
username: 'testuser'
};
set(this, 'author', user);
stubService(this, 'current-user', { user });

let twoMinutesAgo = moment().subtract(2, 'minutes');
let twoMinutesAgoFriendly = twoMinutesAgo.from();
set(this, 'closedAt', twoMinutesAgo);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

renderPage();

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

test('if someone other than the current user closes the message, "Author.username closed this at" is rendered', function(assert) {
assert.expect(1);

let user = {
id: 1,
username: 'currentuser'
};
stubService(this, 'current-user', { user });

let author = {
id: 2,
username: 'authoruser'
};
set(this, 'author', author);

let twoMinutesAgo = moment().subtract(2, 'minutes');
let twoMinutesAgoFriendly = twoMinutesAgo.from();
set(this, 'closedAt', twoMinutesAgo);

renderPage();

let expectedText = `${author.username} closed this ${twoMinutesAgoFriendly}`;
assert.equal(page.closedAt.text, expectedText, 'The closed at timestamp is rendered');
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...'

Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,15 @@ let page = PageObject.create(component);

function renderPage() {
page.render(hbs`
{{conversations/conversation-part
{{conversations/conversation-part-comment
author=author
body=body
sentAt=sentAt
}}
`);
}

moduleForComponent('conversations/conversation-part', 'Integration | Component | conversations/conversation part', {
moduleForComponent('conversations/conversation-part-comment', 'Integration | Component | conversations/conversation part comment', {
integration: true,
beforeEach() {
page.setContext(this);
Expand Down Expand Up @@ -53,7 +53,7 @@ test('it renders all the details', function(assert) {

assert.equal(page.body.text, body, 'The body renders in the chat bubble');
assert.equal(page.sentAt.text, '2 days ago', 'The sent at timestamp renders');
assert.notOk(page.sentByCurrentUser, 'Does not have the current user styles');
assert.notOk(page.isByCurrentUser, 'Does not have the current user styles');
assert.equal(page.photo.url, user.photoThumbUrl, 'The user photo renders');
assertTooltipNotRendered(assert);

Expand Down Expand Up @@ -82,7 +82,7 @@ test('when the current user did not send the message', function(assert) {

renderPage();

assert.notOk(page.sentByCurrentUser, 'Does not have the current user styles');
assert.notOk(page.isByCurrentUser, 'Does not have the current user styles');
});

test('when the current user sent the message', function(assert) {
Expand All @@ -93,5 +93,5 @@ test('when the current user sent the message', function(assert) {

renderPage();

assert.ok(page.sentByCurrentUser, 'Has the current user styles');
assert.ok(page.isByCurrentUser, 'Has the current user styles');
});
12 changes: 9 additions & 3 deletions tests/pages/components/conversations/conversation-part.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,14 @@ export default {
scope: '[data-test-body]'
},

closedAt: {
scope: '[data-test-closed-at]'
},

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


photo: {
scope: '[data-test-target-photo]',
url: attribute('src')
Expand All @@ -23,7 +31,5 @@ export default {

sentAt: {
scope: '[data-test-sent-at]'
},

sentByCurrentUser: hasClass('conversation-part--is-self')
}
};