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.

Sign up for GitHub

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

Add an option to restrict which locales a user can edit #1004

Merged
merged 4 commits into from
Oct 9, 2024
+655 −32
Merged

Add an option to restrict which locales a user can edit #1004

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f2fce47
Add an option to restrict which locales a user can edit
mensinda Feb 23, 2024
1fa84ce
dropdown --> datalist
mensinda Mar 6, 2024
d8684ca
Merge branch 'master' into user_locales
aurambaj Oct 5, 2024
fd0f0e4
Try fixing spotless check
mensinda Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/src/test/java/com/box/l10n/mojito/cli/command/UserDeleteCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public void testDelete() {
String commonName = "Test Mojito";

userService.createUserWithRole(
username, password, Role.ROLE_USER, givenName, surname, commonName, false);
username, password, Role.ROLE_USER, givenName, surname, commonName, null, true, false);
User user = userRepository.findByUsername(username);
assertNotNull(user);

Expand Down
2 changes: 1 addition & 1 deletion cli/src/test/java/com/box/l10n/mojito/cli/command/UserUpdateCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ private User createTestUserUsingUserService(String username, String rolename) {
}
User user =
userService.createUserWithRole(
username, password, role, givenName, surname, commonName, false);
username, password, role, givenName, surname, commonName, null, true, false);
return user;
}
}
20 changes: 20 additions & 0 deletions restclient/src/main/java/com/box/l10n/mojito/rest/entity/User.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,12 @@ public class User {

private String commonName;

private boolean canTranslateAllLocales;

@JsonManagedReference Set<Authority> authorities = new HashSet<>();

@JsonManagedReference Set<UserLocale> userLocales = new HashSet<>();

public Long getId() {
return id;
}
Expand Down Expand Up @@ -90,4 +94,20 @@ public Set<Authority> getAuthorities() {
public void setAuthorities(Set<Authority> authorities) {
this.authorities = authorities;
}

public boolean getCanTranslateAllLocales() {
return canTranslateAllLocales;
}

public void setCanTranslateAllLocales(boolean canTranslateAllLocales) {
this.canTranslateAllLocales = canTranslateAllLocales;
}

public Set<UserLocale> getUserLocales() {
return userLocales;
}

public void setUserLocales(Set<UserLocale> userLocales) {
this.userLocales = userLocales;
}
}
25 changes: 25 additions & 0 deletions restclient/src/main/java/com/box/l10n/mojito/rest/entity/UserLocale.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package com.box.l10n.mojito.rest.entity;

import com.fasterxml.jackson.annotation.JsonBackReference;

public class UserLocale {
@JsonBackReference private User user;

private Locale locale;

public User getUser() {
return user;
}

public void setUser(User user) {
this.user = user;
}

public Locale getLocale() {
return locale;
}

public void setLocale(Locale locale) {
this.locale = locale;
}
}
29 changes: 28 additions & 1 deletion webapp/src/main/java/com/box/l10n/mojito/entity/security/user/User.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,16 @@
name = "User.legacy",
attributeNodes = {
@NamedAttributeNode("createdByUser"),
@NamedAttributeNode(value = "userLocales", subgraph = "User.legacy.userLocales"),
@NamedAttributeNode(value = "authorities", subgraph = "User.legacy.authorities")
},
subgraphs = {
@NamedSubgraph(
name = "User.legacy.authorities",
attributeNodes = {@NamedAttributeNode("createdByUser"), @NamedAttributeNode("user")})
attributeNodes = {@NamedAttributeNode("createdByUser"), @NamedAttributeNode("user")}),
@NamedSubgraph(
name = "User.legacy.userLocales",
attributeNodes = {@NamedAttributeNode("user")}),
})
public class User extends AuditableEntity implements Serializable {

Expand Down Expand Up @@ -69,6 +73,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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy for it to be handled in the GUI as you describe 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be implemented now.


/**
* Sets this flag if the user is created by a process that don't have all the information. Eg.
* pushing an asset for a branch with an owner or header base authentication. If the owner is not
Expand All @@ -80,6 +87,10 @@ public class User extends AuditableEntity implements Serializable {
@Column(name = "partially_created")
Boolean partiallyCreated = false;

@JsonManagedReference
@OneToMany(mappedBy = "user", fetch = FetchType.LAZY)
Set<UserLocale> userLocales = new HashSet<>();

@JsonManagedReference
@OneToMany(mappedBy = "user", fetch = FetchType.LAZY)
Set<Authority> authorities = new HashSet<>();
Expand Down Expand Up @@ -156,6 +167,22 @@ public void setCommonName(String commonName) {
this.commonName = commonName;
}

public boolean getCanTranslateAllLocales() {
return canTranslateAllLocales;
}

public void setCanTranslateAllLocales(boolean canTranslateAllLocales) {
this.canTranslateAllLocales = canTranslateAllLocales;
}

public Set<UserLocale> getUserLocales() {
return userLocales;
}

public void setUserLocales(Set<UserLocale> userLocales) {
this.userLocales = userLocales;
}

public Boolean getPartiallyCreated() {
return partiallyCreated;
}
Expand Down
67 changes: 67 additions & 0 deletions webapp/src/main/java/com/box/l10n/mojito/entity/security/user/UserLocale.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package com.box.l10n.mojito.entity.security.user;

import com.box.l10n.mojito.entity.BaseEntity;
import com.box.l10n.mojito.entity.Locale;
import com.box.l10n.mojito.rest.View;
import com.fasterxml.jackson.annotation.JsonBackReference;
import com.fasterxml.jackson.annotation.JsonView;
import jakarta.persistence.Entity;
import jakarta.persistence.ForeignKey;
import jakarta.persistence.Index;
import jakarta.persistence.JoinColumn;
import jakarta.persistence.ManyToOne;
import jakarta.persistence.Table;
import java.io.Serializable;
import org.hibernate.annotations.BatchSize;

@Entity
@Table(
name = "user_locale",
indexes = {
@Index(
name = "UK__USER_LOCALE__USER_ID__LOCALE_ID",
columnList = "user_id, locale_id",
unique = true)
})
@BatchSize(size = 1000)
public class UserLocale extends BaseEntity implements Serializable {

@ManyToOne
@JsonBackReference
@JoinColumn(
name = "user_id",
foreignKey = @ForeignKey(name = "FK__USER_LOCALE__USER__ID"),
nullable = false)
User user;

@JsonView(View.LocaleSummary.class)
@ManyToOne
@JoinColumn(
name = "locale_id",
foreignKey = @ForeignKey(name = "FK__USER_LOCALE__LOCALE__ID"),
nullable = false)
Locale locale;

public UserLocale() {}

public UserLocale(User user, Locale locale) {
this.user = user;
this.locale = locale;
}

public User getUser() {
return user;
}

public void setUser(User user) {
this.user = user;
}

public Locale getLocale() {
return locale;
}

public void setLocale(Locale locale) {
this.locale = locale;
}
}
8 changes: 8 additions & 0 deletions webapp/src/main/java/com/box/l10n/mojito/react/ReactAppController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.box.l10n.mojito.react;

import com.box.l10n.mojito.entity.security.user.Authority;
import com.box.l10n.mojito.entity.security.user.UserLocale;
import com.box.l10n.mojito.json.ObjectMapper;
import com.box.l10n.mojito.mustache.MustacheBaseContext;
import com.box.l10n.mojito.mustache.MustacheTemplateEngine;
Expand All @@ -15,6 +16,7 @@
import java.nio.charset.StandardCharsets;
import java.util.IllformedLocaleException;
import java.util.Locale;
import java.util.stream.Collectors;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
Expand Down Expand Up @@ -134,6 +136,12 @@ ReactUser getReactUser() {
reactUser.setGivenName(currentAuditor.getGivenName());
reactUser.setSurname(currentAuditor.getSurname());
reactUser.setCommonName(currentAuditor.getCommonName());
reactUser.setCanTranslateAllLocales(currentAuditor.getCanTranslateAllLocales());
reactUser.setUserLocales(
currentAuditor.getUserLocales().stream()
.map(UserLocale::getLocale)
.map(com.box.l10n.mojito.entity.Locale::getBcp47Tag)
.collect(Collectors.toList()));

Role role = Role.ROLE_USER;
Authority authority = authorityRepository.findByUser(currentAuditor);
Expand Down
19 changes: 19 additions & 0 deletions webapp/src/main/java/com/box/l10n/mojito/react/ReactUser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.box.l10n.mojito.react;

import com.box.l10n.mojito.security.Role;
import java.util.List;
import org.springframework.stereotype.Component;

@Component
Expand All @@ -11,6 +12,8 @@ public class ReactUser {
String surname;
String commonName;
Role role;
boolean canTranslateAllLocales;
List<String> userLocales;

public String getUsername() {
return username;
Expand Down Expand Up @@ -51,4 +54,20 @@ public Role getRole() {
public void setRole(Role role) {
this.role = role;
}

public boolean getCanTranslateAllLocales() {
return canTranslateAllLocales;
}

public void setCanTranslateAllLocales(boolean canTranslateAllLocales) {
this.canTranslateAllLocales = canTranslateAllLocales;
}

public List<String> getUserLocales() {
return userLocales;
}

public void setUserLocales(List<String> userLocales) {
this.userLocales = userLocales;
}
}
13 changes: 13 additions & 0 deletions webapp/src/main/java/com/box/l10n/mojito/rest/security/UserWS.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,14 @@
import static org.slf4j.LoggerFactory.getLogger;
import static org.springframework.data.jpa.domain.Specification.where;

import com.box.l10n.mojito.entity.Locale;
import com.box.l10n.mojito.entity.security.user.Authority;
import com.box.l10n.mojito.entity.security.user.User;
import com.box.l10n.mojito.entity.security.user.UserLocale;
import com.box.l10n.mojito.security.Role;
import com.box.l10n.mojito.service.security.user.UserRepository;
import com.box.l10n.mojito.service.security.user.UserService;
import java.util.stream.Collectors;
import org.slf4j.Logger;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.data.domain.Page;
Expand Down Expand Up @@ -98,6 +101,11 @@ public ResponseEntity<User> createUser(@RequestBody User user) {
user.getGivenName(),
user.getSurname(),
user.getCommonName(),
user.getUserLocales().stream()
.map(UserLocale::getLocale)
.map(Locale::getBcp47Tag)
.collect(Collectors.toSet()),
user.getCanTranslateAllLocales(),
false);

return new ResponseEntity<>(createdUser, HttpStatus.CREATED);
Expand Down Expand Up @@ -151,6 +159,11 @@ public void updateUserByUserId(@PathVariable Long userId, @RequestBody User user
user.getGivenName(),
user.getSurname(),
user.getCommonName(),
user.getUserLocales().stream()
.map(UserLocale::getLocale)
.map(Locale::getBcp47Tag)
.collect(Collectors.toSet()),
user.getCanTranslateAllLocales(),
false);
}

Expand Down
Loading
Loading