Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

zxy19
Copy link

@zxy19 zxy19 commented Oct 17, 2024

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 zxy19 requested a review from a team as a code owner October 17, 2024 17:02
@YUCLing
Copy link
Contributor

YUCLing commented Oct 17, 2024

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.

@zxy19
Copy link
Author

zxy19 commented Oct 17, 2024

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.

@YUCLing
Copy link
Contributor

YUCLing commented Oct 17, 2024

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

->whereNotExists(function ($query) {
$query->selectRaw(1)->from('posts')->whereColumn('id', 'post_id')->where('type', 'comment');
})
->delete();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it no need to delete?

just keep it in database for rolling back.

@imorland imorland changed the title fix: Ignore isLiked when post is not comment post. [1.x] [likes] fix: Ignore isLiked when post is not comment post. Oct 21, 2024
@luceos
Copy link
Member

luceos commented Nov 13, 2024

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: #4095 (comment)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants