fix: 🍰 Comment Counters Are Now Equal #3769

Tirokk · 2020-10-06T09:30:24Z

Authored by Elweyn
Aug 3, 2020
Merged Aug 11, 2020

[WIP] 🍰 Pullrequest 2112

Changed the filter so that the comments that are deleted are not counted in the comment number anymore

Issues

Todo

Test to check if the updated comment get directly updated on the frontend

… we see that the test checks the comment count to ignore disabled and deleted comments.

…ctly after the event is triggered.

…ostComments so that issue are fixed.

…ply to comment edited. Test update comment added. Test delete comment pending.

Tirokk · 2020-10-07T13:23:58Z

Authored by cypress[bot]
Aug 3, 2020

Test summary

66 • 0 • 0 • 0

Run details


Project	Human-Connection
Status	Passed
Commit	`823056b`
Started	Aug 11, 2020 2:22 PM
Ended	Aug 11, 2020 2:45 PM
Duration	22:45 💡
OS	Linux Ubuntu Linux - 16.04
Browser	Firefox 68

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Tirokk · 2020-10-07T13:24:00Z

Authored by cypress[bot]
Aug 3, 2020

Test summary

66 • 0 • 0 • 0

Run details


Project	Human-Connection
Status	Passed
Commit	0c3f8c2af0 ℹ️
Started	Aug 11, 2020 2:23 PM
Ended	Aug 11, 2020 2:47 PM
Duration	24:24 💡
OS	Linux Ubuntu Linux - 16.04
Browser	Firefox 68

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Tirokk · 2020-10-07T13:24:02Z

Authored by Tirokk
Aug 3, 2020

Hey @Elweyn ,

can you run the linter on webapp and on backend.
yarn lint --fix
and push again? That would be great …
That’s the reason why the Travis build fails.

PS: In case you work on the locale files in the webapp please run as well
yarn locales --fix
before you push.
PPS: The semantic PR marker bug: was not correct. fix: is correct in this case … ☝🏼

Tirokk · 2020-10-07T13:24:03Z

Authored by Elweyn
Aug 3, 2020

Hey @Tirokk,
thanks was not sure how the semantic had to be done!

I just made the linter on webapp and backend.

Tirokk · 2020-10-07T13:24:35Z

webapp/components/CommentList/CommentList.spec.js

+              content: 'this is a deleted comment',
+              deleted: true,
+              author: { id: 'some-user' },
+            },


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.

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?

Tirokk · 2020-10-07T13:24:37Z

webapp/components/CommentList/CommentList.vue

@@ -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>


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. 💫🍀

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.

Tirokk · 2020-10-07T13:29:05Z

Authored by Elweyn
Aug 6, 2020

Hi @Tirokk
after @Mogge and I realised that this class made some trouble with the comment tickets.
We decided to change it so that we don't work on the computed postComments anymore but on post.comments.

Since the value of post.commentsCount is not updated as long as we don't make a reload of the page, we decided to make a computed property commentsCount that make use the first written filter.
So I added a new comment that was disabled to make sure that the count is stil 1 even when their is a disabled comment and a deleted comment.

Hope this seems good.
Cheers

Tirokk · 2020-10-07T13:29:16Z

webapp/components/CommentList/CommentList.spec.js

+              },
+            },
+            {
+              id: 'comment135',


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

Tirokk · 2020-10-07T13:29:18Z

webapp/components/CommentList/CommentList.vue

@@ -36,8 +36,13 @@ export default {
    },
  },
  computed: {


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.

Tirokk · 2020-10-07T13:29:20Z

Authored by Mogge
Aug 7, 2020

Testing that a comment is updated after editing would be great.

Mogge

Hey @Elweyn ,
supercool you have cared for this one. 🚀

Glad to have you in our project!

Mogge

Great work. Thanx

fix: 🍰 Comment Counters Are Now Equal

Hannes Heine added 8 commits August 3, 2020 15:08

Added a deleted comment to the Test case

9be480a

Filter out the deleted comments of the comment counter

fe99977

Linter on webapp

084a517

Added test comment that is disabled and changes the test name so that…

d513fec

… we see that the test checks the comment count to ignore disabled and deleted comments.

fix: #3357 now the comment deletion and comment updates are seen dire…

7f8be5d

…ctly after the event is triggered.

Changed the class to use this.post.comments instead of the computed p…

def6a9d

…ostComments so that issue are fixed.

Lint over the frontend code.

34ba9d8

Added the user that writes the comment to the authentication. Test re…

823056b

…ply to comment edited. Test update comment added. Test delete comment pending.

Tirokk commented Oct 7, 2020

View reviewed changes

Mogge requested changes Oct 8, 2020

View reviewed changes

Mogge approved these changes Oct 8, 2020

View reviewed changes

Mogge closed this Oct 8, 2020

Mogge added the Github Import label Oct 8, 2020

ulfgebhardt deleted the pr3769head branch January 7, 2021 03:32

Tirokk pushed a commit that referenced this pull request Jan 18, 2021

Merge pull request #3769 from Human-Connection/Elweyn/issue2112

907b321

fix: 🍰 Comment Counters Are Now Equal

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: 🍰 Comment Counters Are Now Equal #3769

fix: 🍰 Comment Counters Are Now Equal #3769

Tirokk commented Oct 6, 2020

Tirokk commented Oct 7, 2020

Tirokk commented Oct 7, 2020

Tirokk commented Oct 7, 2020

Tirokk commented Oct 7, 2020

Tirokk Oct 7, 2020

Tirokk Oct 7, 2020

Tirokk Oct 7, 2020

Tirokk Oct 7, 2020

Tirokk commented Oct 7, 2020

Tirokk Oct 7, 2020

Tirokk Oct 7, 2020

Tirokk commented Oct 7, 2020

Mogge left a comment

Mogge left a comment

fix: 🍰 Comment Counters Are Now Equal #3769

fix: 🍰 Comment Counters Are Now Equal #3769

Conversation

Tirokk commented Oct 6, 2020

[WIP] 🍰 Pullrequest 2112

Issues

Todo

Tirokk commented Oct 7, 2020

Test summary

Run details

Tirokk commented Oct 7, 2020

Test summary

Run details

Tirokk commented Oct 7, 2020

Tirokk commented Oct 7, 2020

Tirokk Oct 7, 2020

Choose a reason for hiding this comment

Tirokk Oct 7, 2020

Choose a reason for hiding this comment

Tirokk Oct 7, 2020

Choose a reason for hiding this comment

Tirokk Oct 7, 2020

Choose a reason for hiding this comment

Tirokk commented Oct 7, 2020

Tirokk Oct 7, 2020

Choose a reason for hiding this comment

Tirokk Oct 7, 2020

Choose a reason for hiding this comment

Tirokk commented Oct 7, 2020

Mogge left a comment

Choose a reason for hiding this comment

Mogge left a comment

Choose a reason for hiding this comment