framework icon indicating copy to clipboard operation
framework copied to clipboard

[1.x] [likes] fix: Ignore isLiked when post is not comment post.

Open zxy19 opened this issue 1 year ago • 3 comments

Changes proposed in this pull request:

When someone set isLiked attribute on a post that's not a comment (e.g. type discussionTagged) through api, it will be saved and send the notification to user. Then in PostLikedNotification component, the method excerpt is trying to get the plainContent to truncate, which will lead to a frontend error.

According to addLikeAction.js, user should only be able to like a comment post, so it should be ignore if target post is not a comment post.

Necessity

  • [ ] Has the problem that is being solved here been clearly explained?
  • [ ] If applicable, have various options for solving this problem been considered?
  • [ ] For core PRs, does this need to be in core, or could it be in an extension?
  • [ ] Are we willing to maintain this for years / potentially forever?

Confirmed

  • [ ] Frontend changes: tested on a local Flarum installation.
  • [ ] Backend changes: tests are green (run composer test).
  • [ ] Core developer confirmed locally this works as intended.
  • [ ] Tests have been added, or are not appropriate here.

zxy19 avatar Oct 17 '24 17:10 zxy19

In that case, you'll be better off detaching all the likes from the posts. Since the like and unlike are both forbidden now, there's no way to achieve it except by modifying the database.

YUCLing avatar Oct 17 '24 17:10 YUCLing

In that case, you'll be better off detaching all the likes from the posts. Since the like and unlike are both forbidden now, there's no way to achieve it except by modifying the database.

You are right. A new migration file was add to remove those records.

zxy19 avatar Oct 17 '24 18:10 zxy19

Hang on... I think this fix will reduce the extensibility of the Likes extension. Another way of fixing this is preferred. 🤔

YUCLing avatar Oct 17 '24 23:10 YUCLing

So the issue here is:

Liking any other Post type than CommentPost is causing issues with notifications.

The PR #4095 relates to this exact issue as well. Reference: https://github.com/flarum/framework/pull/4095#discussion_r1839725082

What I think should happen is create an extensible layer by adding a property to the implementation of every Post type, eg CommentPost that indicates whether notifications should be sent.

I'm closing this PR and the other one and will create an issue to explain further.

PS deleting all previous likes to start with a clean slate will not be appreciated by our users.

luceos avatar Nov 13 '24 08:11 luceos