-
-
Notifications
You must be signed in to change notification settings - Fork 188
Issue-1201 : Notify slack for the new issue/bounties #1338
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
Issue-1201 : Notify slack for the new issue/bounties #1338
Conversation
alexanmtz
left a comment
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.
It's doing great @devmaster-x 👌
Tested posting to the channel, and it's working well 🙏
Just need some small adjustment for the case of the issue imported is set to not_listed
I would recommend to add to the tasks tests a spy which checks if the Slack method is called, and it's not called in case for tasks with not_listed set.
| if (userData.receiveNotifications) { | ||
| TaskMail.new(userData, taskData) | ||
| } | ||
| issueAddedComment(task) |
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.
We should check here before calling issueAddedComment whether the issue is not_listed or private; in that case, it shouldn't be added to the channel.
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.
Updated code to check not_listed and private issues before calling issueAddedComment().
const isTaskPublic = !(taskData.not_listed === true || taskData.private === true)
if (isTaskPublic) {
issueAddedComment(task)
notifyNewIssue(taskData, userData)
}
src/modules/orders/orderBuilds.js
Outdated
| const percentage = orderCreated.Plan?.feePercentage | ||
|
|
||
| // Notify Slack about new bounty | ||
| if (orderCreated.Task && orderCreated.User) { |
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.
Don't post on the channel if the related task is set to not_listed or private
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.
Updated code to check not_listed and private issues before notify about new bounty.
const shouldNotifySlack =
orderCreated.Task &&
orderCreated.User &&
!(
orderCreated.Task.dataValues.not_listed === true ||
orderCreated.Task.dataValues.private === true
)
if (shouldNotifySlack) {
notifyNewBounty(
orderCreated.Task.dataValues,
orderCreated.dataValues,
orderCreated.User.dataValues
)
}
e1d86a3 to
4b1448e
Compare
alexanmtz
left a comment
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.
Hey @devmaster-x, some more improvements are needed to have everything in place to merge.
| .then(async (updatedOrder) => { | ||
| if (orderParameters.plan === 'full') { | ||
| PaymentMail.support(user, task, order) | ||
| } | ||
| PaymentMail.success(user, task, order.amount) | ||
|
|
||
| // Send Slack notification for new bounty payment | ||
| if (orderPayload.paid && orderPayload.status === 'succeeded') { | ||
| const orderData = { | ||
| amount: order.amount || orderParameters.amount, | ||
| currency: order.currency || orderParameters.currency || 'USD' | ||
| } | ||
| notifyNewBounty(task.dataValues, orderData, user).catch((e) => { | ||
| console.log('error on send slack notification for new bounty', e) | ||
| }) | ||
| } | ||
|
|
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.
@devmaster-x because you introduced async it should be something like this?
| .then(async (updatedOrder) => { | |
| if (orderParameters.plan === 'full') { | |
| PaymentMail.support(user, task, order) | |
| } | |
| PaymentMail.success(user, task, order.amount) | |
| // Send Slack notification for new bounty payment | |
| if (orderPayload.paid && orderPayload.status === 'succeeded') { | |
| const orderData = { | |
| amount: order.amount || orderParameters.amount, | |
| currency: order.currency || orderParameters.currency || 'USD' | |
| } | |
| notifyNewBounty(task.dataValues, orderData, user).catch((e) => { | |
| console.log('error on send slack notification for new bounty', e) | |
| }) | |
| } | |
| .then(async (updatedOrder) => { | |
| if (orderParameters.plan === 'full') { | |
| PaymentMail.support(user, task, order) | |
| } | |
| PaymentMail.success(user, task, order.amount) | |
| // Send Slack notification for new bounty payment | |
| if (orderPayload.paid && orderPayload.status === 'succeeded') { | |
| const orderData = { | |
| amount: order.amount || orderParameters.amount, | |
| currency: order.currency || orderParameters.currency || 'USD' | |
| } | |
| try { | |
| await notifyNewBounty(task.dataValues, orderData, user) | |
| } catch (e) { | |
| console.log('error on send slack notification for new bounty', e) | |
| } | |
| } | |
test/task.test.js
Outdated
| it('should not call Slack methods when task is created with not_listed set to true', async () => { | ||
| chai.use(spies) | ||
| const slackModule = require('../src/modules/slack') | ||
| const slackSpy = chai.spy.on(slackModule, 'notifyNewIssue') | ||
| const originalBotModule = require('../src/modules/bot/issueAddedComment') | ||
| const botSpy = chai.spy(originalBotModule) | ||
|
|
||
| try { | ||
| const user = await registerAndLogin(agent) | ||
| const task = await models.Task.create({ | ||
| url: 'https://github.com/worknenjoy/gitpay/issues/999', | ||
| provider: 'github', | ||
| userId: user.body.id, | ||
| title: 'Test Task', | ||
| not_listed: true | ||
| }) | ||
|
|
||
| const userData = await task.getUser() | ||
| const taskData = task.dataValues | ||
|
|
||
| // Test the actual logic from taskBuilds.js | ||
| const isTaskPublic = !(taskData.not_listed === true || taskData.private === true) | ||
| if (isTaskPublic) { | ||
| botSpy(task) | ||
| slackModule.notifyNewIssue(taskData, userData) | ||
| } | ||
|
|
||
| expect(slackSpy).to.not.have.been.called() | ||
| expect(botSpy).to.not.have.been.called() | ||
| } finally { | ||
| chai.spy.restore(slackModule, 'notifyNewIssue') | ||
| } | ||
| }) | ||
|
|
||
| it('should not call Slack methods when task is created with private set to true', async () => { | ||
| chai.use(spies) | ||
| const slackModule = require('../src/modules/slack') | ||
| const slackSpy = chai.spy.on(slackModule, 'notifyNewIssue') | ||
| const originalBotModule = require('../src/modules/bot/issueAddedComment') | ||
| const botSpy = chai.spy(originalBotModule) | ||
|
|
||
| try { | ||
| const user = await registerAndLogin(agent) | ||
| const task = await models.Task.create({ | ||
| url: 'https://github.com/worknenjoy/gitpay/issues/998', | ||
| provider: 'github', | ||
| userId: user.body.id, | ||
| title: 'Test Task', | ||
| private: true | ||
| }) | ||
|
|
||
| const userData = await task.getUser() | ||
| const taskData = task.dataValues | ||
|
|
||
| // Test the actual logic from taskBuilds.js | ||
| const isTaskPublic = !(taskData.not_listed === true || taskData.private === true) | ||
| if (isTaskPublic) { | ||
| botSpy(task) | ||
| slackModule.notifyNewIssue(taskData, userData) | ||
| } | ||
|
|
||
| expect(slackSpy).to.not.have.been.called() | ||
| expect(botSpy).to.not.have.been.called() | ||
| } finally { | ||
| chai.spy.restore(slackModule, 'notifyNewIssue') | ||
| } | ||
| }) | ||
|
|
||
| it('should call Slack methods when task is created as public', async () => { | ||
| chai.use(spies) | ||
|
|
||
| // Set up spies first | ||
| const slackModule = require('../src/modules/slack') | ||
| const slackSpy = chai.spy.on(slackModule, 'notifyNewIssue') | ||
|
|
||
| // For default export function, create a spy wrapper | ||
| delete require.cache[require.resolve('../src/modules/bot/issueAddedComment')] | ||
| const originalBotModule = require('../src/modules/bot/issueAddedComment') | ||
| const botSpy = chai.spy(originalBotModule) | ||
|
|
||
| try { | ||
| const user = await registerAndLogin(agent) | ||
| // Create task without explicitly setting not_listed/private (should default to false) | ||
| const task = await models.Task.create({ | ||
| url: 'https://github.com/worknenjoy/gitpay/issues/997', | ||
| provider: 'github', | ||
| userId: user.body.id, | ||
| title: 'Test Task' | ||
| }) | ||
|
|
||
| const userData = await task.getUser() | ||
| const taskData = task.dataValues | ||
|
|
||
| // Test the actual logic from taskBuilds.js | ||
| const isTaskPublic = !(taskData.not_listed === true || taskData.private === true) | ||
| if (isTaskPublic) { | ||
| // Use the spied version | ||
| botSpy(task) | ||
| slackModule.notifyNewIssue(taskData, userData) | ||
| } | ||
|
|
||
| expect(slackSpy).to.have.been.called() | ||
| expect(botSpy).to.have.been.called() | ||
| } finally { | ||
| chai.spy.restore(slackModule, 'notifyNewIssue') | ||
| // Restore cache | ||
| delete require.cache[require.resolve('../src/modules/slack')] | ||
| delete require.cache[require.resolve('../src/modules/bot/issueAddedComment')] | ||
| } | ||
| }) | ||
|
|
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.
I guess here is not the right approach to test the integration. This should be more like an API testing, check other tests in the file, like calling the agent to the endpoint to create a new task and check if the Slack is called, and instead of these spies, there's a good example of an approach using Stubs on userAuth.test:242, so you can check if your integration is called and the expected params
test/task.test.js
Outdated
| const { notifyNewIssue } = require('../src/modules/slack') | ||
| const issueAddedComment = require('../src/modules/bot/issueAddedComment') |
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.
I guess this is not used here, right? As you moved to another test file.
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.
Let me check.
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.
Yes, you are right, I removed unused imports.
…x/gitpay into feat/post-slack-notifications
alexanmtz
left a comment
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.
Thanks @devmaster-x , testing the functionality it's working in all cases.
It was a good refactoring to create a full module for Slack integration.
We just need to fine tunning your latest changes as requested here, plus we need to address this before we can merge:
-
There's too many comments, you can see the application don't have in general this guideline to comment all, the code and tests should speak for itself
-
We need to move the whole module to './shared', because this is a shared module and not an specific entity
-
We need to only have in this PR the changes related to this PR. i can see you did a lot of improvements, and we can have it in another PR, so we can test without break anything.
for the last item, you can do this:
- To not loose this changes and use for another PR, you can run
git branch refactor/fixes-on-user-role-and-paymentsto create a new branch and keep safe all your changes to create another PR later on - Run this command to no related files that has refactoring not related to this issue (the ones related on user roles, payment components and migration files):
git checkout master -- /path/of/file - Commit the changes and push
When we have this PR merged you can get back to the first branch, and getting the changes for master the only changes in the new PR will be the one left no related to this issue.
I hope is clear and you can always reach out to us on Slack ;)
…x/gitpay into feat/post-slack-notifications
alexanmtz
left a comment
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.
Thanks for all your patience going through this @devmaster-x. As we agreed, I cleaned up your PR to remove all unrelated changes, and I guess we are now good to go. There's still some improvements missing, but let's address this in another PR:
- The invoice payment is not working for some reason, and I removed the file changes from the PR to investigate later on
- We need the url on the Slack notification, so the user can go to the issue page
- I have to remove the slack notificaiton on the webhooks, because this was causing multiple Slack notifications (this can be addressed later, when we have a more centralized logic and source of truth when state changes for entities)
We are moving our architecture into services to make integrations like this easier, so let's first get the basics working.
Related Issues
close #1201
What is updated
Notify about new issues/bounties to the slack channel using webhook.
Screentshot
Test