Re-introduce likes on comments

[tclaus]

This is the continued work initially started by @Flaburgan with PR #7918

Current status: It works (mostly): Likes on comments, persist to database, added tests.

• Added like-icon on Post, Single post view and mobile view

[tclaus]

Found this API issues.
rspec ./spec/integration/api/likes_controller_spec.rb:126 # Api::V1::LikesController#create with right post id fails in liking already liked post
rspec ./spec/integration/api/likes_controller_spec.rb:115 # Api::V1::LikesController#create with right post id succeeds in liking post
rspec ./spec/integration/api/comments_controller_spec.rb:147 # Api::V1::CommentsController#read valid post ID retrieves related comments

Can someone help here?

[Flaburgan]

Hm yeah, the work on likes on comment is older than the work on the API. It means that:

• The API doesn't support liking comments so this has to be added documentation repo. Maybe @jhass can give some tips here
• It is possible that some changes made broke the backend, you need to check that. The federation part is supposed to be working (at least likes made on comments by friendica are relayed) but this has to be checked too.

[tclaus]

Rspec only gives me the API errors, cucumber works locally on my machine. So I am on a point where I appreciate some help.
Felling so close to a new feature.

[Flaburgan]

@tclaus the current test failing doesn't look related to the API, it's an undefined in JS, you can probably fix it I think. Once done, I will do a review.

[tclaus]

Thanks @Flaburgan - I am still on it, and I fixed some rspec errors locally, but there is still a schema issue left over.
The JS error won't occur If I run cucumber on my local machine. (And I've rebased to latest develop)

[denschub]

@tclaus, could you please run stylechecks locally on your machine before pushing a commit? The bot is meant as a safety-check if you miss something, not really as a tool to figure things out. :)

[tclaus]

All checks have passed ❤️

[Flaburgan]

So, thank you very much for your work @tclaus
I did a "User point of view" review: I launched your branch locally and had a look.
First thing I saw is that there is a line break between the icon and the "1 J'aime":

Note that this is not the case for post. So you're missing some CSS rules here.
Another thing, not sure it should be part of that PR but maybe: the users don't have any notifications when their comment is liked.
And last remark: those likes aren't displayed with the mobile interface, and it's not possible to like a comment there too. We have a long history of the mobile interface lagging behind in terms of features and @jhass is actually close to publishing a nice Android (and iOS) app, so you probably don't need to deal about that in this PR but I wanted to point it.
Apart from that, it looks like it works nicely, great work!

Also, I didn't test the federation part (as I only have my local test pod), but I could deploy that branch on diaspora-fr.org once you fix the CSS problem and I checked the code a bit.

[tclaus]

@Flaburgan
I fixed the CSS style, I also done some event handling issues. Looks better now.

The mobile interface is next.

For creating a notification a starting point for creating notifications would be nice - any tips here?
For federation: No clue what's this meant about comment-likes. As the likes is federated in the Model, it may already solved?
I would love to see this on a real website.

[Flaburgan]

Yes that's better, there still is a small problem with the avatar size:

[Flaburgan]

Also you have a commit about http -> https links in that branch, maybe you need to rebase on develop?

[tclaus]

Also you have a commit about http -> https links in that branch, maybe you need to rebase on develop?

No, that's dirt from another PR. Do you have an idea to remove these file from the commits? Do I have to reset everything and commit again?

On the weekend I will work on the other topics.

[Flaburgan]

Also you have a commit about http -> https links in that branch, maybe you need to rebase on develop?

No, that's dirt from another PR. Do you have an idea to remove these file from the commits? Do I have to reset everything and commit again?

On the weekend I will work on the other topics.

Rebase on the develop branch and squash your commits, it will be gone.

[github-actions]

Selector should have depth of applicability no greater than 3, but was 4

[Flaburgan]

So thank you @tclaus I was able to push to your branch.

@denschub I am glad you like it. In the future I guess we can apply the same styling to the desktop version.
I improved a bit the alignment (the real solution would be to use flexbox but I don't want to go through a full refactor) and I also fixed the border-radius at the bottom. Here is how it looks like now:

I also fixed most of the pronto errors. I guess it is ready for review.

Edit: Jasmine tests are green locally, both with Firefox and Chrome... I will investigate.

[SuperTux88]

OK, I reviewed now everything and fixed all the things I found myself, as @tclaus doesn't have time at the moment. I now just need somebody who reviews my commits (the ones I just added, and the ones from a year ago)

Edit: Jasmine tests are green locally, both with Firefox and Chrome... I will investigate.

@Flaburgan you broke that comments were lazy-loaded in the stream, which the test was testing for. And you didn't regenerate fixtures locally, so jasmine was still using the old fixtures where the comments were still lazy-loaded. I also fixed that so comments are laxy-loaded in the stream again.

[SuperTux88]

Approving the commits and changes not made by myself ;)

[denschub]

This PR still has five Pronto Ruby violations:

• app/controllers/likes_controller.rb:31 W: Metrics/AbcSize: Assignment Branch Condition size for create is too high. [<2, 22, 3> 22.29/20]
• app/controllers/notifications_controller.rb:11 W: Metrics/AbcSize: Assignment Branch Condition size for index is too high. [<18, 51, 10> 55/20]
• app/controllers/notifications_controller.rb:11 W: Metrics/MethodLength: Method has too many lines. [29/20]
• app/controllers/notifications_controller.rb:70 W: Metrics/AbcSize: Assignment Branch Condition size for read_all is too high. [<4, 32, 4> 32.5/20]
• app/helpers/notifications_helper.rb:19 W: Metrics/AbcSize: Assignment Branch Condition size for object_link is too high. [<3, 25, 7> 26.13/20]

as well as one SCSS violation:

• app/assets/stylesheets/mobile/comments.scss:131 W: Selector should have depth of applicability no greater than 3, but was 4

If we don't want to fix them, maybe we should add disable comments for those? Although I'm also not opposed to just ignoring them, but I felt like this is worth highlighting.

Either way, I have one question, but I'm approving anyway as I don't see this as blocking.

[SuperTux88]

If we don't want to fix them, maybe we should add disable comments for those?

I didn't want to add disable-comments for AbcSize and MethodLength, as it's not that "I don't want to fix them", more a "I don't want to fix them NOW". Because maybe these methods should be refactored (so I didn't want to just disable the warning now) or maybe we want to raise the limits globally for these rules if we are fine with that complexity?

As these violations were already there before, they just triggered now because I moved some minor things around, but otherwise didn't really touch these methods. And I also didn't want to refactor these methods in the scope of this PR, as it's already complex enough. So I just wanted to ignore them for now in this PR.

I'm not sure about the violation in the CSS, if we should change that or put a comment there or just also ignore it 🤷‍♂️

[denschub]

Let's ignore them, then. I'm fine with that, just wanted to raise that because I noticed. :)