Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,47 @@ public static class ApplicationConfig {

@NotBlank
private String name;

@NotNull
private FeedbackConfig feedback;

@NotNull
private GoogleApiConfig googleApi;

@Getter
@Setter
@ConfigurationProperties("feedback")
public static class FeedbackConfig {

@NotNull
private Integer maxSuggestions;

@NotBlank
private String requestSubject;
}

@Getter
@Setter
@ConfigurationProperties("google-api")
public static class GoogleApiConfig {

@NotBlank
private String delegatedUser;

@NotNull
private ScopeConfig scopes;

@Getter
@Setter
@ConfigurationProperties("scopes")
public static class ScopeConfig {

@NotBlank
private String scopeForDriveApi;

@NotBlank
private String scopeForDirectoryApi;
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
package com.objectcomputing.checkins.services.feedback.suggestions;

import com.objectcomputing.checkins.configuration.CheckInsConfiguration;
import com.objectcomputing.checkins.exceptions.PermissionException;
import com.objectcomputing.checkins.services.memberprofile.MemberProfile;
import com.objectcomputing.checkins.services.memberprofile.MemberProfileServices;
import com.objectcomputing.checkins.services.memberprofile.currentuser.CurrentUserServices;
import com.objectcomputing.checkins.services.team.member.TeamMember;
import com.objectcomputing.checkins.services.team.member.TeamMemberServices;
import io.micronaut.context.annotation.Property;
import jakarta.inject.Singleton;

import java.time.LocalDate;
Expand All @@ -19,24 +19,21 @@
import static com.objectcomputing.checkins.services.validate.PermissionsValidation.NOT_AUTHORIZED_MSG;

@Singleton
public class FeedbackSuggestionServiceImpl implements FeedbackSuggestionsService {

public static final String MAX_SUGGESTIONS = "check-ins.application.feedback.max-suggestions";
class FeedbackSuggestionServiceImpl implements FeedbackSuggestionsService {

private final MemberProfileServices memberProfileServices;
private final CurrentUserServices currentUserServices;
private final TeamMemberServices teamMemberServices;
private final Integer maxSuggestions;


public FeedbackSuggestionServiceImpl(MemberProfileServices memberProfileServices,
CurrentUserServices currentUserServices,
TeamMemberServices teamMemberServices,
@Property(name = MAX_SUGGESTIONS) Integer maxSuggestions) {
FeedbackSuggestionServiceImpl(MemberProfileServices memberProfileServices,
CurrentUserServices currentUserServices,
TeamMemberServices teamMemberServices,
CheckInsConfiguration checkInsConfiguration) {
this.memberProfileServices = memberProfileServices;
this.currentUserServices = currentUserServices;
this.teamMemberServices = teamMemberServices;
this.maxSuggestions = maxSuggestions;
this.maxSuggestions = checkInsConfiguration.getApplication().getFeedback().getMaxSuggestions();
}

@Override
Expand All @@ -50,49 +47,47 @@ public List<FeedbackSuggestionDTO> getSuggestionsByProfileId(UUID id) {
}

List<FeedbackSuggestionDTO> suggestions = new LinkedList<>();
if(suggestFor.getSupervisorid() != null && !suggestFor.getSupervisorid().equals(currentUser.getId()) && isMemberActive(suggestFor.getSupervisorid())) {
if (suggestFor.getSupervisorid() != null && !suggestFor.getSupervisorid().equals(currentUser.getId()) && isMemberActive(suggestFor.getSupervisorid())) {
suggestions.add(new FeedbackSuggestionDTO("Supervisor of requestee", suggestFor.getSupervisorid()));
}

if(suggestFor.getPdlId() != null && !suggestFor.getPdlId().equals(currentUser.getId()) && isMemberActive(suggestFor.getPdlId())) {
if (suggestFor.getPdlId() != null && !suggestFor.getPdlId().equals(currentUser.getId()) && isMemberActive(suggestFor.getPdlId())) {
suggestions.add(new FeedbackSuggestionDTO("PDL of requestee", suggestFor.getPdlId()));
}

Set<TeamMember> teamMemberships = teamMemberServices.findByFields(null, id, null);

for(TeamMember currentMembership: teamMemberships){
for (TeamMember currentMembership : teamMemberships) {
Set<TeamMember> teamMembers = teamMemberServices.findByFields(currentMembership.getTeamId(), null, null);
Set<TeamMember> leads = teamMembers.stream().filter(member -> member.isLead()).collect(Collectors.toSet());
Set<TeamMember> leads = teamMembers.stream().filter(TeamMember::isLead).collect(Collectors.toSet());
leads = filterTerminated(leads);
for(TeamMember lead: leads) {
if(suggestions.size() < maxSuggestions && !lead.getMemberId().equals(id) && !lead.getMemberId().equals(currentUserId)) {
for (TeamMember lead : leads) {
if (suggestions.size() < maxSuggestions && !lead.getMemberId().equals(id) && !lead.getMemberId().equals(currentUserId)) {
suggestions.add(new FeedbackSuggestionDTO("Team lead for requestee", lead.getMemberId()));
}
}

if(suggestions.size() >= maxSuggestions) break;
if (suggestions.size() >= maxSuggestions) break;
}

for(TeamMember currentMembership: teamMemberships){
for (TeamMember currentMembership : teamMemberships) {
Set<TeamMember> teamMembers = teamMemberServices.findByFields(currentMembership.getTeamId(), null, null);
teamMembers = teamMembers.stream().filter(member -> !member.isLead()).collect(Collectors.toSet());
teamMembers = filterTerminated(teamMembers);
for(TeamMember teamMember: teamMembers) {
if(suggestions.size() < maxSuggestions && !teamMember.getMemberId().equals(id) && !teamMember.getMemberId().equals(currentUserId)) {
for (TeamMember teamMember : teamMembers) {
if (suggestions.size() < maxSuggestions && !teamMember.getMemberId().equals(id) && !teamMember.getMemberId().equals(currentUserId)) {
suggestions.add(new FeedbackSuggestionDTO("Team member for requestee", teamMember.getMemberId()));
}
}

if(suggestions.size() >= maxSuggestions) break;
if (suggestions.size() >= maxSuggestions) break;
}

return suggestions;
}

private Set<TeamMember> filterTerminated(Set<TeamMember> suggestions) {
suggestions = suggestions.stream().filter((TeamMember suggestion) -> {
return isMemberActive(suggestion.getMemberId());
}).collect(Collectors.toSet());
suggestions = suggestions.stream().filter(suggestion -> isMemberActive(suggestion.getMemberId())).collect(Collectors.toSet());
return suggestions;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import com.objectcomputing.checkins.services.reviews.ReviewPeriod;
import com.objectcomputing.checkins.services.reviews.ReviewPeriodRepository;
import com.objectcomputing.checkins.util.Util;
import io.micronaut.context.annotation.Property;
import jakarta.inject.Named;
import jakarta.inject.Singleton;
import org.slf4j.Logger;
Expand All @@ -35,8 +34,6 @@ public class FeedbackRequestServicesImpl implements FeedbackRequestServices {

private static final Logger LOG = LoggerFactory.getLogger(FeedbackRequestServicesImpl.class);

public static final String FEEDBACK_REQUEST_NOTIFICATION_SUBJECT = "check-ins.application.feedback.notifications.subject";
public static final String FEEDBACK_REQUEST_NOTIFICATION_CONTENT = "check-ins.application.feedback.notifications.content";
private final FeedbackRequestRepository feedbackReqRepository;
private final CurrentUserServices currentUserServices;
private final MemberProfileServices memberProfileServices;
Expand All @@ -50,15 +47,14 @@ public FeedbackRequestServicesImpl(FeedbackRequestRepository feedbackReqReposito
MemberProfileServices memberProfileServices,
ReviewPeriodRepository reviewPeriodRepository,
@Named(MailJetFactory.HTML_FORMAT) EmailSender emailSender,
@Property(name = FEEDBACK_REQUEST_NOTIFICATION_SUBJECT) String notificationSubject,
CheckInsConfiguration checkInsConfiguration
) {
this.feedbackReqRepository = feedbackReqRepository;
this.currentUserServices = currentUserServices;
this.memberProfileServices = memberProfileServices;
this.reviewPeriodRepository = reviewPeriodRepository;
this.emailSender = emailSender;
this.notificationSubject = notificationSubject;
this.notificationSubject = checkInsConfiguration.getApplication().getFeedback().getRequestSubject();
this.webURL = checkInsConfiguration.getWebAddress();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,15 @@ public class GoogleAccessor {

private final JsonFactory JSON_FACTORY = GsonFactory.getDefaultInstance();
private final NetHttpTransport httpTransport = GoogleNetHttpTransport.newTrustedTransport();
private final String applicationName;
private final GoogleAuthenticator authenticator;
private final Environment environment;
private final CheckInsConfiguration checkInsConfiguration;

public GoogleAccessor(
GoogleAuthenticator authenticator,
Environment environment,
CheckInsConfiguration checkInsConfiguration
) throws GeneralSecurityException, IOException {
this.applicationName = checkInsConfiguration.getApplication().getName();
this.checkInsConfiguration = checkInsConfiguration;
this.authenticator = authenticator;
this.environment = environment;
}

/**
Expand All @@ -47,13 +44,13 @@ public GoogleAccessor(
*/
public Drive accessGoogleDrive() throws IOException {

String apiScope = environment.getProperty("check-ins.application.google-api.scopes.scopeForDriveApi", String.class).orElse("");
String apiScope = checkInsConfiguration.getApplication().getGoogleApi().getScopes().getScopeForDriveApi();
List<String> scope = Collections.singletonList(apiScope);

HttpRequestInitializer requestInitializer = new HttpCredentialsAdapter(authenticator.setupCredentials(scope));
return new Drive
.Builder(httpTransport, JSON_FACTORY, requestInitializer)
.setApplicationName(applicationName)
.setApplicationName(checkInsConfiguration.getApplication().getName())
.build();
}

Expand All @@ -65,16 +62,16 @@ public Drive accessGoogleDrive() throws IOException {
*/
public Directory accessGoogleDirectory() throws IOException {

String apiScope = environment.getProperty("check-ins.application.google-api.scopes.scopeForDirectoryApi", String.class).orElse("");
String delegatedUser = environment.getProperty("check-ins.application.google-api.delegated-user", String.class).orElse("");
String apiScope = checkInsConfiguration.getApplication().getGoogleApi().getScopes().getScopeForDirectoryApi();
String delegatedUser = checkInsConfiguration.getApplication().getGoogleApi().getDelegatedUser();
List<String> scope = Arrays.asList(apiScope.split(","));

HttpRequestInitializer requestInitializer = new HttpCredentialsAdapter(
authenticator.setupServiceAccountCredentials(scope, delegatedUser));

return new Directory
.Builder(httpTransport, JSON_FACTORY, requestInitializer)
.setApplicationName(applicationName)
.setApplicationName(checkInsConfiguration.getApplication().getName())
.build();
}
}
8 changes: 3 additions & 5 deletions server/src/main/resources/application.yml
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,11 @@ check-ins:
google-api:
delegated-user: ${ GSUITE_SUPER_ADMIN }
scopes:
scopeForDriveApi: "https://www.googleapis.com/auth/drive.file"
scopeForDirectoryApi: "https://www.googleapis.com/auth/admin.directory.user.readonly"
scope-for-drive-api: "https://www.googleapis.com/auth/drive.file"
scope-for-directory-api: "https://www.googleapis.com/auth/admin.directory.user.readonly"
Comment on lines -95 to +96
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a no-op change, but kebab-case configuration is more idiomatic than camelCase

feedback:
max-suggestions: 6
notifications:
subject: "Feedback request"
content: "You have received a feedback request. Please go to the "
request-subject: "Feedback request"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

content was unused so I removed it, and flattened the structure 1 level

web-address: ${ WEB_ADDRESS }
---
flyway:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
package com.objectcomputing.checkins.services.feedback.suggestions;

import com.objectcomputing.checkins.services.TestContainersSuite;
import com.objectcomputing.checkins.services.fixture.*;
import com.objectcomputing.checkins.services.fixture.FeedbackFixture;
import com.objectcomputing.checkins.services.fixture.MemberProfileFixture;
import com.objectcomputing.checkins.services.fixture.RoleFixture;
import com.objectcomputing.checkins.services.fixture.TeamFixture;
import com.objectcomputing.checkins.services.fixture.TeamMemberFixture;
import com.objectcomputing.checkins.services.memberprofile.MemberProfile;
import com.objectcomputing.checkins.services.role.RoleType;
import com.objectcomputing.checkins.services.team.Team;
Expand All @@ -12,31 +16,25 @@
import io.micronaut.http.HttpStatus;
import io.micronaut.http.client.HttpClient;
import io.micronaut.http.client.annotation.Client;
import jakarta.inject.Inject;
import org.junit.jupiter.api.Test;

import jakarta.inject.Inject;
import java.util.*;
import java.util.List;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import static org.junit.jupiter.api.Assertions.*;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;

class FeedbackSuggestionsControllerTest extends TestContainersSuite implements MemberProfileFixture, TeamMemberFixture, TeamFixture, FeedbackFixture, RoleFixture {
private static final Logger LOG = LoggerFactory.getLogger(FeedbackSuggestionsControllerTest.class);

private final String supervisorReason = "Supervisor of requestee";
private final String teamLeadReason = "Team lead for requestee";
private final String teamMemberReason = "Team member for requestee";
private final String pdlReason = "PDL of requestee";


@Inject
@Client("/services/feedback/suggestions")
HttpClient client;

void assertContentEqualsEntity(FeedbackSuggestionDTO ideal, FeedbackSuggestionDTO actualResponse) {
assertEquals(ideal.getReason(), actualResponse.getReason());
assertEquals(ideal.getId(), actualResponse.getId());
}
@Test
void testGetRecsIfPdl() {
Team team = createDefaultTeam();
Expand Down Expand Up @@ -190,4 +188,9 @@ void testDoesNotIncludeTerminatedTeamMembers() {
assertContentEqualsEntity(idealTwo, response.getBody().get().get(1));
assertContentEqualsEntity(idealThree, response.getBody().get().get(2));
}

private void assertContentEqualsEntity(FeedbackSuggestionDTO ideal, FeedbackSuggestionDTO actualResponse) {
assertEquals(ideal.getReason(), actualResponse.getReason());
assertEquals(ideal.getId(), actualResponse.getId());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,6 @@ class FeedbackRequestControllerTest extends TestContainersSuite implements Membe
@Client("/services/feedback/requests")
HttpClient client;

@Property(name = FeedbackRequestServicesImpl.FEEDBACK_REQUEST_NOTIFICATION_SUBJECT)
String notificationSubject;

@Inject
CheckInsConfiguration checkInsConfiguration;

Expand Down Expand Up @@ -214,7 +211,7 @@ void testCreateFeedbackRequestSendsEmailNow() {
"SEND_EMAIL",
fromName,
pdlMemberProfile.getWorkEmail(),
notificationSubject,
checkInsConfiguration.getApplication().getFeedback().getRequestSubject(),
correctContent,
recipient.getWorkEmail()
),
Expand Down Expand Up @@ -1078,7 +1075,7 @@ void testFeedbackRequestEnableEditsSendsEmail() {
"SEND_EMAIL",
fromName,
pdl.getWorkEmail(),
notificationSubject,
checkInsConfiguration.getApplication().getFeedback().getRequestSubject(),
correctContent,
recipient.getWorkEmail()
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.any;

import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
Expand Down Expand Up @@ -66,7 +67,7 @@ void setUp() {
reviewPeriodRepository = Mockito.mock(ReviewPeriodRepository.class);

feedbackRequestServices = new FeedbackRequestServicesImpl(feedbackReqRepository, currentUserServices,
memberProfileServices, reviewPeriodRepository, emailSender, "DNC", checkInsConfiguration);
memberProfileServices, reviewPeriodRepository, emailSender, checkInsConfiguration);
}

@Test
Expand Down
3 changes: 3 additions & 0 deletions server/src/test/resources/application-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ flyway:
---
check-ins:
web-address: "https://checkins.objectcomputing.com"
application:
google-api:
delegated-user: test@objectcomputing.com
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added as the property is defined as coming from an env var in application.yml and this would otherwise fail under test

---
aes:
key: BOGUS_TEST_KEY
Expand Down