[WIP] Fix 404 on removing a comment

[ionphractal]

At the moment, the API returns 404 when deleting a comment. This prevents implementation of Human-Connection/WebApp#347 .

It turns out the issue lies in the before all hook softDelete, which seems to be somewhat incompatible or rather order sensitive with the restrictToOwner hook.

feathersjs-ecosystem/feathers-hooks-common#185

I don't have enough experience with feathersjs, but it seems this error can be mitigated by changing the hook order when restrictToOwner is used, without sacrificing functionality.

What are your thoughts on that? @roschaefer@appinteractive

[appinteractive]

Think this will break as the soft delete hook must stay inside the all hook part to my knowledge.

[ionphractal]

I only found that it had to be inside before and is recommended in all (https://feathers-plus.github.io/v1/feathers-hooks-common/#softdelete), but doesn't have to be.
Also, in the meantioned feathers-hooks-common issue from my first comment, it is written in the other methods and praised as a workaround.
If I remember correctly I removed the hook completely for testing and it correctly deleted the comment without leaving any trace behind. You also didn't see the "this comment was removed" notice anymore.

But like this it still shows the notice so I suggest it works correctly.
Have you tested it yourself?

[appinteractive]

Okay thanks for double checking that! 🙂

[Gerald1614]

Hi, i just tested the fix 247 with this fix and i confirm it is doign what is expected to do. when i delete a comment the backend returns no error message when comment is deleted and my promise is solved.
I will update my own PR on fix 247. and as soon as this PR will be merged as well as mine we will be able to close those two issues. thanks a lot

[roschaefer]

@appinteractive is it safe to merge?

[appinteractive]

No it’s not as the moderator would delete the comment permanently without leaving a trace. That’s not intended as far as I can tell. Or do I not get it wrong?

[appinteractive]

Also somehow the build is falling.

[ionphractal]

@appinteractive
The build is failing because there are missing credentials for some notification. Don't know the details but this is a configuration issue of the pipeline.

0.32s$ cat ./coverage/lcov.info | codacy-coverage
[error] "2018-11-22T14:58:48.359Z" 'Error sending coverage'
[error] "2018-11-22T14:58:48.361Z" Error: Token is required
at Object.module.exports [as handleInput] (/home/travis/.config/yarn/global/node_modules/codacy-coverage/lib/handleInput.js:22:35)
at Socket.<anonymous> (/home/travis/.config/yarn/global/node_modules/codacy-coverage/bin/codacy-coverage.js:48:20)
at Socket.emit (events.js:187:15)
at endReadableNT (_stream_readable.js:1094:12)
at process._tickCallback (internal/process/next_tick.js:63:19)

I did not say the moderator can erase it without a trace... (I said: if the softDelete hook was omitted completely, then whoever deleted the comment, would delete it completely from MongoDB without leaving a trace.

Btw. I don't know what is intended, but the change should behave almost the same as without, because the softDelete hook is executed before all of the actions. With the exception that the order of execution is changed for the remove action (which basically prevents Feathers to call delete on something it doesn't know after it already soft-deleted the entry).

Could you please elaborate on what the expected behavior is (your specs) and I will see what I can do about it.

[appinteractive]

If I correctly understand, your change only soft deletes wheh a user deletes it in the Frontend, but not when it’s triggered on the server or by an admin or moderator.

That’s the confusing part for me. I ask me why you did it exactly that way. Before it was done without that restriction if I read it correctly. That’s one of the problems with the feathers hooks, they get very complex (mostly hidden) very fast and you get that Bad feeling.

The codacy part, it will be removed tomorrow completely anyway. It stopped working as they changed something on their side.

[ionphractal]

Ah could it be that you mean because I added softDelete to the inside of the unless condition?
Now that you say it, and if I understand the documentation correctly, it won't be executed for example when isProvider returns true... Maybe unless is unsuitable for this case then.
The additional challenge is to know which hooks do a probing get on their own (which conflicts with softDelete in terms of order) but for now I would just want to fix restrictToOwner.

Anyway I'll do some research on this and come back then. Thanks for the hint!

[ionphractal]

So, to retain exact behavior as before, with the exception of the single case when restrictToOwner is called, I used the following pseudo code to understand what feathers does.

This is the way it was before any modification (in order how the hooks were executed):

before all requests do
exec softDelete()
exec xss({ fields: xssFields })
before remove requests do
exec authenticate('jwt')
unless isProvider('server') is true do
unless isModerator() is true do
exec isVerified()
exec restrictToOwner()
exec softDelete()

Because unless conditions are quite a head twister (for me at least), I rewrote it to using if conditions:

before all requests do
exec softDelete()
xss({ fields: xssFields })
before remove requests do
exec authenticate('jwt')
if isProvider('server') is false do
if isModerator() is false do
exec isVerified()
exec restrictToOwner()
exec softDelete()

To fix the 404 behavior, it would have to be:

before all requests do
exec xss({ fields: xssFields })
before remove requests do
if isProvider('server') is true then
exec softDelete()
exec authenticate('jwt')
else if isProvider('server') is false then
if isModerator() is true
exec softDelete()
exec authenticate('jwt')
else if isModerator() is false then
exec authenticate('jwt')
exec isVerified()
exec restrictToOwner()
exec softDelete()

With feathers hooks syntax, that seems to be (omitting before all here):

 remove: [
iff(isProvider('server'),
softDelete(),
authenticate('jwt')
).else( // isProvider == false
iff(isModerator(),
softDelete(),
authenticate('jwt')
).else( // isModerator == false
authenticate('jwt'),
isVerified(),
restrictToOwner(),
softDelete()
)
)
]

Does that make sense?

Btw. when I tested, I notice that I don't see a delete button in neither the moderator nor the administrator view for comments. But that is a WebApp "issue" (:isOwner="comment.userId === user._id"). So the additional conditions are not even implemented so far.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (develop@4437261). Click here to learn what that means.
The diff coverage is n/a.

@@ Coverage Diff @@## develop #182 +/- ##
==========================================
Coverage ? 66.64% ==========================================
Files ? 146 Lines ? 1925 Branches ? 0 ==========================================
Hits ? 1283 Misses ? 642 Partials ? 0

Impacted Files	Coverage Δ
server/services/comments/comments.hooks.js	91.3% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4437261...236fe42. Read the comment docs.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (develop@4437261). Click here to learn what that means.
The diff coverage is n/a.

@@ Coverage Diff @@## develop #182 +/- ##
==========================================
Coverage ? 66.64% ==========================================
Files ? 146 Lines ? 1925 Branches ? 0 ==========================================
Hits ? 1283 Misses ? 642 Partials ? 0

Impacted Files	Coverage Δ
server/services/comments/comments.hooks.js	91.3% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4437261...236fe42. Read the comment docs.