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

[UX] Use the human-readable name of the role in the "role deleted" message #4790

Open
klonos opened this issue Dec 8, 2020 · 9 comments · May be fixed by backdrop/backdrop#3429
Open

[UX] Use the human-readable name of the role in the "role deleted" message #4790

klonos opened this issue Dec 8, 2020 · 9 comments · May be fixed by backdrop/backdrop#3429

Comments

@klonos
Copy link
Member

klonos commented Dec 8, 2020

While working on #3813, I've noticed that the machine name of the role is shown in the message:

image

@klonos
Copy link
Member Author

klonos commented Dec 8, 2020

PR up for review/testing: backdrop/backdrop#3429

image

@indigoxela
Copy link
Member

I've noticed that the machine name of the role is shown in the message

Indeed, confirmed on a 1.17.4 install.

I did a quick check in the Tugboat sandbox and can confirm that the human readable string is shown now, when a role has been deleted. Works as expected.

@indigoxela
Copy link
Member

Oops, almost, one test is still failing:

FAIL: The role has been deleted | Other | user.test | 2193 | UserRoleAdminTestCase->testRoleAdministration()

It's not a random failure, it also fails when run via UI in the sandbox.

@indigoxela
Copy link
Member

indigoxela commented Dec 8, 2020

Nope, you already fixed that. Seems that Zen.CI/Tugboat didn't pick up the latest changes? Turn it off and on again?

@klonos
Copy link
Member Author

klonos commented Dec 8, 2020

Meh, twasn't ZenCI ...it needed a strip_tags() in assertText() (because of the <em> around the role name) ...but then ZenCI did start misbehaving ...so I did turn it off and then on again ...multiple times, and now tests are green 👍 ...FINALLY!

@indigoxela
Copy link
Member

indigoxela commented Dec 9, 2020

it needed a strip_tags() in assertText() (because of the <em>...

Right, % wraps the string in <em>, didn't check that. You could also use assertRaw() and then with markup, but strip_tags() also works.

@indigoxela
Copy link
Member

Code looks good, RTBC.

@quicksketch
Copy link
Member

I think there's an issue with calling check_plain() in the save function will cause double-encoding. I posted a review to the PR: backdrop/backdrop#3429 (review)

@jenlampton jenlampton modified the milestones: 1.17.5, 1.18.1 Jan 14, 2021
@jenlampton jenlampton modified the milestones: 1.18.1, 1.18.2 Jan 21, 2021
@jenlampton jenlampton modified the milestones: 1.18.2, 1.18.3 Mar 24, 2021
@jenlampton jenlampton modified the milestones: 1.18.3, 1.18.14 Apr 21, 2021
@jenlampton jenlampton modified the milestones: 1.18.4, 1.19.1 May 16, 2021
@jenlampton jenlampton modified the milestones: 1.19.1, 1.19.2 May 26, 2021
@indigoxela
Copy link
Member

Sadly, the related PR now has conflicts and changes have been requested, anyway.

As it got no updates since Dec. 2020 it seems stalled. Removing milestone.

@indigoxela indigoxela removed this from the 1.19.2 milestone Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants