Feature/issue 82 add slack service#207
Conversation
|
Omar Orphy seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Hi @haiphucnguyen, so regarding your comments from PR #203, I decided it makes sense to create a new PR addressing only the addition of the Slack Service by itself. If you would like starting this PR to add what you had mentioned before regarding Service Provider Interfaces, we can also do that. |
|
Nice work on this implementation! 🎉 @Norphy . I leave some comments for your consideration. Great progress — keep it up! Let me know if you want help updating this, happy to pair on it. 👍 |
apps/backend/commons/src/main/java/io/flowinquiry/config/FlowInquiryProperties.java
Outdated
Show resolved
Hide resolved
apps/backend/server/src/test/java/io/flowinquiry/modules/collab/service/SlackServiceIT.java
Outdated
Show resolved
Hide resolved
apps/backend/server/src/test/java/io/flowinquiry/modules/collab/service/SlackServiceIT.java
Outdated
Show resolved
Hide resolved
apps/backend/commons/src/main/java/io/flowinquiry/modules/collab/domain/SlackMessage.java
Show resolved
Hide resolved
apps/backend/commons/src/main/java/io/flowinquiry/modules/collab/service/SlackService.java
Show resolved
Hide resolved
apps/backend/server/src/test/java/io/flowinquiry/modules/collab/service/SlackServiceIT.java
Outdated
Show resolved
Hide resolved
apps/backend/server/src/test/java/io/flowinquiry/modules/collab/service/SlackServiceIT.java
Outdated
Show resolved
Hide resolved
apps/backend/server/src/test/java/io/flowinquiry/modules/collab/service/SlackServiceIT.java
Outdated
Show resolved
Hide resolved
apps/backend/server/src/test/java/io/flowinquiry/modules/collab/service/SlackServiceIT.java
Outdated
Show resolved
Hide resolved
apps/backend/commons/src/main/java/io/flowinquiry/modules/collab/service/SlackService.java
Show resolved
Hide resolved
Signed-off-by: Omar Orphy <[email protected]>
haiphucnguyen
left a comment
haiphucnguyen
left a comment
There was a problem hiding this comment.
Thanks, @Norphy ! I’ve left a few comments—once those are addressed, we’ll be all set.
apps/backend/commons/src/main/java/io/flowinquiry/modules/collab/service/SlackService.java
Show resolved
Hide resolved
apps/backend/commons/src/main/java/io/flowinquiry/modules/collab/service/SlackService.java
Show resolved
Hide resolved
apps/backend/commons/src/main/java/io/flowinquiry/modules/collab/domain/SlackMessage.java
Show resolved
Hide resolved
Signed-off-by: Omar Orphy <[email protected]>
|
@Norphy Your PR looks great overall! A couple of files just need formatting. Could you install our Git hooks (see https://docs.flowinquiry.io/developer_guides#git-hooks)? Once set up, make any small commit—pre-commit will auto-format everything—then push the updated commit and you’ll be all set. |
|
Hi @haiphucnguyen, is there anything you would like updated for this PR? |
haiphucnguyen
left a comment
haiphucnguyen
left a comment
There was a problem hiding this comment.
LGTM. Thank you @Norphy
apps/backend/server/src/test/java/io/flowinquiry/modules/collab/service/SlackServiceIT.java
Show resolved
Hide resolved
Looks good—thank you, @Norphy! I’ve been having some issues receiving GitHub notifications, so I didn’t get alerted about your changes. In the future, once you're done, feel free to request a re-review . That would really help ensure I know the PR is ready for another look. 
|
|
Thanks for the approval! Ah, I will keep that in mind in the future. Thanks again! |
3 participants
Description
Add Slack Service to be used as notifications provider. This PR addresses issue 82
Changes Made
Slack service added
Unit Tests for Slack service