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

fix: 🍰 Comment Counters Are Now Equal #3769

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
77 changes: 65 additions & 12 deletions webapp/components/CommentList/CommentList.spec.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { config, mount } from '@vue/test-utils'
import CommentList from './CommentList'
import CommentCard from '~/components/CommentCard/CommentCard'
import Vuex from 'vuex'
import Vue from 'vue'

Expand All @@ -26,6 +25,23 @@ describe('CommentList.vue', () => {
id: 'comment134',
contentExcerpt: 'this is a comment',
content: 'this is a comment',
author: {
id: 'some-user',
slug: 'some-slug',
},
},
{
id: 'comment135',
Copy link
Member Author

Choose a reason for hiding this comment

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

Tirokk Authored by Tirokk
Aug 7, 2020


Outdated (history rewrite) - original diff


@@ -28,6 +28,20 @@ describe('CommentList.vue', () => {
               content: 'this is a comment',
               author: { id: 'some-user' },
             },
+            {
+              id: 'comment135',
+              contentExcerpt: 'this is a deleted comment',
+              content: 'this is a deleted comment',
+              deleted: true,
+              author: { id: 'some-user' },
+            },
+            {
+              id: 'comment136',
+              contentExcerpt: 'this is a disabled comment',
+              content: 'this is a disabled comment',
+              disabled: true,
+              author: { id: 'some-user' },
+            },

Okay. Cool!
Then this test extension is needed. 😍

contentExcerpt: 'this is a deleted comment',
content: 'this is a deleted comment',
deleted: true,
author: { id: 'some-user' },
},
Copy link
Member Author

Choose a reason for hiding this comment

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

Tirokk Authored by Tirokk
Aug 4, 2020


Outdated (history rewrite) - original diff


@@ -28,6 +28,13 @@ describe('CommentList.vue', () => {
               content: 'this is a comment',
               author: { id: 'some-user' },
             },
+            {
+              id: 'comment135',
+              contentExcerpt: 'this is a deleted comment',
+              content: 'this is a deleted comment',
+              deleted: true,
+              author: { id: 'some-user' },
+            },

Because of a different solution I suggest in the next comment👇🏼 this test makes no sense anymore.

Would be cool to replace it with a test in backend/src/schema/resolvers/posts.spec.js which checks that only comments not deleted and not disabled are counted by the Cypher queries.

Copy link
Member Author

Choose a reason for hiding this comment

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

Elweyn Authored by Elweyn
Aug 5, 2020


Thanks for the response. I think it is important to check if their are deleted comments that it does not break the other tests, so we could let this one and add another one in backend/src/schema/resolvers/posts.spec.js to check if the count is setten correctly what do you think?

{
id: 'comment136',
contentExcerpt: 'this is a disabled comment',
content: 'this is a disabled comment',
disabled: true,
author: { id: 'some-user' },
},
],
Expand All @@ -35,7 +51,7 @@ describe('CommentList.vue', () => {
getters: {
'auth/isModerator': () => false,
'auth/user': () => {
return {}
return { id: 'some-user' }
},
},
})
Expand Down Expand Up @@ -70,7 +86,7 @@ describe('CommentList.vue', () => {
})
}

it('displays a comments counter', () => {
it('displays a comments counter that ignores disabled and deleted comments', () => {
wrapper = Wrapper()
expect(wrapper.find('.count').text()).toEqual('1')
})
Expand Down Expand Up @@ -101,26 +117,63 @@ describe('CommentList.vue', () => {
})
})

describe('Comment', () => {
describe('Respond to Comment', () => {
beforeEach(() => {
wrapper = Wrapper()
})

it('Comment emitted reply()', () => {
wrapper.find(CommentCard).vm.$emit('reply', {
id: 'commentAuthorId',
slug: 'ogerly',
})
Vue.nextTick()
it('emits reply to comment', () => {
wrapper.find('.reply-button').trigger('click')
expect(wrapper.emitted('reply')).toEqual([
[
{
id: 'commentAuthorId',
slug: 'ogerly',
id: 'some-user',
slug: 'some-slug',
},
],
])
})
})

describe('edit Comment', () => {
beforeEach(() => {
wrapper = Wrapper()
})

it('updates comment after edit', () => {
wrapper.vm.updateCommentList({
id: 'comment134',
contentExcerpt: 'this is an edited comment',
content: 'this is an edited comment',
author: {
id: 'some-user',
slug: 'some-slug',
},
})
expect(wrapper.props('post').comments[0].content).toEqual('this is an edited comment')
})
})

describe('delete Comment', () => {
beforeEach(() => {
wrapper = Wrapper()
})

// TODO: Test does not find .count = 0 but 1. Can't understand why...
it.skip('sets counter to 0', async () => {
wrapper.vm.updateCommentList({
id: 'comment134',
contentExcerpt: 'this is another deleted comment',
content: 'this is an another deleted comment',
deleted: true,
author: {
id: 'some-user',
slug: 'some-slug',
},
})
await Vue.nextTick()
await expect(wrapper.find('.count').text()).toEqual('0')
})
})
})
})
17 changes: 11 additions & 6 deletions webapp/components/CommentList/CommentList.vue
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
<template>
<div id="comments" class="comment-list">
<h3 class="title">
<counter-icon icon="comments" :count="postComments.length" />
<counter-icon icon="comments" :count="commentsCount" />
{{ $t('common.comment', null, 0) }}
</h3>
Copy link
Member Author

Choose a reason for hiding this comment

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

Tirokk Authored by Tirokk
Aug 4, 2020


Outdated (history rewrite) - original diff


@@ -1,7 +1,10 @@
 <template>
   <div id="comments" class="comment-list">
     <h3 class="title">
-      <counter-icon icon="comments" :count="postComments.length" />
+      <counter-icon
+        icon="comments"
+        :count="postComments.filter((comment) => !comment.deleted).length"

I looked at the other place in webapp/components/PostTeaser/PostTeaser.vue where the counter is set on a post teaser. This is the part which differs from that here.
Because I know this value of post.commentsCount comes direct from the backend and because I have found this doesn’t count comment.deleted or comment.disabled I will suggest this solution to be taken over here.

Suggested change
</h3>
:count="post.commentsCount“

A test in backend/src/schema/resolvers/posts.spec.js is missing which checks that only comments not deleted and not disabled are counted by the Cypher queries.
If you are motivated would be nice to implement this one. 💫🍀

Copy link
Member Author

Choose a reason for hiding this comment

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

Elweyn Authored by Elweyn
Aug 5, 2020


Thanks a lot, this one seems a lot better than a filter on the deleted comments!
I will implement this so the frontend code stays clean of logic.

<div v-if="postComments" id="comments" class="comments">
<div v-if="post.comments" id="comments" class="comments">
<comment-card
v-for="comment in postComments"
v-for="comment in post.comments"
:key="comment.id"
:comment="comment"
:postId="post.id"
Expand Down Expand Up @@ -36,8 +36,13 @@ export default {
},
},
computed: {
Copy link
Member Author

Choose a reason for hiding this comment

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

Tirokk Authored by Tirokk
Aug 7, 2020


Outdated (history rewrite) - original diff


@@ -39,6 +39,14 @@ export default {
     postComments() {
       return (this.post && this.post.comments) || []
     },

This code is now superfluous, I think.

postComments() {
return (this.post && this.post.comments) || []
commentsCount() {
return (
(this.post &&
this.post.comments &&
this.post.comments.filter((comment) => !comment.deleted && !comment.disabled).length) ||
0
)
},
},
methods: {
Expand All @@ -48,7 +53,7 @@ export default {
return anchor === '#comments'
},
updateCommentList(updatedComment) {
this.postComments = this.postComments.map((comment) => {
this.post.comments = this.post.comments.map((comment) => {
return comment.id === updatedComment.id ? updatedComment : comment
})
},
Expand Down