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.

Sign up for GitHub

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

Jump to bottom

[WIP] fix #247 comment patch and delete addressed #347

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

[WIP] fix #247 comment patch and delete addressed #347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fa26d7c
Merge pull request #336 from Human-Connection/develop
roschaefer Oct 23, 2018
20fe531
comment patch and delete addressed
Oct 29, 2018
7f97162
comment patch and delete addressed
Oct 29, 2018
73dd331
fix#247 - delete and update comments
Nov 4, 2018
6e511bf
Merge branch 'no-refresh-after-comment-update-or-delete' of https://g…
Nov 4, 2018
e880a74
added mutation for comment update
Nov 4, 2018
beb7404
upodated package.json to match sanitize-html version
Nov 8, 2018
ad9389c
Merge branch 'develop' into no-refresh-after-comment-update-or-delete
Lulalaby Nov 9, 2018
749c730
Expose several bugs in the current implementation
roschaefer Nov 12, 2018
7c0c5ff
Use async/await style
roschaefer Nov 12, 2018
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
18 changes: 0 additions & 18 deletions .env.example
View file
Open in desktop

This file was deleted.

7 changes: 2 additions & 5 deletions components/Comments/Comment.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,14 @@
<form class="comment-form" @submit.prevent="patchComment" v-else>
<hc-editor identifier="comment"
editorClass="autowrap"
v-on:input="editorText"
v-model="newContent"
:editorOptions="editorOptions" />
<div class="comment-form-actions">
<hc-button
class="button is-hidden-mobile"
color="light"
:disabled="isLoading"
@click="cancelEdit">
@click.prevent="cancelEdit">
{{ $t('button.cancel') }}
</hc-button>
<hc-button
Expand Down Expand Up @@ -170,9 +169,7 @@
remove: 'comments/remove',
patch: 'comments/patch'
}),
editorText (newText) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know, the editorText method was used to show the confirmation dialog, when a user accidently or intentionally clicked a button to leave the current page. So your latest changes would not be lost. @ionphractal could you confirm?

Actually I would love to see you @Gerald1614 pair-program with a community member. @Gerald1614 did you join our Discord Server already? You can organize a pair-programming with e.g. @ionphractal. He speaks perfect English as far as I know.

Copy link
Contributor

Choose a reason for hiding this comment

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

here's the invitation link to our public Discord Server: https://discord.gg/6ub73U3

Copy link
Author

Choose a reason for hiding this comment

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

Hi, I am on discord. i have a very busy week but i will use discord to contact @ionphractal. I will be happy to spend time with him. it will be easier than playign ping pong with chat messages. thanks, Gerald

this.$emit('input', newText)
},

