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

FEATURE: Implement SiteSetting to Allow Anonymous Likes #22131

Merged
merged 5 commits into from Jul 21, 2023

Conversation

meltingmettle
Copy link
Contributor

@meltingmettle meltingmettle commented Jun 15, 2023

This pull request adds a SiteSetting to allow admins to enable or disable anonymous liking.

Changes:

  • Add allow_anonymous_likes SiteSetting
  • Tweak is_my_own? to recognize "like" post actions for anonymous users, since this behavior is required in order to allow anonymous users to remove their likes.
  • Update post_serializer to remove the :can_act scope from anonymous users after post_undo_action_window_mins has closed.

@CLAassistant
Copy link

CLAassistant commented Jun 15, 2023

CLA assistant check
All committers have signed the CLA.

@meltingmettle meltingmettle force-pushed the feature/allow-anonymous-likes branch 3 times, most recently from 9487b80 to c977fea Compare June 15, 2023 16:04
Copy link

@kevinchen234 kevinchen234 left a comment

Choose a reason for hiding this comment

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

Added a few spec comments but 👍

@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/pull-request-sitesetting-to-anonymous-users-to-like-posts/268660/1

pmusaraj
pmusaraj previously approved these changes Jun 22, 2023
Copy link
Contributor

@pmusaraj pmusaraj left a comment

Choose a reason for hiding this comment

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

Thanks @meltingmettle, the PR looks good. I am curious, what does the like popup look like in this scenario? I would expect it to be a list of anonymized user avatars. Is that correct?

Approving, will look into merging it shortly.

@nattsw nattsw dismissed pmusaraj’s stale review June 23, 2023 04:01

Translations missing

Copy link
Contributor

@nattsw nattsw left a comment

Choose a reason for hiding this comment

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

I think there are some UX issues here:

We still need to comply to post_undo_action_window_mins, which isn't happening. Ideally after X mins, the anonymous user should see this.

Screenshot 2023-06-23 at 2 08 21 PM

The current bug now is that when the anon user clicks on the red ❤️ to undo the like, the frontend shows that the "like" is removed, but when you refresh the page it is actually still liked.

config/site_settings.yml Show resolved Hide resolved
return(
SiteSetting.allow_anonymous_likes? && obj.class == PostAction && obj.is_like? &&
obj.user_id == @user.id
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

syntax_tree really wanted this format. :/

Originally this looked like:
return SiteSetting.allow_anonymous_likes? && obj.class == PostAction && obj.is_like? && obj.user_id == @user.id if anonymous?

@meltingmettle
Copy link
Contributor Author

Friendly bump! Thank you! @nattsw @pmusaraj

@nattsw
Copy link
Contributor

nattsw commented Jul 12, 2023

Hey there @meltingmettle, the issue hasn't been addressed. This is what I am seeing now with the newest commit.

Screen.Recording.2023-07-13.at.12.40.23.AM.mov

This is what needs to be shown:

Screen.Recording.2023-07-13.at.12.42.59.AM.mov

We need to abide by post_undo_action_window_mins. This disallows the user from unliking the post after the value in the site setting.

You can follow these steps:

  • Anonymous user likes a post.
  • Wait post_undo_action_window_mins (default is 10, but you can change to 1 locally)
  • Ensure that second video recording happens - user is disallowed from unliking.

@meltingmettle
Copy link
Contributor Author

meltingmettle commented Jul 13, 2023

Hi @nattsw, thank you for the second review. I'm having a bit of trouble figuring out where the disallow is supposed to happen sincepost_undo_action_window_mins is only referenced in can_delete_post_action?, and the method is correctly returning false once the post_undo_action_window_mins has passed.

However, despite this method returning false correctly after the window has closed, the disallow symbol does not appear, but there is no other method that references post_undo_action_window_mins. I realize that can_delete_post_action? is called when the page is loaded, but I can't quite locate where the actual disallow icon is invoked.

Could you please point me towards the block of code that calls the disallow symbol? Thank you!

@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/pull-request-sitesetting-to-anonymous-users-to-like-posts/268660/11

@nattsw
Copy link
Contributor

nattsw commented Jul 17, 2023

@meltingmettle

I traced the attribute from the css class:

Screenshot 2023-07-17 at 10 45 09 PM

// If the user has already liked the post and doesn't have permission
// to undo that operation, then indicate via the title that they've liked it
// and disable the button. Otherwise, set the title even if the user
// is anonymous (meaning they don't currently have permission to like);
// this is important for accessibility.
if (attrs.liked && !attrs.canToggleLike) {
button.title = "post.controls.has_liked";
} else {
button.title = attrs.liked
? "post.controls.undo_like"
: "post.controls.like";
}
if (currentUser && !attrs.canToggleLike) {
button.disabled = true;
}
return button;

then

postAtts.canToggleLike = likeAction.get("canToggle");

then

then, I suspect the problem is with this line:

if scope.post_can_act?(
object,
sym,
opts: {
taken_actions: actions,
},
can_see_post: can_see_post,
)
summary[:can_act] = true
end

In the case where the user is not anon, the "can_act" value is not present. Conversely, when the user is anon, "can_act" is present. There's probably an issue here with the scope.

Can you take a stab at figuring this one?

@meltingmettle
Copy link
Contributor Author

Thank you for the tip! That was super helpful! I’ve pushed a patch, manually verified the behavior, and added additional test coverage.

It’s ready for another round of review when you have the chance. 😄

Copy link
Contributor

@nattsw nattsw left a comment

Choose a reason for hiding this comment

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

Woohoo!

🚀

Will merge this once tests are passing

EDIT: Merged main due to failing spec due to webdriver update.

@nattsw nattsw merged commit 978d528 into discourse:main Jul 21, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants