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

Hide website link for accounts under ten days old #3142

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

cmho
Copy link
Contributor

@cmho cmho commented Jun 24, 2023

CODE TOUR: To help discourage spammers from registering accounts here, we want to hide the website link on the profile to other users until the account is ten days old, and display a message telling the logged-in user why it's not displaying yet.

Closes #2795.

@kareila
Copy link
Member

kareila commented Jun 24, 2023

This is a good start, but it doesn't entirely meet the requested spec. Just at a glance:

  • it seems to show the "your profile" message to everyone who views the profile, not just the journal owner?
  • it should still show the actual link to logged-in users with suspend privileges.

@cmho
Copy link
Contributor Author

cmho commented Jun 24, 2023

@kareila I think this should hit all the requirements now?

@kareila
Copy link
Member

kareila commented Jul 28, 2023

Okay, deep breaths. This is like, 70% of the way there! You're doing great.

I am going to break this review into three segments.

my $spam_time_threshold = scalar (localtime() - (ONE_DAY * 10)) <= scalar LJ::mysql_time( $u->timecreate );

^^ This line gave me an error: Argument "2022-10-18 22:31:21" isn't numeric in localtime

I spent half an hour trying different variations on this to get the right output, so you don't have to. Here's what I ended up with:

my $spam_time_threshold = (DateTime->now->epoch - (ONE_DAY * 10)) > $u->timecreate;

Notice I reversed the direction of the comparison, because the link will display normally if the expression is true.

if ($remote->has_priv('suspend') || $spam_time_threshold) {

If the page is being viewed logged out, this will fail because $remote will be unset. Also, you've already calculated $spam_time_threshold, and we expect it to usually be true, so it makes sense to check that first. So here's the way I recommend rewriting that:

if ( $spam_time_threshold || ( $remote && $remote->has_priv('suspend') ) ) {

As written, the "Other people will not see this link..." message will always display to the user if their account is less than 10 days old, but I think we only want to display it if the user has saved a URL. So I would move all this back into the if ($url) { conditional - the first one, line 527 in the original file.

... Phew, I think that's everything! Thanks for hanging in there <3

@alierak
Copy link
Member

alierak commented Jul 28, 2023

For what it's worth, I'd never heard of DateTime->now->epoch, so I checked what it does, and it's equivalent to just "time", as in my $spam_time_threshold = (time - (ONE_DAY * 10)) > $u->timecreate;

@cmho
Copy link
Contributor Author

cmho commented Jul 28, 2023

ok i thiiiiink that should do it (also tbh don't worry about the big paragraph of feedback; this is still fewer comments than i get from work lmao and that's in a language i'm good at)

@kareila
Copy link
Member

kareila commented Jul 28, 2023

For what it's worth, I'd never heard of DateTime->now->epoch, so I checked what it does, and it's equivalent to just "time", as in my $spam_time_threshold = (time - (ONE_DAY * 10)) > $u->timecreate;

Well sure, and ONE_DAY * 10 is equivalent to 864000, but there's an argument to be made that there are times when a bit of additional complexity makes the code's intent clearer to the casual observer.

@kareila
Copy link
Member

kareila commented Jul 28, 2023

Okay, three things:

  1. The direction of the inequality in the time comparison row is still backwards from what I think it should be. Have you tested it?
  2. This backs out the change from Be slightly more lenient about removing HTML links from userbios #3212
  3. Please, please, please tidy your code changes before submitting.

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

Successfully merging this pull request may close these issues.

restrict visibility of URL link for 10 days on newly created account profiles
4 participants