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

Gerald1614 · 2018-10-29T19:00:56Z

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

Quickfix

roschaefer

@Gerald1614 thank you so much for your contribution!

I haven't tested this code but I think it is very strange that there is no need for editorText. I thought, without it, you cannot enter text input anymore.

In the meantime, until sb. gives it a check, you can already improve these two things here.

Also: You can improve the description of your PR if you markup your code listing with triple backticks `

store/comments.js

components/Comments/Comment.vue

roschaefer · 2018-10-30T15:19:39Z

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 · 2018-10-31T11:05:01Z

ok I will do it later thanks

…ithub.com/Gerald1614/WebApp into no-refresh-after-comment-update-or-delete

Gerald1614 · 2018-11-06T01:15:55Z

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.

recheck

Gerald1614 · 2018-11-08T14:39:08Z

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

Lulalaby

lets try

Lulalaby · 2018-11-09T20:24:43Z

@Gerald1614 can i remove WIP?

Gerald1614 · 2018-11-10T23:24:42Z

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

roschaefer

@Gerald1614 thank you so much for making a stab at issue #247. If your PR is related to an issue, you can add fix #247 to the description of the pull request. It will automagically close the respective issue when this PR is merged, see Github's autoclosing feature.

Also, it would be great to add software-tests to your PR. I will try to scaffold them for you.

roschaefer · 2018-11-12T18:14:35Z

store/comments.js

@@ -111,10 +118,14 @@ export const actions = {
  create ({dispatch}, data) {
    return this.app.$api.service('comments').create(data)
  },
-  patch ({dispatch}, data) {
+  patch ({dispatch, commit}, data) {


remove obsolete argument dispatch if possible

roschaefer · 2018-11-12T18:15:12Z

store/comments.js

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


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

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.

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.

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 ?

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

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.

@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

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

roschaefer · 2018-11-12T18:19:56Z

components/Comments/Comment.vue

@@ -170,9 +169,7 @@
        remove: 'comments/remove',
        patch: 'comments/patch'
      }),
-      editorText (newText) {


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.

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

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

roschaefer · 2018-11-12T18:22:00Z

.env.example

@@ -1,18 +0,0 @@
-# WEBAPP


Why was .env.example removed?

my mistake, in the documentation (https://docs.human-connection.org/documentation/web-app/installation) it mentions .env file so when i first installed the product i changed .env.example by .env

roschaefer · 2018-11-12T18:35:22Z

yarn.lock

@@ -5,10 +5,12 @@
 "@ava/babel-plugin-throws-helper@^2.0.0":
  version "2.0.0"
  resolved "https://registry.yarnpkg.com/@ava/babel-plugin-throws-helper/-/babel-plugin-throws-helper-2.0.0.tgz#2fc1fe3c211a71071a4eca7b8f7af5842cd1ae7c"
+  integrity sha1-L8H+PCEacQcaTsp7j3r1hCzRrnw=


@Gerald1614 your yarn version is between v1.10.0 and v1.12.0 see yarn's release notes here. 😆 try to avoid unrelated changes in your PR. You can e.g. git checkout origin/master -- yarn.lock.

If you want, you can create a new PR called "Add integrity-sha to yarn.lock". It doesn't seem to hurt.

that was my adding with the integrity. i resolved it via the jnternal github resolver on this page. so there was no commit msg to ad

store/comments.js

@Gerald1614

@Gerald1614 please fix

roschaefer · 2018-11-12T21:23:36Z

@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 · 2018-11-12T21:27:35Z

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

roschaefer · 2018-11-12T21:34:04Z

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.

redo

Gerald1614 · 2018-11-16T12:56:59Z

@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 · 2018-11-16T18:35:57Z

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

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

roschaefer · 2018-11-22T19:47:40Z

@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 · 2018-11-22T19:55:15Z

@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 · 2018-11-22T21:49:41Z

@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 · 2018-11-23T10:51:52Z

@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: Human-Connection/API#182

Gerald1614 · 2018-11-24T14:51:28Z

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 (#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.

roschaefer and others added 2 commits October 23, 2018 23:15

Merge pull request Human-Connection#336 from Human-Connection/develop

fa26d7c

Quickfix

comment patch and delete addressed

20fe531

roschaefer previously requested changes Oct 30, 2018

View reviewed changes

store/comments.js Outdated Show resolved Hide resolved

components/Comments/Comment.vue Outdated Show resolved Hide resolved

Gerald Michelant added 4 commits November 4, 2018 15:48

comment patch and delete addressed

7f97162

fix#247 - delete and update comments

73dd331

Merge branch 'no-refresh-after-comment-update-or-delete' of https://g…

6e511bf

…ithub.com/Gerald1614/WebApp into no-refresh-after-comment-update-or-delete

added mutation for comment update

e880a74

Lulalaby requested a review from roschaefer November 6, 2018 08:08

Lulalaby added the bug label Nov 6, 2018

upodated package.json to match sanitize-html version

beb7404

Merge branch 'develop' into no-refresh-after-comment-update-or-delete

ad9389c

Lulalaby self-requested a review November 9, 2018 20:21

Lulalaby previously approved these changes Nov 9, 2018

View reviewed changes

roschaefer requested changes Nov 12, 2018

View reviewed changes

roschaefer reviewed Nov 12, 2018

View reviewed changes

store/comments.js Outdated Show resolved Hide resolved

roschaefer added 2 commits November 12, 2018 22:15

Expose several bugs in the current implementation

749c730

@Gerald1614 please fix

Use async/await style

7c0c5ff

ionphractal mentioned this pull request Nov 22, 2018

[WIP] Fix 404 on removing a comment Human-Connection/API#182

Open

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

Are you sure you want to change the base?

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

Conversation

Gerald1614 commented Oct 29, 2018

roschaefer left a comment

Choose a reason for hiding this comment

roschaefer commented Oct 30, 2018

Gerald1614 commented Oct 31, 2018

Gerald1614 commented Nov 6, 2018

Gerald1614 commented Nov 8, 2018

Lulalaby left a comment

Choose a reason for hiding this comment

Lulalaby commented Nov 9, 2018

Gerald1614 commented Nov 10, 2018

roschaefer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roschaefer commented Nov 12, 2018

roschaefer commented Nov 12, 2018

roschaefer commented Nov 12, 2018 • edited Loading

Gerald1614 commented Nov 16, 2018

roschaefer commented Nov 16, 2018

roschaefer commented Nov 22, 2018

Gerald1614 commented Nov 22, 2018

Gerald1614 commented Nov 22, 2018

API: find categories

API: get users

API: get users

API: find settings

API: find status

ionphractal commented Nov 23, 2018

Gerald1614 commented Nov 24, 2018

roschaefer left a comment •

edited

Loading

roschaefer commented Nov 12, 2018 •

edited

Loading