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

Fixes #4262 - Exclude bots, spiders and thread authors from thread views #4266

Merged
merged 14 commits into from
Apr 14, 2021

Conversation

Eldenroot
Copy link
Contributor

Resolves #4262

Thank you buddy @lairdshaw - for your mentoring, I really appreciate it!

@lairdshaw
Copy link
Contributor

Happy to have helped, buddy. I think you ended up with a good outcome.

showthread.php Outdated Show resolved Hide resolved
@dvz
Copy link
Contributor

dvz commented Feb 23, 2021

It may be more intuitive to flip the meaning of these settings and use include instead of exclude.

@lairdshaw
Copy link
Contributor

lairdshaw commented Feb 23, 2021

It may be more intuitive to flip the meaning of these settings and use include instead of exclude.

Doh. It was me who advised the inversion. I still stand by that choice. I think inclusion is more ambiguous in this scenario. e.g., The user is a guest, and the setting says "include guests" so s/he is included, but the user is also not a spider, and the setting says "include spiders", so on that basis should s/he not be included...?... It's ambiguous. A principle of exclusion is not (IMO) as ambiguous: if any of the exclusion criteria are applicable, then the user is excluded. If none of them apply, then the user is included.

(I do realise that from a strictly logical point of view, this whole argument could be turned around on me, so I guess all I'm saying is that I find exclusion more intuitive - but if the consensus is that it's not, then that's OK with me).

@Eldenroot Eldenroot requested a review from dvz February 23, 2021 20:52
showthread.php Outdated Show resolved Hide resolved
Co-authored-by: dvz <devilshakerz@gmail.com>
@Eldenroot Eldenroot closed this Feb 25, 2021
@Eldenroot Eldenroot reopened this Feb 25, 2021
@Eldenroot
Copy link
Contributor Author

@dvz thank you, now it should be fine.

@dvz
Copy link
Contributor

dvz commented Feb 25, 2021

Currently No values would result in double negatives - skimming through descriptions and spotting [...] guests [...] with a Yes / No setting may be interpreted as include/exclude guests.

Would include X be mistaken for [only] include X, rather than [also] include X?

We can also use alternatives like Increment Thread View Count on Guest Visits.

install/resources/settings.xml Outdated Show resolved Hide resolved
install/resources/settings.xml Outdated Show resolved Hide resolved
install/resources/settings.xml Outdated Show resolved Hide resolved
@Sama34
Copy link
Contributor

Sama34 commented Feb 25, 2021

I think a include approach with a Increment Thread View Count on Guest Visits wording is the best approach to avoid confusion.

@Eldenroot
Copy link
Contributor Author

@Sama34 - please check this comment from @dvz - #4262 (comment)

It was changed to off by default because of the request.

- change logic
- change exclude to include
- change titles
- minor lang corrections

@dvz @Sama34
@lairdshaw
Copy link
Contributor

I defer to your stronger position, @dvz and @Sama34. Exclusion was not, as I'd initially thought, as good a framing as inclusion. You guys are correct...

@Eldenroot Eldenroot requested review from dvz and Sama34 February 25, 2021 21:34
@Eldenroot
Copy link
Contributor Author

Thank you all for your valuable feedback! Now it should be fixed and all your suggestions were implemented.

@Eldenroot Eldenroot marked this pull request as draft February 25, 2021 21:36
@Eldenroot Eldenroot marked this pull request as ready for review February 25, 2021 21:39
showthread.php Outdated Show resolved Hide resolved
install/resources/settings.xml Outdated Show resolved Hide resolved
Eldenroot and others added 2 commits February 25, 2021 23:15
Co-authored-by: dvz <devilshakerz@gmail.com>
@Ben-MyBB Ben-MyBB requested a review from dvz March 13, 2021 19:31
@Sama34 Sama34 merged commit 278802f into mybb:feature Apr 14, 2021
@Eldenroot Eldenroot deleted the patch-27 branch April 14, 2021 03:46
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.

Add options to prevent spiders and thread autor from increasing thread view count
4 participants