Replace ban/banned with suspend/suspended in user facing text #5816

rhymes · 2020-01-30T12:47:29Z

What type of PR is this? (check all applicable)

Description

We're going to change the language, at least in user facing strings, from ban/banned/banished to suspend/suspended.

citizen428

NOTE: approved despite failing check because it's a simple wording change from "banned" to "suspended" to make it work.

The PR description mentions "at least in user facing strings", so I think this is ok. 👍

Overall though I'm a fan of having a ubiquitous language, which would include renaming the methods at one point. The more words we have for the same thing ("ban", "banish", "suspend"), the harder it will become to onboard new people.

citizen428

👍

citizen428

Looking good @rhymes, thanks. Just left some questions.

citizen428 · 2020-02-05T07:44:23Z

app/controllers/application_controller.rb

@@ -46,7 +46,7 @@ def after_sign_in_path_for(resource)
  end

  def raise_banned
-    raise "BANNED" if current_user&.banned
+    raise "SUSPENDED" if current_user&.banned


General question: the way this is used is through before_actions like this one:

before_action :raise_banned, only: %i[new create update]

Could it potentially make sense to move this check into a policy?

Also on the danger of sounding nitpicky, it's a bit confusing to see a method called raise_banned and then raise "SUSPENDED" right below. Maybe we can rename the method to raise_suspended`? It only would need to change in 3 places from what I can tell.

Could it potentially make sense to move this check into a policy?

Not sure TBH as I'm totally unfamiliar with all the suspension tooling, but I think it's out of the scope here

Also on the danger of sounding nitpicky, it's a bit confusing to see a method called raise_banned and then raise "SUSPENDED" right below. Maybe we can rename the method to raise_suspended`? It only would need to change in 3 places from what I can tell.

Done!

Agree that a policy would be outside the scope of this PR, but it seemed like a good point to start the discussion. 😃

Thanks for changing the method name!

How painful would it be to rename the role on the user as well? Just looking at this method, we have current_user&.banned, which might cause a bit of confusion since "banned" is actually now "suspended". I don't think this should block this PR, but we probably shouldn't have too many different ways to refer to the same concept.

Also, looking at role.rb, we also have a comment_banned role. Do we need to make any modifications to handle that?

Yeah, the goal is to slowly change it all I think. We'd probably need a migration to go over the roles table and update that role.

Let's see what we can do in a follow up PR :)

That sounds great. Definitely seems like it could be its own PR 😄

mstruve

We need to update our honeybadger error handling to ensure these errors are grouped appropriately

rhymes · 2020-02-07T01:13:59Z

@mstruve please take over this PR as I'm not entirely sure what I should put in the initializer 👀

SUSPENDED => suspended ?
SUSPENDED => banned ?

something else?

mstruve · 2020-02-07T16:18:59Z

Either works but if we don't want a new error to pop up in Honeybadger then lets go with

SUSPENDED => banned

mstruve

LGTM 🚀

Replace ban/banned/banished with suspend/suspended in user facing text

5357b17

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 30, 2020

rhymes requested review from citizen428 and jacobherrington January 30, 2020 12:47

citizen428 previously approved these changes Jan 31, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 31, 2020

Fix broken specs

a737cb8

rhymes dismissed citizen428’s stale review via a737cb8 January 31, 2020 11:19

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Jan 31, 2020

rhymes requested a review from citizen428 January 31, 2020 11:19

citizen428 previously approved these changes Feb 3, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 3, 2020

weekly-digest bot mentioned this pull request Feb 3, 2020

Weekly Digest (27 January, 2020 - 3 February, 2020) #5847

Closed

Banished should stay banished

bfd7147

rhymes dismissed citizen428’s stale review via bfd7147 February 4, 2020 17:12

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 4, 2020

rhymes changed the title ~~Replace ban/banned/banished with suspend/suspended in user facing text~~ Replace ban/banned with suspend/suspended in user facing text Feb 4, 2020

rhymes requested a review from citizen428 February 4, 2020 17:13

citizen428 reviewed Feb 5, 2020

View reviewed changes

Rename raise_banned to raise_suspended

7160391

rhymes requested a review from citizen428 February 5, 2020 12:12

citizen428 previously approved these changes Feb 6, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 6, 2020

rhymes requested a review from vaidehijoshi February 6, 2020 08:34

vaidehijoshi previously approved these changes Feb 6, 2020

View reviewed changes

mstruve suggested changes Feb 6, 2020

View reviewed changes

pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 6, 2020

weekly-digest bot mentioned this pull request Feb 10, 2020

Weekly Digest (3 February, 2020 - 10 February, 2020) #5980

Closed

Update Honeybadger configuration

9a7e9fb

rhymes dismissed stale reviews from vaidehijoshi and citizen428 via 9a7e9fb February 10, 2020 13:48

rhymes requested a review from a team February 10, 2020 13:48

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Feb 10, 2020

rhymes requested review from mstruve and vaidehijoshi February 10, 2020 13:49

mstruve approved these changes Feb 10, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 10, 2020

vaidehijoshi approved these changes Feb 10, 2020

View reviewed changes

mstruve merged commit c41d66e into forem:master Feb 10, 2020

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 10, 2020

rhymes deleted the rhymes/replace-ban_banned-suspend_suspended branch February 10, 2020 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace ban/banned with suspend/suspended in user facing text #5816

Replace ban/banned with suspend/suspended in user facing text #5816

rhymes commented Jan 30, 2020

citizen428 left a comment •

edited

citizen428 left a comment

citizen428 left a comment

citizen428 Feb 5, 2020

citizen428 Feb 5, 2020

rhymes Feb 5, 2020

citizen428 Feb 6, 2020

vaidehijoshi Feb 6, 2020

rhymes Feb 6, 2020

vaidehijoshi Feb 6, 2020

mstruve left a comment

rhymes commented Feb 7, 2020

mstruve commented Feb 7, 2020

mstruve left a comment

Replace ban/banned with suspend/suspended in user facing text #5816

Replace ban/banned with suspend/suspended in user facing text #5816

Conversation

rhymes commented Jan 30, 2020

What type of PR is this? (check all applicable)

Description

citizen428 left a comment • edited

Choose a reason for hiding this comment

citizen428 left a comment

Choose a reason for hiding this comment

citizen428 left a comment

Choose a reason for hiding this comment

citizen428 Feb 5, 2020

Choose a reason for hiding this comment

citizen428 Feb 5, 2020

Choose a reason for hiding this comment

rhymes Feb 5, 2020

Choose a reason for hiding this comment

citizen428 Feb 6, 2020

Choose a reason for hiding this comment

vaidehijoshi Feb 6, 2020

Choose a reason for hiding this comment

rhymes Feb 6, 2020

Choose a reason for hiding this comment

vaidehijoshi Feb 6, 2020

Choose a reason for hiding this comment

mstruve left a comment

Choose a reason for hiding this comment

rhymes commented Feb 7, 2020

mstruve commented Feb 7, 2020

mstruve left a comment

Choose a reason for hiding this comment

citizen428 left a comment •

edited