Skip to content

Conversation

@aribray
Copy link

@aribray aribray 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 used Postman to check the API response. We also used the Slack API documentation to make sure our response matched the expected response. In the future, using Postman will really help visualize the expected response.
Give a short summary of the request/response cycle. Where does your program fit into that scheme? The client is our program, and the server is the Slack API. The details of the request are in either the .get or .post verbs. The details about the response come back in JSON format.
How does your program check for and handle errors when using the Slack API? It checks if we have a valid token and raises a SlackError (which inherits from StandardError) if the token is invalid. It checks if the ["ok"] value is true. We don't check for the 200 status code because the SlackAPI always returns that. We also test for invalid user names and channel names and raise a SlackError if found.
Did you need to make any changes to the design work we did in class? If so, what were they? In our original UML, we didn't have a Workspace. As we were working in the CLI, we realized that many of our methods could live in a Workspace and this will make our program more flexible for future changes.
Did you use any of the inheritance idioms we've talked about in class? How? Yes, we used inheritance for the get method and for the send message method. We used it for these methods because they were very similar for both user and channel classes.
How does VCR aid in testing a program that uses an API? The VCR aids in testing a program that uses an API because it records the API call and you only need to do it once, not every time that you run your tests. This speeds up testing and reduces cost (if the API call is not free).

msfinnan and others added 24 commits March 19, 2019 12:27
… channel details to accomodate table print. Updated slack.rb to use table print
… workspace. Moved associated tests to workspace
@CheezItMan
Copy link

slack.rb

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene (no slack tokens) Good number of commits and good commit messages
Comprehension questions Check
Functionality
List users/channels Check
Select user/channel Check, but I can't use the ID field
Show details Check
Send message Check
Program runs without crashing Yes, but if I have an invalid user selected the app crashes
Implementation
API errors are handled appropriately Your library code is raising errors properly, but your UI doesn't handle them when they get raised.
Inheritance model matches in-class activity NOPE
Inheritance idioms (abstract class, template methods, polymorphism) used appropriately Well done, good use of inheritance
Methods are used to break work down into simpler tasks For the most part, see my inline comments for some of your UI.
Class and instance methods are used appropriately Your User and Channel classes could be used to create objects representing individual users or channels. Then they could inherit the send_msg method resulting in a more flexible design by using duck typing.
Overall Not bad, you hit most of the learning goals. See my comments inline and let me know if you have questions. The biggest issues you have to work on are OO Design, see my notes on Channel and User and check out the instructor reference solution.

# TODO project

puts "Thank you for using the Ada Slack CLI"
until response == "7" || response == "quit"

Choose a reason for hiding this comment

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

Oh thank god you respond to the numbers!


selected_user = ""

response["members"].each do |member|

Choose a reason for hiding this comment

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

You could also use the .find method as well.

expect(response.length).must_equal 3
end
end
it "will raise an error if given an invalid key" do

Choose a reason for hiding this comment

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

👍

end

describe "see_details" do
it "shows details for selected user" do

Choose a reason for hiding this comment

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

What about when no user has been selected. You also have 2 describe blocks with the same "see_details" description... Little odd


puts "Thank you for using the Ada Slack CLI"
until response == "7" || response == "quit"
if response == "list users" || response == "1"

Choose a reason for hiding this comment

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

This set of if-else blocks, really looks like it could be broken up into methods.


if response["ok"]
response["members"].each do |member|
user_list << {"name" => member["name"], "real name" => member["real_name"], "slack id" => member["id"]}

Choose a reason for hiding this comment

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

This method would make more sense if it returned a list of User instances with name, read name and id properties.

raise SlackAPI::SlackError, "There was an error. The error message is #{response["error"]}"
else
response["channels"].each do |channel|
channel_list << { "name" => channel["name"], "topic" => channel["topic"]["value"], "member count" => channel["members"].length, "id" => channel["id"] }
Copy link

@CheezItMan CheezItMan Mar 31, 2019

Choose a reason for hiding this comment

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

You have missed an opportunity to create an array of Channel instances instead of an array of hashes.

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