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

Correctly interpret RR settings when forced by subreddit settings #447

Merged
merged 2 commits into from
Jan 21, 2021

Conversation

eritbh
Copy link
Member

@eritbh eritbh commented Jan 21, 2021

Fixes #445. I think previously some of these controls were disabled if the parent setting for them wasn't selected (e.g. "sticky reply" was disabled if you were sending the reason as a PM rather than a comment, since PMs can't be stickied). Now though it doesn't seem like they're disabled unless they're set and forced by subreddit configuration, and in that scenario we want them to apply, just not be changed.

@eritbh eritbh added bug something isn't working module: removalreasons labels Jan 21, 2021
@eritbh eritbh added this to the v5.5 milestone Jan 21, 2021
@creesch
Copy link
Member

creesch commented Jan 21, 2021

Now though it doesn't seem like they're disabled unless they're set and forced by subreddit configuration

Did you do a extra check to confirm this? Seems like you actually added it 4 years ago specifically to deal with comments. I haven't checked it myself yet properly, but just wanted to bring it up before we introduce regression here.

@eritbh
Copy link
Member Author

eritbh commented Jan 21, 2021

Ohh, that makes sense. You're right, I'll have to change this around a bit to properly handle comments.

@eritbh eritbh changed the title Ignore disabled state of removal reason setting controls Correctly interpret RR settings when forced by subreddit settings Jan 21, 2021
@eritbh
Copy link
Member Author

eritbh commented Jan 21, 2021

Resolved with last push. The disabled state of the inputs is still ignored when reading the settings in code, but the options that only apply to posts will not be checked for visual consistency.

In the future we might want to have a clearer distinction here between "You can't change this because it doesn't apply to the kind of thing you're removing" and "you can't change this because your subreddit enforces these settings".

@Venefilyn
Copy link
Member

In the future we might want to have a clearer distinction here between "You can't change this because it doesn't apply to the kind of thing you're removing" and "you can't change this because your subreddit enforces these settings".

In an ideal UX world we wouldn't even display it. Why show the user content when it is completely useless and is just clutter?

Copy link
Member

@Venefilyn Venefilyn left a comment

Choose a reason for hiding this comment

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

Not tested it in a browser but LGTM

@eritbh eritbh merged commit 678413d into master Jan 21, 2021
@eritbh eritbh deleted the fix-rr-setting-checks branch January 21, 2021 23:03
eritbh added a commit that referenced this pull request Sep 5, 2024
* Ignore disabled state of setting controls

* Don't try to take invalid RR actions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something isn't working module: removalreasons
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removal reasons settings aren't read correctly when the "force" sub option is used
3 participants