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

Jump to bottom

refactor: SlackClient 로직 개선 및 로깅 추가 #637

Merged
merged 6 commits into from
Oct 20, 2022
Merged

refactor: SlackClient 로직 개선 및 로깅 추가 #637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9b17e05
feat: 슬랙 API 예외 처리 중복 코드 제거
yeon-06 Oct 19, 2022
437ba96
feat: 슬랙 API 예외 로깅
yeon-06 Oct 19, 2022
b0fc135
refactor: request 값 로깅
yeon-06 Oct 19, 2022
cc4def9
refactor: 누락된 로직 재생성
yeon-06 Oct 19, 2022
c280bd8
test: builder 대신 DTO를 이용해 모킹이 제대로 되지 않던 현상 개선
yeon-06 Oct 19, 2022
d1d7c95
Merge branch 'develop' of https://github.com/woowacourse-teams/2022-p…
yeon-06 Oct 19, 2022
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
116 changes: 59 additions & 57 deletions backend/src/main/java/com/pickpick/support/SlackClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@
import com.pickpick.workspace.domain.Workspace;
import com.slack.api.methods.MethodsClient;
import com.slack.api.methods.SlackApiException;
import com.slack.api.methods.SlackApiRequest;
import com.slack.api.methods.SlackApiTextResponse;
import com.slack.api.methods.request.chat.ChatPostMessageRequest;
import com.slack.api.methods.request.conversations.ConversationsInviteRequest;
import com.slack.api.methods.request.conversations.ConversationsListRequest;
import com.slack.api.methods.request.oauth.OAuthV2AccessRequest;
import com.slack.api.methods.request.users.UsersIdentityRequest;
import com.slack.api.methods.response.chat.ChatPostMessageResponse;
import com.slack.api.methods.request.users.UsersListRequest;
import com.slack.api.methods.response.conversations.ConversationsInviteResponse;
import com.slack.api.methods.response.conversations.ConversationsListResponse;
import com.slack.api.methods.response.oauth.OAuthV2AccessResponse;
Expand All @@ -28,15 +30,17 @@
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import lombok.extern.slf4j.Slf4j;
import org.springframework.stereotype.Component;

