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

Improvements #9

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Improvements #9

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
8274bc3
Change birthdays database to YAML / JSON
shinenelson May 28, 2017
17a3ea0
Configurize db_path for the birthdays database
shinenelson May 28, 2017
726d61a
Rename configurations.json to configuration.json.example
shinenelson May 28, 2017
c45e1e1
Optimize bot to work with {hash}ed birthdays
shinenelson May 28, 2017
9f7bc88
Configurize mention'ing team members
shinenelson Jun 4, 2017
dfbcc78
Update README.md with more detailed instructions
shinenelson Jun 4, 2017
d409122
Rename db_path to birthdays_path (Resolves https://github.com/jeKnowl…
shinenelson Jun 4, 2017
077a104
Log Time.now
shinenelson Jun 4, 2017
2ee35b3
change date keys to string
shinenelson Aug 1, 2017
9b83de9
handle empty / nil YAML.load_file
shinenelson Aug 1, 2017
ed5c9cb
remove spaces (Resolves https://github.com/jeKnowledge/slack-birthday…
shinenelson Aug 3, 2017
e901b71
move birthday list filter to birthday_bot from birthday_reader
shinenelson Aug 7, 2017
f4dbfe8
add today = Time.now (Resolves https://github.com/jeKnowledge/slack-b…
shinenelson Aug 7, 2017
e3a0763
def build_user_list
shinenelson Aug 7, 2017
56c8b07
[refactor] remove httparty dependency
shinenelson Nov 21, 2019
e64afd9
[docs] replace db_path with birthdays_path
shinenelson Nov 21, 2019
0596516
[readme] fix URL in contributors' list
shinenelson Nov 21, 2019
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
1 change: 1 addition & 0 deletions configurations.json.example
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{
"db_path": "birthdays.yaml",
"mention": true,
"slack_url": "https://hooks.slack.com/...",
"channel_name": "general",
"greeting_message": "Happy birthday !",
Expand Down
17 changes: 13 additions & 4 deletions lib/birthday_bot.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,16 @@ def start!

puts "Checking who was born today"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should keep Time.now in the log

Copy link
Author

Choose a reason for hiding this comment

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

Ah, you wanted it in the log. The way I handle my crons are, I prepend date before my executing command. But I don't know if this works with Heroku's scheduler (does it?). Else I'll just put the date back in the log.

unless birthdays.nil?
users = "<@#{ birthdays[0] }>"
users = "#{ mention birthdays[0] }"
if birthdays.count > 1
puts "#{ birthdays.count } people were born today"
if birthdays.count == 2
users = " <@#{ birthdays[0] }> and <@#{ birthdays[1] }> "
users = " #{ mention birthdays[0] } and #{ mention birthdays[1] } "
else
for i in 1..birthdays.count-2
users += ", <@#{ birthdays[i] }>"
users.concat( ", #{ mention birthdays[i] }" )
end
users += " and <@#{ birthdays[i+1] }> "
users.concat( " and #{ mention birthdays[i+1] } " )
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can simplify this:

puts "#{birthdays.count} were born today"

today = Time.now
birthdays_today = birthdays[today.month][today.day]

unless birthdays_today.empty?
  users = build_user_list(birthdays_today)
  message = "#{users} #{@config.greeting_message}"
  HTTParty.post(@config.slack_url, body: { channel: @config.channel_name,
    username: @config.bot_name,
    text: message,
    icon_emoji: @config.bot_emoji }.to_json)
end
 
# ...

def build_user_list(birthdays)
  if birthdays.size == 1
    birthdays.first
  else
    users = birthdays.take(birthdays.count - 1).join(", ")
    users += " and #{birthdays[birthdays.count - 1]}" if birthdays.count > 1
  end
end

What do you think ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@shinenelson Did you take a look at this ?

(mentions are missing here)

message = "#{users} #{@config.greeting_message}"
Expand All @@ -35,4 +35,13 @@ def start!
puts "Today is a day that no one was born"
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you looked at my previous comment with a suggestion to refactor this function ?

puts "#{birthdays.count} were born today"

today = Time.now
birthdays_today = birthdays[today.month][today.day]

unless birthdays_today.empty?
  users = build_user_list(birthdays_today)
  message = "#{users} #{@config.greeting_message}"
  HTTParty.post(@config.slack_url, body: { channel: @config.channel_name,
    username: @config.bot_name,
    text: message,
    icon_emoji: @config.bot_emoji }.to_json)
end
 
# ...

def build_user_list(birthdays)
  if birthdays.size == 1
    birthdays.first
  else
    users = birthdays.take(birthdays.count - 1).join(", ")
    users += " and #{birthdays[birthdays.count - 1]}" if birthdays.count > 1
  end
end

end

def mention ( name )
if @config.mention
"<@#{ name }>"
else
name
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you give an example where disabling mentions is useful ?

Copy link
Author

Choose a reason for hiding this comment

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

The earlier version of the database stored the real names of users. The bot wished users with their real names. So, I thought that could be a valid use-case

But like my commit message said, some people might prefer wishing their team-mates by their real names (probably for more family-togetherness) and/or not choosing to spam the users with mentions on the wishes (but that wil probably be taken care of by other team-mates, anyway).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense, thanks !


end
2 changes: 2 additions & 0 deletions lib/config_reader.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

class ConfigReader
attr_accessor :db_path,
:mention,
:slack_url,
:channel_name,
:greeting_message,
Expand All @@ -13,6 +14,7 @@ def load(filename)
file = File.read(filename)
config = JSON.parse(file)
@db_path = config['db_path']
@mention = config['mention']
@slack_url = config['slack_url']
@channel_name = config['channel_name']
@greeting_message = config['greeting_message']
Expand Down