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.

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

Post a message to slack #90

Merged
merged 7 commits into from Jul 9, 2019

Conversation

Projects
None yet
3 participants
@AmanRaj1608
Copy link
Member

commented Jul 3, 2019

Send message to slack when someone gets invited

Fixes #89

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Non Functional Requirement

  • Follows the code style of this project.
  • Tests Cover Changes
  • I have performed a self-review of my own code
  • All new and existing tests passed.
  • Documentation
post message to slack
Send message to slack when someone gets invited #89
@yashLadha
Copy link
Member

left a comment

PTAL

server.js Outdated
})
.catch(error => {
console.log('FAILED: Send slack webhook', error)
// reject(new Error('FAILED: Send slack webhook'))

This comment has been minimized.

Copy link
@yashLadha

yashLadha Jul 3, 2019

Member

In case of failure, we can work on max_retry logic. Sometimes what happens is that when slack gets more load, it puts the requests in queue and sometimes those requests are also dropped. So to ensure correctness from our side we can rely on max-retry logic and can store the value in some constant.

This comment has been minimized.

Copy link
@AmanRaj1608

AmanRaj1608 Jul 3, 2019

Author Member

Thanks, I will read about this and fix this issue.

This comment has been minimized.

Copy link
@aashutoshrathi

aashutoshrathi Jul 3, 2019

Member

In case of failure, we can redirect error to another channel too, if we want to.

This comment has been minimized.

Copy link
@yashLadha

yashLadha Jul 4, 2019

Member

Yes, even after themax_retry we can send a single request to post error on a different slack channel. No need to use max_retry logic there because it won't benefit to waste resources on successfully sending of error 😉

This comment has been minimized.

Copy link
@AmanRaj1608

AmanRaj1608 Jul 4, 2019

Author Member

@yashLadha
I don't know how to do it using axios 😅
If you could send some reference , It will be very helpful

This comment has been minimized.

Copy link
@yashLadha

yashLadha Jul 4, 2019

Member

Try to look for the retry params, a quick google search will get you to the solution. If you are still not able to find out, ping me I will tell you.

Show resolved Hide resolved server.js
@aashutoshrathi
Copy link
Member

left a comment

I tested the code locally, worked fine.
here's output: https://iiitvadodara.slack.com/archives/CKP5MG4LA/p1562185719000100

Left a few comments.

Show resolved Hide resolved server.js
Show resolved Hide resolved server.js Outdated
Show resolved Hide resolved server.js Outdated
Show resolved Hide resolved server.js Outdated
server.js Outdated
})
.catch(error => {
console.log('FAILED: Send slack webhook', error)
// reject(new Error('FAILED: Send slack webhook'))

This comment has been minimized.

Copy link
@aashutoshrathi

aashutoshrathi Jul 3, 2019

Member

In case of failure, we can redirect error to another channel too, if we want to.

AmanRaj1608 added some commits Jul 5, 2019

minor fix
Added
1. dotenv
2. Change message
3. Fix link
@AmanRaj1608

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

@yashLadha
Sir I tried this logic which I found here axios/axios#164 (comment)

  axios.post(webhookURL, JSON.stringify(options),
    { retry: 5, retryDelay: 1000 })
    .then(response => {
      console.log('SUCCESS: Sent slack webhook:', response.data)
    })
    .catch(error => {
      console.log('FAILED: Sent slack webhook', error)
    })

I also find this amazing package https://www.npmjs.com/package/axios-retry .
This npm package is quite good

Which one should I use

npm
Axios plugin that intercepts failed requests and retries them whenever posible.
@aashutoshrathi
Copy link
Member

left a comment

LGTM, just add retry logic

@yashLadha

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

max_retry logic
Added max_retry logic to send request 3 times in case first failed.
@AmanRaj1608

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2019

@yashLadha
Please review 😀

@yashLadha
Copy link
Member

left a comment

Promises aren't synchronous. PTAL

Show resolved Hide resolved server.js Outdated
max_retry working ✔
Fixed asynchronous behaviour
@AmanRaj1608

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

@yashLadha
Sir it's working properly now please review again 😅. It will send message only one time if URL is correct and fails for 3 times if I enter incorrect URL.

Show resolved Hide resolved server.js Outdated
@yashLadha
Copy link
Member

left a comment

PTAL, else all looks good to me.

@AmanRaj1608

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

@yashLadha
Done 😀

@aashutoshrathi
Copy link
Member

left a comment

Why aren't we using async-await?

@aashutoshrathi

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

Also, can you once check the file that all variable names are camelCase.
Thanks

@AmanRaj1608

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

Why aren't we using async-await?

I tried using async-await but somehow it was not working properly. It was like

flag 0
flag 1
flag 2
SUCCESS: Sent slack webhook
SUCCESS: Sent slack webhook
SUCCESS: Sent slack webhook

I am not sure what I was doing wrong. So I ended up doing like this 😥.


Also, can you once check the file that all variable names are camelCase.
Thanks

Yes I followed code style as ES Lint yells if I don't follow it. And before pushing code I did used npm run lint:fix.

Show resolved Hide resolved server.js Outdated
@yashLadha
Copy link
Member

left a comment

LGTM

@aashutoshrathi aashutoshrathi merged commit 6dd403f into iiitv:master Jul 9, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.