Fix client-side duplicate comments bug on submit and load more

[ionphractal]

This PR fixes #321 .

The issue there is a race condition when the processing of the results too longer after loading additional comments from the API while calling fetchAllByContributionId.
It was either caused in https://github.com/Human-Connection/WebApp/blob/develop/store/comments.js#L54 or when loading additional comments via "Load more comments" button or when the page is loaded with a given comment ID.

You can simulate the race condition by changing https://github.com/Human-Connection/WebApp/blob/develop/store/comments.js#L90-L96 to

        setTimeout(() => {
          let newComments = orderBy(uniq(state.comments.concat(result.data)), ['createdAt'], ['asc'])
          commit('setCommentCount', result.total)
          commit('set', newComments)
          commit('isLoading', false)
        }, 1000)

The bug was introduced in #256 because I previously assumed find() returns a Promise and treated it like that in https://github.com/Human-Connection/WebApp/blob/develop/store/comments.js#L65-L66.

The function now returns a proper Promise.

@roschaefer @appinteractive @Lulalaby Would you please test?

[roschaefer]

I can confirm manually, this is fixing the bug!

[ionphractal]

@roschaefer Yesterday we discussed that you wanted to favor simplicity over complexity and asked to load the remaining comments all in one request (set "$limit" to "0"/"-1"). However, this does not work because default pagination settings are done in the API (https://github.com/Human-Connection/API/blob/develop/config/default.json#L9-L12).
Actual behavior:

• When you set $limit to "0", nothing is loaded but the total comment count (due to pagination being enabled, see the "Pro Tip" at the end of https://docs.feathersjs.com/api/databases/common.html#pagination).
• When you set $limit to "-1", only a single comment is loaded after the index of $skip.
• When you remove $limit completely, a default limit of "30" is assumed.

I also read the FeathersJS documentation and found a link to feathersjs/feathers#382 (comment) which also sais it is a bad practice to let the client load all data at once. So I would say, we leave loading mechanism as it is right now, just applying this fix. Then you can also write your regression test ;)

By the way, I've also found an issue with the _.uniq function not working as I expected which allowed the duplicates to be added to the store in the first place. It seems even though you load the same object from the API, the JS object is not identical. Therefore I explicitly compare the object _id now. You can easily test this by removing the $skip parameter from find(). That will certainly load the same data (from index 0 onward) again and again when you press "show more comments" or add/manipulate a comment. After I fixed it, there will be no more duplicates in the store.

@mimicc83 @appinteractive

[appinteractive]

@ionphractal you could set the limit to an insane amount, not pretty but would work.

But loading all is really not a good practice.

[appinteractive]

@ionphractal why do you use promise instead of async/await from es6?

[roschaefer]

@ionphractal and one more thing: When we go through the closed and recently updated pull requests later, we would like to tell by the titles what has changed 🙂

[ionphractal]

@appinteractive I think the "max" setting in the API config would then determine the limit. So you have to change it there, which is really not pretty :D
I use promises mainly because I'm not much familiar with the async/await syntax and also I don't know how to do error handling there. If you prefer async/await, the branch is open for maintainer changes :)

@roschaefer Ok.

[roschaefer]

After implementing tests I believe, the setTimeout introduces an unrelated bug that has similar effects. commits are synchronous and $api.service.find() does return a Promise. I am not sure anymore, if we might continue to see this error in production after this bugfix.

[roschaefer]

I think I'm done here. I was not able to expose the bug with a regression test. I still doubt that we really fixed this bug in production. I cannot help but say let's merge this and see if we get support requests.

In general, let's never recursively call async actions from now on, please.

[roschaefer]

See my comment, I'm unable to reproduce the bug with a test

[Lulalaby]

Seem's ok from my view