Add an option to restrict which locales a user can edit #1004
Conversation
| } | ||
|
|
||
| return ( | ||
| <div className="mtm users-locale-select-root"> |
There was a problem hiding this comment.
Functionality looks good!
Is it possible to disable the entire locale selection section and uncheck the can translate all locales checkbox if a users role is ROLE_USER? When testing the functionality I enabled de-DE and fr-FR for a user but then couldn't translate those locales in the UI until I realised I needed to set the users role to TRANSLATOR too, I think having these config boxes disabled and unselected for users with the basic user role would make that requirement clear.
| @@ -55,6 +55,9 @@ public class User extends AuditableEntity implements Serializable { | |||
| @JsonView(View.IdAndName.class) | |||
| String commonName; | |||
|
|
|||
| @Column(name = "can_translate_all_locales", nullable = false) | |||
| boolean canTranslateAllLocales = true; | |||
There was a problem hiding this comment.
If the default user role is USER I think we should default this value to false, otherwise it might be confusing in the GUI that a basic user has this enabled but still can't translate any locales
There was a problem hiding this comment.
I think, we should keep it as true. The default role for new users is USER, so it will likely be fairly common to accidentally create USER users instead of TRANSLATOR users. If the ADMIN then changes the role for the new user, then the new user will still be unable to translate anything, because the canTranslateAllLocales is now false. --> The ADMIN now has to change two settings instead of one.
Additionally, in your scenario, what is supposed to happen if the ADMIN downgrades a TRANSLATOR to USER? Should canTranslateAllLocales also be disabled? In this case the role selection dropdown would effectively control two inputs, which feels inconsistent and unintuitive. If the canTranslateAllLocales field is not disabled it is inconsistent with creating a new user.
So, I would rather solve this in the GUI, by disabling the checkbox for USER and displaying a short note explaining that this option won't have an effect because of the currently selected role. IMHO, this would have the least surprising behaviour for all users.
There was a problem hiding this comment.
I'm happy for it to be handled in the GUI as you describe 👍
There was a problem hiding this comment.
Should be implemented now.
| > | ||
| {this.props.modal.selectedLocale} | ||
| </Dropdown.Toggle> | ||
| <Dropdown.Menu |
There was a problem hiding this comment.
The contents of this dropdown list is every locale in the DB, it might be better to just have a search ahead that displays a few options as the user starts entering in the locale details if that's possible? You do already seem to have that functionality there so if there's a way to remove the initial list of every locale and just have the 'search ahead' leftover that would be nice. It's not a major issue if that's not a straightforward update.
There was a problem hiding this comment.
With the a dropdown, I would be against it, since the default expectation with a dropdown element is that all elements are present, and not that typing to find a specific locale is mandatory.
I could change it to a regular input field and use a Datalist instead, but then you would loose the option to browse through all available locales.
There was a problem hiding this comment.
My feeling is that if someone is selecting locales that a user is allowed to translate then they'll likely be coming with a short list of specific locales to enable so I would be leaning towards a Datalist and dropping the ability to scroll through all locales in a dropdown.
@aurambaj any thoughts on this?
There was a problem hiding this comment.
Changed it to a datalist for now.
| @@ -206,7 +262,7 @@ public User updatePassword(String currentPassword, String newPassword) { | |||
| * @return The newly created user | |||
| */ | |||
| public User createUserWithRole(String username, String password, Role role) { | |||
| return createUserWithRole(username, password, role, null, null, null, false); | |||
| return createUserWithRole(username, password, role, null, null, null, null, true, false); | |||
There was a problem hiding this comment.
If the user role is a basic user I think we should set the canTranslateAllLocales boolean to false, so something like
createUserWithRole(username, password, role, null, null, null, null, role != ROLE_USER ? true : false , false);
| @@ -293,8 +349,9 @@ public User createBasicUser( | |||
| givenName, | |||
| surname, | |||
| commonName, | |||
| null, | |||
| true, | |||
|
@mensinda Thanks for raising this PR, I've tested the changes and left a few review comments. Overall it looks good though 😄 |
mensinda
force-pushed
the
user_locales
branch
2 times, most recently
from
ac09c8f
to
c0ebe29
Compare
|
Is there anything missing for this to get merged? I think everything from the review should be addressed with the discussion and changes here. |
This PR adds the option to restrict what locales a user can edit. This way, users that are not that well versed with mojito can't accidentally change the translations for wrong locales.