removeComment () {
this.$dialog.confirm({
title: this.$t('component.contribution.commentDelete'),
Expand Down
4 changes: 0 additions & 4 deletions components/Comments/CommentForm.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
<form class="comment-form" @submit.prevent="submitComment">
<hc-editor identifier="comment"
ref="editor"
v-on:input="editorText"
editorClass="autowrap"
v-model="form.content"
:editorOptions="editorOptions" />
Expand Down Expand Up @@ -77,9 +76,6 @@
}
},
methods: {
editorText (newText) {
this.$emit('input', newText)
},
reply (comment) {
if (!comment) {
return
Expand Down
1 change: 0 additions & 1 deletion components/Comments/Comments.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
</div>
<transition-group v-else-if="comments.length >= 1" name="comment" tag="div">
<comment @reply="onReply"
v-on:input="editorText"
v-for="comment in comments"
:isAuthor="comment.userId === post.userId"
:isOwner="comment.userId === user._id"
Expand Down
19 changes: 14 additions & 5 deletions store/comments.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,14 @@ export const mutations = {
clear (state) {
state.comments = []
},

updateComment (state, data) {
state.comments[state.comments.findIndex(comment => comment._id === data._id)].contentExcerpt = data.content
Copy link
Contributor

Choose a reason for hiding this comment

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

why only contentExerpt and why contentExcerpt = data.content? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

because it is the field used to store the text of the comment. so here, once the comment has been updated on the backend, i update the state of the store through a mutation. in previous code, there was no mutation so the value of the state to which the computed property was bind did not trigger a change of value, this is why a refresh was required (all comments were loaded from the backend) with the mutation, the change of state will trigger a change in the computed property and vuejs will update teh UI accordingly. Another nice effect is to limit requests to the backend. if the store is updated when we update, delete or create a comment, there is no reason to send a request to the back end to access the data, we should trust the state of the store.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but why is contentExcerptoverwritten with 'content`? Is only content given? If you don't want to wait for the API response, feel free to change or remove the Tests I wrote yesterday.

Copy link
Author

Choose a reason for hiding this comment

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

usually, calls to APIs are done in actions. this is what we do here:
patch ({commit}, data) {
return this.app.$api.service('comments').patch(data._id, data)
.then(() => {
commit('updateComment', data)
})
},
I Do not know what the API is doing but I guess it takes all the data. but for my store, I am changing only ContentExcerpt because it is the only thing a user can modify.
I am waiting for the API to make the change before initiating the mutation. if the API answers with a promise I then call the mutation. No need to go back to the API at the mutation level. does it make sense ?

Copy link
Author

Choose a reason for hiding this comment

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

@roschaefer , now i understand why you asked why i was updating contentExcerpt only. i guess your ask is why i do not update Content object ? it is because the comment API is not returning content. when i look at the comment object, the API returns comment objects with content Excerpt but no content. and I guess this is why I receive an error message on my promise. on the API when you try to delete a comment in fact you allocate the comment.deleted object to true and change the comment.content = "DELETED" but this field does not exist. so the API sends back an error message to the front end, just because you try to patch a filed that does not exist. I think it is clearly a bug. I am not familiar enough with Feather to make further investigations but as you said i may create a bug so that somebody could look at this.

Copy link

Choose a reason for hiding this comment

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

For the record: the 'content' field exists on the backend side but is omitted (https://github.com/Human-Connection/API/blob/develop/server/services/comments/comments.hooks.js#L122) when using find(). That is why it is not in the store when the page is loaded as it uses the find method. I presume this was done to preserve bandwidth as by default only the excerpts of the comments are shown. The content field is then loaded when the comment is expanded or edited (https://github.com/Human-Connection/WebApp/blob/develop/components/Comments/Comment.vue#L187-L221). In that regard, I think you need to fetch the comment with get() before you do any mutation on it.

Copy link
Author

Choose a reason for hiding this comment

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

@ionphractal not sure doing two fetch reduce the bandwidth. I do not know if there are other reasons behind that but I saw that at several places. so in fact we do a lot of back and forth with the backend as we fetch data a first time, do another one to fetch the comment and then a request to delete the comment. if we could get the full object to the backend it could perform the update of the comment.content and return a 200 message so i can proceed to the mutation of the state. we save a fetch

Copy link
Author

Choose a reason for hiding this comment

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

@ionphractal, in fact, returning the content is just one way we could leverage more the store instead of fetching data agin, but to solve my fix on deletion. can you check why the deletion of comment call is not returning a 200 message? it is is the only thing that is missing to close this issue. my understanding is that we are moving the backend so no need to chnage no the way of dealign with the backend bu tthis 404 message is preventing the promise to be solved.

},
removeComment (state, id) {
const cmt = state.comments[state.comments.findIndex(comment => comment._id === id)]
cmt.deleted = true
},
setContributionId (state, contributionId) {
state.contributionId = contributionId
}
Expand Down Expand Up @@ -75,7 +83,6 @@ export const actions = {
return
}
commit('setContributionId', contributionId)

return this.app.$api.service('comments').find({
query: {
contributionId: contributionId,
Expand Down Expand Up @@ -111,10 +118,12 @@ export const actions = {
create ({dispatch}, data) {
return this.app.$api.service('comments').create(data)
},
patch ({dispatch}, data) {
return this.app.$api.service('comments').patch(data._id, data)
async patch ({dispatch, commit}, data) {
const result = await this.app.$api.service('comments').patch(data._id, data)
commit('updateComment', data)
},
remove ({dispatch}, id) {
return this.app.$api.service('comments').remove(id)
async remove ({commit}, id) {
commit('removeComment', id)
const result = await this.app.$api.service('comments').remove(id)
}
}
102 changes: 102 additions & 0 deletions store/comments.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,22 @@ let state
let dispatch
let commit


let someComment = {
_id: '5be9d1d23f0988002f4bea67',
content: "<p>I'm content</p>",
contributionId: '5be988fa50822c002f206580',
contentExcerpt: "<p>I'm content</p>",
userId: '5be988d550822c002f206521',
hasMore: false,
createdAt: '2018-11-12T19:17:38.068Z',
updatedAt: '2018-11-12T19:17:38.068Z',
deleted: false,
upvoteCount: 0,
upvotes: [],
__v: 0
}

beforeEach(() => {
dispatch = jest.fn(() => Promise.resolve())
commit = jest.fn()
Expand All @@ -21,6 +37,53 @@ describe('isLoading', () => {
})
})

describe('updateComment', () => {
let updatedComment = Object.assign({}, someComment, {
contentExcerpt: "<p>I'm updated</p>",
updatedAt: '2018-11-12T19:17:47.785Z'
})

test('updates contentExcerpt', () => {
state = { comments: [someComment] }
mutations.updateComment(state, updatedComment)
expect(state.comments[0].contentExcerpt).toBe("<p>I'm updated</p>")
})

test('updates updatedAt', () => {
state = { comments: [someComment] }
mutations.updateComment(state, updatedComment)
expect(state.comments[0].updatedAt).toBe('2018-11-12T19:17:47.785Z')
})

test('does not crash when no ID is found', () => {
state = { comments: [someComment] }
updatedComment = Object.assign({}, someComment, { _id: 'notFound' })
mutations.updateComment(state, updatedComment)
expect(state.comments[0].deleted).toBe(true)
})
})

describe('removeComment', () => {
let removedComment = {
_id: '5be9d1d23f0988002f4bea67',
createdAt: '2018-11-12T19:17:38.068Z',
updatedAt: '2018-11-12T19:17:38.068Z',
deleted: true
}

test('does not crash when no ID is found', () => {
state = { comments: [someComment] }
mutations.removeComment(state, 'notfound')
expect(state.comments[0].deleted).toBe(true)
})

test('marks comment as deleted', () => {
state = { comments: [someComment] }
mutations.removeComment(state, '5be9d1d23f0988002f4bea67')
expect(state.comments[0].deleted).toBe(true)
})
})

describe('given a mock api', () => {
let action
let api
Expand All @@ -38,6 +101,45 @@ describe('given a mock api', () => {
})
})

describe("$api.service('comments').patch returns an updated comment", () => {
let updatedComment = Object.assign({}, someComment, {
contentExcerpt: "<p>I'm updated</p>",
updatedAt: '2018-11-12T19:17:47.785Z'
})

beforeEach(() => {
api.use('/comments', { async patch () { return updatedComment } })
const module = { app: { $api: api } }
action = actions.patch.bind(module)
})

describe('patch', () => {
it('commits the api response comment', async () => {
await action({state, dispatch, commit}, { _id: 'whatever' })
const expected = [ [ 'updateComment', updatedComment ] ]
expect(commit.mock.calls).toEqual(expected)
})
})
})

describe("$api.service('comments').remove returns an updated comment", () => {
let updatedComment = Object.assign({}, someComment, { deleted: true })

beforeEach(() => {
api.use('/comments', { async remove () { return updatedComment } })
const module = { app: { $api: api } }
action = actions.remove.bind(module)
})

describe('remove', async () => {
it('commits the api response comment', async () => {
await action({state, dispatch, commit}, { _id: 'whatever' })
const expected = [ [ 'removeComment', updatedComment ] ]
expect(commit.mock.calls).toEqual(expected)
})
})
})

describe('fetchByContributionId', () => {
beforeEach(() => {
const module = { app: { $api: api } }
Expand Down
Loading