Skip to content

Conversation

@kdow
Copy link

@kdow kdow commented Mar 22, 2019

slack.rb

Congratulations! You're submitting your assignment!

You and your partner should collaborate on the answers to these questions.

Comprehension Questions

Question Answer
How did you go about exploring the Slack API? Did you learn anything that would be useful for your next project involving an API? We looked at the documentation and used Postman to see what our results looked like. We learned how useful Postman is for seeing responses and how it's useful to see how parameters changing alters the response.
Give a short summary of the request/response cycle. Where does your program fit into that scheme? You send a request to the API and will receive a response. We used that with self.get to populate the lists of users and channels. We also used send message to post data to the API and received a response indicating the success of that request.
How does your program check for and handle errors when using the Slack API? We raised an error if the message was not "ok" (as all responses came back with a status of 200).
Did you need to make any changes to the design work we did in class? If so, what were they? We added a parameter to a few methods in Workspace. The parameters are pieces of information coming from the user in the View.
Did you use any of the inheritance idioms we've talked about in class? How? Our Recipient class is an abstract class. It is never instantiated, it just serves as a parent for user and channel. In Recipient, self.list and details were template methods. These methods are needed in the child classes, but are not completed in the parent class. The methods in Recipient will raise an error if called directly. Workspace uses polymorphism in the show_details method. It will treat users and channels the same way.
How does VCR aid in testing a program that uses an API? It reduces API calls, which are costly. It does this by creating cassettes and using the cassettes in subsequent test runs.

kdow and others added 30 commits March 19, 2019 11:28
@droberts-sea
Copy link

slack.rb

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene (no slack tokens) yes
Comprehension questions yes
Functionality
List users/channels yes
Select user/channel yes
Show details yes
Send message yes
Program runs without crashing yes
Implementation
API errors are handled appropriately Kind of - you do a good job of checking for an error and raising a reasonable exception after sending the request. However, your command line code doesn't rescue that error, so the user still ends up seeing a stack trace instead of a polite error message.
Inheritance model matches in-class activity yes
Inheritance idioms (abstract class, template methods, polymorphism) used appropriately yes
Methods are used to break work down into simpler tasks yes
Class and instance methods are used appropriately yes
Overall Excellent job overall. This code is well-written and well-tested. It is clear to me that the learning goals around understanding the request/response cycle, consuming and API, and implementing a design using inheritance from scratch were all met. I've left a few inline comments for you to review below, but in general I'm quite impressed by what I see here. Keep up the hard work!

def find_channel(channel_info)
@channels.each do |channel|
if channel_info == channel.slack_id || channel_info == channel.name
return channel

Choose a reason for hiding this comment

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

This code and find_user above are almost exactly the same. Could you DRY this up?

it "should return nil, given invalid user" do
bad_username = ""
@workspace.select_user(bad_username)
expect(@workspace.selected).must_be_nil

Choose a reason for hiding this comment

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

If a user was previously selected, what should happen to that user?

it "returns nil if selected is nil" do
if @workspace.selected == nil
expect(@workspace.show_details).must_be_nil
end

Choose a reason for hiding this comment

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

You've done a great job of identifying and testing for failure cases throughout this project.

url = "https://slack.com/api/users.list"
data = {
"token": ENV["SLACK_API_TOKEN"],
}

Choose a reason for hiding this comment

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

Most of this data is going to be common to all subclasses of Recipient, the only thing that's changing is the end of the URL (users.list). Could you move some or all of it to Recipient.get?

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.

3 participants