@Slf4j
@Component
public class SlackClient implements ExternalClient {

private static final String LOGGING_INFO = "[Request] Slack Api Request - {}";
private static final String OAUTH_ACCESS_METHOD_NAME = "oauthV2Access";
private static final String USERS_IDENTITY_METHOD_NAME = "usersIdentity";
private static final String USER_LIST_METHOD_NAME = "usersList";
private static final String CHANNEL_INFO_METHOD_NAME = "conversationsInfo";
private static final String CHANNEL_LIST_METHOD_NAME = "conversationsList";
private static final String CHANNEL_INVITE_METHOD_NAME = "conversationsInvite";
private static final String CHAT_POST_METHOD_NAME = "chatPostMessage";
Expand All @@ -55,33 +59,20 @@ public SlackClient(final SlackProperties slackProperties, final MethodsClient me

@Override
public String callUserToken(final String code) {
String loginRedirectUrl = slackProperties.getLoginRedirectUrl();
OAuthV2AccessResponse response = callOAuth2(code, loginRedirectUrl);
validateResponse(OAUTH_ACCESS_METHOD_NAME, response);
OAuthV2AccessResponse response = callOAuth2(code, slackProperties.getLoginRedirectUrl());
return response.getAuthedUser().getAccessToken();
}

@Override
public WorkspaceInfoDto callWorkspaceInfo(final String code) {
String workspaceRedirectUrl = slackProperties.getWorkspaceRedirectUrl();
OAuthV2AccessResponse response = callOAuth2(code, workspaceRedirectUrl);

OAuthV2AccessResponse response = callOAuth2(code, slackProperties.getWorkspaceRedirectUrl());
return new WorkspaceInfoDto(response.getTeam().getId(), response.getAccessToken(), response.getBotUserId(),
response.getAuthedUser().getAccessToken());
}

private OAuthV2AccessResponse callOAuth2(final String code, final String redirectUrl) {
OAuthV2AccessRequest request = generateOAuthRequest(code, redirectUrl);

try {
OAuthV2AccessResponse response = methodsClient
.oauthV2Access(request);
validateResponse(OAUTH_ACCESS_METHOD_NAME, response);
return response;

} catch (IOException | SlackApiException e) {
throw new SlackApiCallException(OAUTH_ACCESS_METHOD_NAME);
}
return execute(methodsClient::oauthV2Access, OAUTH_ACCESS_METHOD_NAME, request);
Comment on lines -76 to +75
Copy link
Collaborator

Choose a reason for hiding this comment

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

try~catch가 어디론가 옮겨졌다 생각하고 음 반복 로직을 더 깔끔하게 못만드나 했는데 이미 멋지게 중복을 없애두셨군요.. 🥺!!!!!

}

private OAuthV2AccessRequest generateOAuthRequest(final String code, final String redirectUrl) {
Expand All @@ -99,24 +90,26 @@ public String callMemberSlackId(final String accessToken) {
.token(accessToken)
.build();

try {
UsersIdentityResponse response = methodsClient.usersIdentity(request);
validateResponse(USERS_IDENTITY_METHOD_NAME, response);
return response.getUser().getId();
} catch (IOException | SlackApiException e) {
throw new SlackApiCallException(USERS_IDENTITY_METHOD_NAME);
}
UsersIdentityResponse response = execute(
methodsClient::usersIdentity,
USERS_IDENTITY_METHOD_NAME,
request);

return response.getUser().getId();
}

@Override
public List<Member> findMembersByWorkspace(final Workspace workspace) {
try {
UsersListResponse response = methodsClient.usersList(request -> request.token(workspace.getBotToken()));
validateResponse(USER_LIST_METHOD_NAME, response);
return toMembers(response.getMembers(), workspace);
} catch (IOException | SlackApiException e) {
throw new SlackApiCallException(USER_LIST_METHOD_NAME);
}
UsersListRequest request = UsersListRequest.builder()
.token(workspace.getBotToken())
.build();

UsersListResponse response = execute(
methodsClient::usersList,
USER_LIST_METHOD_NAME,
request);

return toMembers(response.getMembers(), workspace);
}

private List<Member> toMembers(final List<User> users, final Workspace workspace) {
Expand All @@ -141,14 +134,8 @@ private Member toMember(final User user, final Workspace workspace) {

@Override
public List<Channel> findChannelsByWorkspace(final Workspace workspace) {
try {
ConversationsListResponse response = methodsClient.conversationsList(
request -> request.token(workspace.getBotToken()));
validateResponse(CHANNEL_LIST_METHOD_NAME, response);
return toChannels(response.getChannels(), workspace);
} catch (IOException | SlackApiException e) {
throw new SlackApiCallException(CHANNEL_LIST_METHOD_NAME);
}
ConversationsListResponse response = findChannels(workspace.getBotToken());
return toChannels(response.getChannels(), workspace);
}

private List<Channel> toChannels(final List<Conversation> channels, final Workspace workspace) {
Expand All @@ -163,20 +150,24 @@ private Channel toChannel(final Conversation channel, final Workspace workspace)

@Override
public Participation findChannelParticipation(final String userToken) {
try {
ConversationsListResponse response = methodsClient.conversationsList(
request -> request.token(userToken));
validateResponse(CHANNEL_LIST_METHOD_NAME, response);
ConversationsListResponse response = findChannels(userToken);

Map<String, Boolean> participation = response.getChannels()
.stream()
.collect(Collectors.toMap(Conversation::getId, Conversation::isMember));
Map<String, Boolean> participation = response.getChannels()
.stream()
.collect(Collectors.toMap(Conversation::getId, Conversation::isMember));

return new Participation(participation);
return new Participation(participation);
}

} catch (IOException | SlackApiException e) {
throw new SlackApiCallException(CHANNEL_LIST_METHOD_NAME);
}
private ConversationsListResponse findChannels(String accessToken) {
ConversationsListRequest request = ConversationsListRequest.builder()
.token(accessToken)
.build();

return execute(
methodsClient::conversationsList,
CHANNEL_LIST_METHOD_NAME,
request);
}

@Override
Expand All @@ -188,12 +179,7 @@ public void sendMessage(final Reminder reminder) {
.token(member.getWorkspace().getBotToken())
.build();

try {
ChatPostMessageResponse response = methodsClient.chatPostMessage(request);
validateResponse(CHAT_POST_METHOD_NAME, response);
} catch (IOException | SlackApiException e) {
throw new SlackApiCallException(CHAT_POST_METHOD_NAME);
}
execute(methodsClient::chatPostMessage, CHAT_POST_METHOD_NAME, request);
}

@Override
Expand All @@ -203,6 +189,7 @@ public void inviteBotToChannel(final Member member, final Channel channel) {
.token(member.getToken())
.users(List.of(member.getWorkspace().getBotSlackId()))
.build();
log.info(LOGGING_INFO, request);

try {
ConversationsInviteResponse response = methodsClient.conversationsInvite(request);
Copy link
Collaborator Author

@yeon-06 yeon-06 Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

inviteBotToChannel() 메서드는 중간에 isBotAlreadyInChannel()이라는 메서드를 실행하면서 다른 메서드들과는 다른 로직을 갖고 있기 때문에 execute()를 사용하지 않았습니다.

Expand All @@ -211,10 +198,25 @@ public void inviteBotToChannel(final Member member, final Channel channel) {
}
validateResponse(CHANNEL_INVITE_METHOD_NAME, response);
} catch (IOException | SlackApiException e) {
log.error(CHANNEL_INVITE_METHOD_NAME, e);
throw new SlackApiCallException(CHANNEL_INVITE_METHOD_NAME);
}
}

private <T extends SlackApiTextResponse, K extends SlackApiRequest> T execute(
final SlackFunction<T, K> slackFunction, final String methodName, final K request) {
log.info(LOGGING_INFO, request);

try {
T result = slackFunction.execute(request);
validateResponse(methodName, result);
return result;
} catch (IOException | SlackApiException e) {
log.error(methodName, e);
throw new SlackApiCallException(methodName);
}
}

private boolean isBotAlreadyInChannel(final ConversationsInviteResponse response) {
return !response.isOk() && "already_in_channel".equals(response.getError());
}
Expand Down
12 changes: 12 additions & 0 deletions backend/src/main/java/com/pickpick/support/SlackFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.pickpick.support;

import com.slack.api.methods.SlackApiException;
import com.slack.api.methods.SlackApiRequest;
import com.slack.api.methods.SlackApiTextResponse;
import java.io.IOException;

@FunctionalInterface
public interface SlackFunction<T extends SlackApiTextResponse, K extends SlackApiRequest> {

T execute(K request) throws IOException, SlackApiException;
}
9 changes: 4 additions & 5 deletions backend/src/test/java/com/pickpick/auth/application/AuthServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,12 @@
import com.pickpick.support.DatabaseCleaner;
import com.pickpick.workspace.domain.Workspace;
import com.pickpick.workspace.domain.WorkspaceRepository;
import com.slack.api.RequestConfigurator;
import com.slack.api.methods.MethodsClient;
import com.slack.api.methods.SlackApiException;
import com.slack.api.methods.request.conversations.ConversationsListRequest.ConversationsListRequestBuilder;
import com.slack.api.methods.request.conversations.ConversationsListRequest;
import com.slack.api.methods.request.oauth.OAuthV2AccessRequest;
import com.slack.api.methods.request.users.UsersIdentityRequest;
import com.slack.api.methods.request.users.UsersListRequest.UsersListRequestBuilder;
import com.slack.api.methods.request.users.UsersListRequest;
import com.slack.api.methods.response.conversations.ConversationsListResponse;
import com.slack.api.methods.response.oauth.OAuthV2AccessResponse;
import com.slack.api.methods.response.oauth.OAuthV2AccessResponse.AuthedUser;
Expand Down Expand Up @@ -126,9 +125,9 @@ void registerWorkspace() throws SlackApiException, IOException {

given(slackClient.oauthV2Access(any(OAuthV2AccessRequest.class)))
.willReturn(generateOAuthV2AccessResponse(workspaceSlackId));
given(slackClient.conversationsList((RequestConfigurator<ConversationsListRequestBuilder>) any()))
given(slackClient.conversationsList(any(ConversationsListRequest.class)))
.willReturn(generateConversationsListResponse());
given(slackClient.usersList((RequestConfigurator<UsersListRequestBuilder>) any()))
given(slackClient.usersList(any(UsersListRequest.class)))
Comment on lines -129 to +130
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기도 깔끔하게 정리됐군요..!

.willReturn(generateUsersListResponse(memberSlackId));
given(slackClient.usersIdentity(any(UsersIdentityRequest.class)))
.willReturn(generateUsersIdentityResponse(memberSlackId));
Expand Down
7 changes: 3 additions & 4 deletions backend/src/test/java/com/pickpick/channel/application/ChannelServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,9 @@
import com.pickpick.support.DatabaseCleaner;
import com.pickpick.workspace.domain.Workspace;
import com.pickpick.workspace.domain.WorkspaceRepository;
import com.slack.api.RequestConfigurator;
import com.slack.api.methods.MethodsClient;
import com.slack.api.methods.SlackApiException;
import com.slack.api.methods.request.conversations.ConversationsListRequest.ConversationsListRequestBuilder;
import com.slack.api.methods.request.conversations.ConversationsListRequest;
import com.slack.api.methods.response.conversations.ConversationsListResponse;
import com.slack.api.model.Conversation;
import java.io.IOException;
Expand Down Expand Up @@ -79,7 +78,7 @@ void findAll() throws SlackApiException, IOException {

channelSubscriptions.save(new ChannelSubscription(freeChat, yeonLog, 1));

given(methodsClient.conversationsList((RequestConfigurator<ConversationsListRequestBuilder>) any()))
given(methodsClient.conversationsList(any(ConversationsListRequest.class)))
.willReturn(generateConversationsListResponse(notice, freeChat, qna));

// when
Expand Down Expand Up @@ -107,7 +106,7 @@ void findChannelsHasUser() throws SlackApiException, IOException {

channelSubscriptions.save(new ChannelSubscription(freeChat, yeonLog, 1));

given(methodsClient.conversationsList((RequestConfigurator<ConversationsListRequestBuilder>) any()))
given(methodsClient.conversationsList(any(ConversationsListRequest.class)))
.willReturn(generateConversationsListResponse(notice));

// when
Expand Down