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

Added app.json, to allow for one click installation in Heroku #9

Open
wants to merge 2 commits into
base: latest
Choose a base branch
from

Conversation

dvidsilva
Copy link

Aded slack event listener for team_join
Added information to README about team_join config vars

Aded slack event listener for team_join
Added information to README about team_join config vars
Copy link
Member

@zkat zkat left a comment

Choose a reason for hiding this comment

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

Great work so far! I made a few comments for stuff that should probably get addressed before being merged in. Thanks a lot for all this!

require('./events/team_join')(token, data, e => {
console.log('sent welcome message')
})
})
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd prefer this even be tracked through the Events API -- I've enabled the team_join event on both slack apps, so you should be able to add an entry to lib/events

Copy link
Member

Choose a reason for hiding this comment

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

So basically, in order to switch, you just need to remove this chunk of code -- lib/events will be called automatically by the events API.

Copy link
Author

Choose a reason for hiding this comment

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

Oh that event wasn't working and I wasn't sure if it was deprecated or something or inactive. Thanks for activating I will change that.

Copy link
Member

Choose a reason for hiding this comment

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

Let me know if it still doesn't work -- it might need a new permission scope which would be a massive pita

Copy link
Author

Choose a reason for hiding this comment

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

oh ok :/ yep that's why I opted for this quick and dirty solution, but the proper way is better. I'll have time again tonight and will let you know.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome! 👍👍👍 Take your time.💚


const cocTxt = process.env.COC_URL ? `<${process.env.COC_URL}|Code of Conduct>` : 'Code of Conduct'
const welcome_message_text = process.env.WELCOME_MESSAGE_TEXT || `Welcome! Review the ${cocTxt} before participating in the community.`
const send_welcome_message = process.env.SEND_WELCOME_MESSAGE === 'FALSE' ? false : true
Copy link
Member

Choose a reason for hiding this comment

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

We need to figure out a way to make this configurable per-Slack, by Admins. I've been considering adding an admin-only /config command. How do you feel about this part of your patch being left unused until it can be configured? Different Slacks are going to have completely different needs as far as their welcome messages go, too. >_<

Copy link
Author

Choose a reason for hiding this comment

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

Yep, I realized that later, this would be global config for everyone? how are you currently managing configurations per team, I can change it so it uses that.

Copy link
Member

Choose a reason for hiding this comment

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

We don't have a way to do that right now, which kinda sucks but was in my mental to-do list for Soon™.

You did the bulk of the work for this feature too so what I meant is that we could merge this with default messages, and then customize it once the feature is up. I'm still not sure what the best way to do that is.

const send_welcome_message = process.env.SEND_WELCOME_MESSAGE === 'FALSE' ? false : true

module.exports.command = 'join-private [channel]'
module.exports.desc = 'Invites user to the given channel, or lists them.'
Copy link
Member

Choose a reason for hiding this comment

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

This seems like you copy-pasted the command and left the command spec stuff in place?

Copy link
Author

Choose a reason for hiding this comment

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

yep. my bad

module.exports.command = 'join-private [channel]'
module.exports.desc = 'Invites user to the given channel, or lists them.'

module.exports = (token, data, cb) => {
Copy link
Member

Choose a reason for hiding this comment

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

If you switch to the event API fully (you don't need to move this file, it's fine where it is!), you will only get (data, cb) as arguments.

console.log('new user joined team: ', data)
cb()
console.log(send_welcome_message, welcome_message_text)
Copy link
Member

Choose a reason for hiding this comment

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

dangling console.log

cb()
console.log(send_welcome_message, welcome_message_text)
if (!send_welcome_message || !welcome_message_text) {
return (cb ? cb() : undefined)
Copy link
Member

Choose a reason for hiding this comment

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

cb() always needs to be called. It will always be passed in by the events handler.

channel: data.user.id,
text: welcome_message_text
}).then(() => {
return (cb ? cb() : undefined)
Copy link
Member

Choose a reason for hiding this comment

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

see above comment about cb always being available.

"description": ""
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

How do you feel about filling in these descriptions?

Copy link
Author

Choose a reason for hiding this comment

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

I don't know what exactly goes there, would you mind helping with that text, I don't wanna confuse the users.

Copy link
Author

Choose a reason for hiding this comment

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

I can separate the things into two different PRs too, they're not really the same feature.

Copy link
Member

Choose a reason for hiding this comment

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

They're basically application-level tokens. This isn't really a blocker either so don't worry about it. They're used by Slack to identify and authenticate applications, and there verification token is for authenticating individual requests -- these are simply things you grab from slack

@dvidsilva
Copy link
Author

@zkat added the changes so that it uses the events file and api. Not 100% sure that it works but if the authorizations are made it should work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants