[WIP] fix #247 comment patch and delete addressed

[Gerald1614]

I removed v-on:input=“editorText” from commentForm.vue and comment.vue as I do not think they are useful. I just used an existing action to trigger a mutation after comment update to get the field being updated just after the submission of the updated comment. then i discovered that the same process happened when the comment update was cancelled. i then added a .prevent on the cancel button @click.prevent=“cancelEdit"

for delete I added a new mutation called removeComment

removeComment (state, id) {
const cmt = state.comments[state.comments.findIndex(comment => comment._id === id)]
console.log(cmt)
cmt.deleted = true
},

which is called by this action

remove ({commit}, id) {
commit('removeComment', id)
return this.app.$api.service('comments').remove(id)
// .then(() => {
// commit('removeComment', id)
// })
}

definitive code should be remove``` ({commit}, id) { return this.app.$api.service('comments').remove(id) .then(() => { commit('removeComment', id) }) }

but the API was sending me an error message ('item not found', which is weird as I did not touch the code to call the API) that prevented the mutation to happen. 
It would be greta if someone could test the code with the .then so mutation is done only if the change happened in the backend.

thanks for your feedback
Gerald

[roschaefer]

Some more things:

• If possible, rebase your feature branch like this: git remote add origin [email protected]:Human-Connection/WebApp.git git fetch origin git rebase origin/master
• Add fix NUMBER_OF_ISSUE to your description of the PR. That will link your PR with the issue you are trying to fix.
• Edit the PR title in a way, that would help a non-developer to understand the problem and the fix. We will use the PR titles for a changelog

[Gerald1614]

ok I will do it later thanks

[Gerald1614]

HI @roschaefer, I did commit a new version that removes all comments. I also created a mutation for the update so the code is clean for both delete and update of comment. i do not know why there is a conflict with yarn.lock. i thought those kind of files would be ignored. i dod not touched it. maybe you can just igmore the changes on this file and try the rest.

[Gerald1614]

i just made another commit with an updated version of sanitize-html and now the only discripency is the integrity check from yarn.

[Lulalaby]

@Gerald1614 can i remove WIP?

[Gerald1614]

for me the code is working so yes, you can remove the WIP

[roschaefer]

@Gerald1614 I worked on your branch and added a couple of tests that expose bugs in the current implementation. You can run those tests with yarn run test.

I hope these tests give you some guidelines for testing. The vue-test-utils api docs are a great read!

Please contact @ionphractal for the changes related to the confirmation dialog.

[roschaefer]

Great! The build server has failed. That's what we want.

[roschaefer]

Ah, one more thing. Usually, we should use https://feathers-plus.github.io/v1/feathers-vuex/ instead of re-implementing this behaviour. We already use it for another pull request. I don't ask you @Gerald1614 to refactor the store/comments.js with feathers-vuex, but I just want to make you aware of that library.

Human connection is transitioning to GraphQL+Express+Neo4j instead of REST+FeathersJS+MongoDB and we will replace FeathersJS, so it's not worth to spend time on feathers-vuex anymore.

[Gerald1614]

@roschaefer , I just looked quickly at your commit with async await utilisation. You are using a const that is then not used and it makes the test failed. if we look at the vuex doc here is how async await needs to be used differnetly actions: { async actionA ({ commit }) { commit('gotData', await getData()) }, async actionB ({ dispatch, commit }) { await dispatch('actionA') // wait for actionA to finish commit('gotOtherData', await getOtherData()) } } the other issue we are facing is that the API removes the comment but returns a bad request message that prevents the promise to then get resolved. I know that becasue initially i tried to implement the following code that never got executed
remove ({commit}, id) { return this.app.$api.service('comments').remove(id) .then(()=> { commit('removeComment', id) } } I am not familiar with the APi but it would be good somebody would check the code i added is not affected the request so the bug was there before. I agree with you we should not commit the transaction before knowing the API did execute the removal.

[roschaefer]

@Gerald1614 could you please format the inline code with backticks? That makes it much easier to read. See Github help.

I know that I added constants that are not used. I wanted to give you a hint, how to make the tests pass :wink:

If the backend always responds with bad message upon removal, that is a serious bug and is worth a bug report.

[roschaefer]

@Gerald1614 I mentioned you in our video blog! https://youtu.be/LiD3BmH09z8

What's the status on this PR? The build server crashed, I see

The command "eval git clone --depth=50 --branch=no-refresh-after-comment-update-or-delete https://github.com/Human-Connection/WebApp.git Human-Connection/WebApp " failed. Retrying, 2 of 3.

What the heck? https://travis-ci.org/Human-Connection/WebApp/jobs/454177158#L423

[Gerald1614]

@roschaefer since a few days, the app is crashing and I do not know why as i did nto touch anything. it looks like it is going back and forward with the backend. I also discuss with @ionphractal and i need anyway to bring back the code used for dialog box in case people are leaving the page. i am thinking about creating a new branch from scratch and reduce modifications to address the refresh aspects. the PR can not anyway be used as the promise returned by the backend sends a 404 message so i can not chain with a mutation. it is being addressed.

[Gerald1614]

@roschaefer I just cloned the repository and when i run it I am facing the same issue as i had with my code. i do not =know if somebody else made same changes recently but while the app is workign fine, i receive tons of logs that repeat eternally ### refreshJWT auth/init true false

API: find categories

data undefined

API: get users

data undefined

API: get users

data undefined

API: find settings

data undefined

API: find status

data undefined I thought I may have done something but it is not the case, i am using current API and current WebApp and i have same message as on my fork.

[ionphractal]

@Gerald1614 When I checked out your repo/branch, the app didn't crash on me. What's the error message? Maybe try deleting node_modules and executing yarn again. Regarding the messages you posted in your last comment: that's "normal" for me to get.

Yes, things got quite messy, which is why they started developing the app from scratch, see 'nitro' repos.

Regarding the 404, I opened a PR yesterday: https://github.com/Human-Connection/API/pull/182

[Gerald1614]

Hi @ionphractal , @roschaefer, because of those messages I thought there was something corrupted so i restarted all my repository from scratch to provide you a clean code, instead of creating a new branch i just deleted all i did in the past. i just made a new PR (https://github.com/Human-Connection/WebApp/pull/361) that addresses this issue. Please proceed with the other PR instead of this one. thanks to the PR made on teh backend, the frontend behaves now as expected which mean I receive a success message from the API which allows me to solve my promise when I delete a comment and the comment is deleted without having to refresh.