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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename Decidim.password_blacklist to Decidim.denied_passwords #10288

Merged
merged 3 commits into from Mar 6, 2023

Conversation

alecslupu
Copy link
Contributor

@alecslupu alecslupu commented Jan 28, 2023

馃帺 What? Why?

Rename several terms like "blacklist", "whitelist".

馃搶 Related Issues

Link your PR to an issue

Testing

Describe the best way to test or validate your PR.

馃摲 Screenshots

Please add screenshots of the changes you're proposing
Description

鈾ワ笍 Thank you!

@alecslupu alecslupu force-pushed the fix/6678 branch 2 times, most recently from a935b77 to ec17e78 Compare January 28, 2023 18:48
@alecslupu alecslupu added this to Pending review from Product in Maintainers via automation Jan 28, 2023
@alecslupu alecslupu requested a review from a team January 28, 2023 19:26
@alecslupu alecslupu marked this pull request as ready for review January 28, 2023 19:27
@ahukkanen ahukkanen added module: admin type: fix PRs that implement a fix for a bug module: core labels Jan 31, 2023
Copy link
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

This is otherwise fine but I think we should also remove the terms from the translation keys.

@alecslupu
Copy link
Contributor Author

On it !

@alecslupu
Copy link
Contributor Author

I have looked, and changing the blacklist may be too complex for the scope. We will need to rename PasswordValidator#blacklisted?

Going a step further, we may need to rename the Decidim.password_blacklist config setting to something like Decidim.denied_passwords

Shall we go on this rabbit hole ?

@ahukkanen
Copy link
Contributor

Yeah, I think we need to do that and relabel this as change rather than a fix.

We did something similar before as well with the "download your data" terminology which required a lot of such changes (see #9196).

@ahukkanen ahukkanen added type: change PRs that implement a change for an existing feature and removed type: fix PRs that implement a fix for a bug labels Jan 31, 2023
@alecslupu alecslupu changed the title Replace discriminatory terms Rename Decidim.password_blacklist to Decidim.denied_passwords Jan 31, 2023
@verarojman
Copy link
Contributor

Thanks for addressing this! 馃挄

@alecslupu
Copy link
Contributor Author

@ahukkanen this can now be reviewed.

@alecslupu alecslupu moved this from Pending review from Product to To Review by Core in Maintainers Feb 9, 2023
Copy link
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

Just few suggestions.

Otherwise LGTM.

decidim-core/lib/decidim/core.rb Outdated Show resolved Hide resolved
decidim-admin/config/locales/en.yml Outdated Show resolved Hide resolved
@ahukkanen ahukkanen merged commit bfc862f into decidim:develop Mar 6, 2023
Maintainers automation moved this from To Review by Core to Done Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: admin module: core type: change PRs that implement a change for an existing feature
Projects
Development

Successfully merging this pull request may close these issues.

Replace use of whitelist with allowlist and blacklist with denylist (and other discriminatory/biased terms)
3 participants