-
Notifications
You must be signed in to change notification settings - Fork 26
Branches - Sara + Monick #7
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
base: master
Are you sure you want to change the base?
Conversation
… all necessary data
…. Updated tests after refactoring
… message to slackcli and it works
slack.rbWhat We're Looking For
GREAT WORK on this project, Monick and Sara! Your project code is clean, well-written, your test's nominal cases look great, and overall works well. Notably, I think that your The biggest thing to note is that there aren't any tests that talk about any edge cases; what happens when things go wrong? We didn't go into handling API errors in depth during class, but I'd encourage you two to start thinking about: what happens if my response comes back with a status code that's not That being said, all of my other comments are minor suggestions for how to improve code style. Overall, this is a great project submission; well done! |
| end | ||
|
|
||
| def self.list | ||
| method_url = "https://slack.com/api/channels.list" |
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.
Minor suggestion: Could this be a constant variable?
| channels = response.parsed_response["channels"] | ||
| i = 0 | ||
| all_channels = [] | ||
| channels.each do |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.
Minor suggestion for refactoring: instead of using .each and i and incrementing i, consider using channels.each_with_index do |channel, index|, which has built-in support for an index
| channel: recipient, | ||
| text: message | ||
| } | ||
| slack_post = HTTParty.post(method_url, query: query_params) |
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.
Something to think about for the future: What happens if this POST request comes back with a response that has an error? Your code currently doesn't handle that!
| when "select user" | ||
| ap "Type a username or Slack ID." | ||
| search_user = gets.chomp | ||
| selected_recipient = SlackCli::User.list.find do |i| |
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.
Minor suggestion: Instead of the variable name i, could we change it to something like recipient or user? This just helps me understand what kinds of elements are in the SlackCli::User.list array
| selected_recipient = SlackCli::User.list.find do |i| | ||
| i[:user_name]== search_user || i[:id] == search_user | ||
| end | ||
| if selected_recipient == nil |
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.
Minor suggestion: the code selected_recipient.nil? does the same thing as selected_recipient == nil, in case that looks better in your opinion
| end | ||
| end | ||
|
|
||
| describe "channel list method should return correct values" do |
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.
Great tests in here!
| VCR.use_cassette("slack_details") do | ||
| test = SlackCli::Recipient.send_message(recipient:@selected_recipient.id, message:@message) | ||
| expect(test.code).must_equal 200 | ||
| expect(test.body).must_include "IT WORKS!" |
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.
Honestly, this is a great test overall!
slack.rb
Congratulations! You're submitting your assignment!
You and your partner should collaborate on the answers to these questions.
Comprehension Questions