Skip to content

Conversation

@cyndilopez
Copy link

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? Reading the documentation on the API was very helpful to understand the required and optional arguments we used and can use for future interactions.
Give a short summary of the request/response cycle. Where does your program fit into that scheme? In our case our slack channel interacts with the user (view), our workspace functions as the controller allowing user requests to correspond with the methods from user and channel classes (model) with recipient as a parent .
How does your program check for and handle errors when using the Slack API? We checked that the repsonse returned a code of 200 and "true" for keyword "ok". It the program didn't pass this test, then a SlackAPIerror was raised with a message corresponding to the error. SlackAPIerror is a class that inherits methods and attributes from Standard Error.
Did you need to make any changes to the design work we did in class? If so, what were they? No, we didn't make any changes.
Did you use any of the inheritance idioms we've talked about in class? How? We used "super" in channel and user to inherit the slack_id and name from recipient. User and Channel inherited methods, including the self.list and self.details template methods from Recipient, and SlackAPIError inherited from Standard Error.
How does VCR aid in testing a program that uses an API? It collaborates with WebMock which intercepts http responses from common http libraries, and as we still have to provide the response data VCR records the http responses in cassettes, providing faster responses since we don’t have to contact the API for testing.

cyndilopez and others added 30 commits March 19, 2019 11:38
@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 no - looks like this requirement was missed
Select user/channel yes
Show details yes
Send message yes
Program runs without crashing yes
Implementation
API errors are handled appropriately
Inheritance model matches in-class activity
Inheritance idioms (abstract class, template methods, polymorphism) used appropriately
Methods are used to break work down into simpler tasks
Class and instance methods are used appropriately
Overall Good job overall. This code is well-written and well-tested. Though you missed the requirement to be able to list users and channels, 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. Also, good work on the optionals. I've left a few inline comments for you to review below. Review those, make sure to read the requirements a little more carefully next time, and keep up the hard work!

response = HTTParty.post(url, body: params)
unless response.code == 200 && response.parsed_response["ok"]
raise SlackApiError, response["error"]
end

Choose a reason for hiding this comment

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

Line 21 raises an exception, but not the one you want.

select user
You chose to select a user. Please provide a username or Slack ID
dan
Traceback (most recent call last):
	5: from lib/slack.rb:105:in `<main>'
	4: from lib/slack.rb:40:in `main'
	3: from lib/slack.rb:40:in `new'
	2: from /Users/droberts/Ada/c11/projects/slack-cli/lib/workspace.rb:11:in `initialize'
	1: from /Users/droberts/Ada/c11/projects/slack-cli/lib/user.rb:16:in `list'
/Users/droberts/Ada/c11/projects/slack-cli/lib/recipient.rb:33:in `get': uninitialized constant Recipient::SlackApiError (NameError)

Ruby doesn't know what a SlackApiError is, so you get a NameError when you try to raise one. Looks like you're missing require_relative 'apierror' at the top of this file.

def initialize(slack_id:, name:)
@slack_id = slack_id
raise ArgumentError if !name.is_a? String
@name = name

Choose a reason for hiding this comment

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

Best practice is to avoid explicit type checking in a dynamic language like Ruby. This is for two reasons. First, if name really needs to be a String, then we'll get an error soon enough one way or the other. Second, if the user passes in something that close enough to a string to work (i.e. implements the String interface), there's no reason for our code to fail.

def select_channel
channel_selected = channels.detect do |channel|
channel.slack_id == selected || channel.name == selected
end

Choose a reason for hiding this comment

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

This code is very similar to the code for select_user above. Could you DRY this up somehow?

def send_message(message, recipient)
params = {}
params[:text] = message
params[:channel] = recipient.slack_id

Choose a reason for hiding this comment

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

Instead of taking the recipient as an argument here, you should send the message to whatever is saved in @selected. Similarly for details below.

user_selected = users.detect do |user|
user.slack_id == selected || user.name == selected
end
return user_selected

Choose a reason for hiding this comment

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

Good use of the .detect enumerable here.

Since this doesn't save the user in @selected, I'm confused what that instance variable is for.

it "returns nil if username or slack id is not found" do
VCR.use_cassette("check nil returned") do
selected = "chewy" #id of nonexistent slackbot
@workspace2 = Workspace.new(selected: selected)

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 error cases throughout this project.

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