-
Notifications
You must be signed in to change notification settings - Fork 3
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
Closes #2590 - Deleted users makes it impossible to download CSV cont… #2593
Closes #2590 - Deleted users makes it impossible to download CSV cont… #2593
Conversation
2ed2460
to
8fba0a3
Compare
@@ -27,8 +27,13 @@ public class DashboardUserInfoService { | |||
public List<DashboardUserInfo> createDashboardUsers(UserListResult userListResult) { | |||
List<DashboardUserInfo> userInfoList = new LinkedList<>(); | |||
for (AuthenticatedUser user : userListResult.getUserList()) { | |||
userInfoList.add(new DashboardUserInfo(user, getAuthProviderFriendlyName(user), | |||
hasSelectedConfirmedEmail(user), getUserNotificationLanguageDisplayName(user, session.getLocale()))); | |||
if (!user.isErased()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is the best place to filter. What about counters, are they still correct? I would have guessed that UserServiceBean
or queries in AuthenticatedUser
would be a better place to filter out erased accounts in order have a more consistent impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -40,7 +40,9 @@ public void write(OutputStream outputStream, List<AuthenticatedUser> authenticat | |||
|
|||
csvPrinter.printRecord(AuthenticatedUserCSVRecord.getHeaders()); | |||
for(AuthenticatedUser user : authenticatedUsers) { | |||
csvPrinter.printRecord(buildRecord(user).getValues()); | |||
if(! user.isErased()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, this seems to be the wrong place to filter out records.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…aining users' list
e4d33aa
to
922bacd
Compare
…aining users' list