Skip to content

Conversation

@eaball35
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? We spent time reviewing the SlackAPI documentation for the various requests including looking at attributes needed, error messages, etc. We got more comfortable looking at API documentation, using .ENV file, calling requests, using Postman.
Give a short summary of the request/response cycle. Where does your program fit into that scheme? 1. Request 2. Process 3. Response - We (client) send slack our request for info (given arguments & key) and slack(server) processes our request and then responds accordingly.
How does your program check for and handle errors when using the Slack API? It looks for response "ok" is true and raises and error if not. Error gets rescued so that it doesn't crash slack.rb but instead bumps user to menu again.
Did you need to make any changes to the design work we did in class? If so, what were they? We pretty much followed what was given.
Did you use any of the inheritance idioms we've talked about in class? How? Yes, we used them for 'load_all' and 'details' methods.
How does VCR aid in testing a program that uses an API? It helps reduce the number of API calls, particularly for repeat calls. Repeat calls can cause issues when the API has rate limits. Also, tests will still run if connection is lost.

@jmaddox19
Copy link

slack.rb

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene (no slack tokens) X
Comprehension questions X
Functionality
List users/channels Yes!
Select user/channel Yes!
Show details Yes!
Send message Works in a fashion different from that specified in requirements, only when I change the hard-coded channel in the code and only for users. See code comment in "send_message" for more details.
Program runs without crashing If an API call fails (ie. when sending a message) the SlackApiError is never handled to prevent the program from crashing. That said, that's the only time the program crashed for me and it's great that the api errors were checked and a custom exception raised!
Implementation
API errors are handled appropriately API error codes are handled by raising customer exception! Great! One improvement on this would be to handle (ie. rescue) the exceptions in the areas where these methods are called and give a user-readable error message. See inline comments for more details.
Inheritance model matches in-class activity Yes! Other than "send_message" missing from Recipient.
Inheritance idioms (abstract class, template methods, polymorphism) used appropriately Great parent methods in Recipient! Good job setting up calls from the children to the parent! Abstract methods setup appropriately!
Methods are used to break work down into simpler tasks Yes! Methods are neat and small! with the exception of "menu_action" which is understandable :)
Class and instance methods are used appropriately Yes!
Tests written for User functionality Yep
Tests written for Channel Functionality Yep!
Tests written for sending a message Yep!
Overall The design and code separation looks great overall! Testing looks great! There were a few weird things I called out in the inline comments. Overall though it looks great! I see clear demonstration of comfort with APIs, API errors, inheritance, and composition. Yay!

end

text = get_text
query_params = { token: ENV["SLACK_KEY"], channel: "CN69B7XMW", text: text, user: msg_recipient.id}

Choose a reason for hiding this comment

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

There are a couple issues with the way this is written.
The api used is meant to send a message to a specific user within a channel, rather than sending a message directly to a user or generally to a channel , which is intended to be the two options. (See api.slack.com/methods/chat.postMessage.) Happy to talk about that is the difference is unclear. Because of that, it looks like the recipient needs to be a user so trying to generally send a message to a channel won't work and the channel that needs to be entered separately is hard-coded here to be CN69B7XMW. This means that when I try to send a message to a user from my own test workspace, I'm unable to because the hard-coded channel doesn't exist.
In addition, the send_message method was meant to be supported in Recipient so that from Workspace you'd just need to call @entity.send_message(text) from Workspace.
I'm sure this will all become more clear as we do more projects but also feel free to ask a TA, tutor, me, or another staff member to dig into this with you deeper.

Choose a reason for hiding this comment

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

yeah... we couldn't figure out the correct api to use bc I thought our scope is not covered. Will go check out ohter people's code for this one haha.


@all_channels.each do |channel|
if (input == channel.id) || (input == channel.name.upcase)
return channel

Choose a reason for hiding this comment

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

This could update the @entity instance variable directly. This would be more true to OO design rather than expecting the variable to be updated by "slack.rb" from the return value. If this were done that way, no explicit return value would be needed because the "state" of the workspace instance would already be updated.


when "C", "SEND MESSAGE"
puts "SENDING MESSAGE"
send_message

Choose a reason for hiding this comment

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

similar to the way there is code to handle ArgumentErrors, it'd be good to have code here to handle the Slack Api Error.

Choose a reason for hiding this comment

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

oops, we forgot to wrap some of the methods in a begin/rescue block.

end

def self.load_all
names = self.get_names

Choose a reason for hiding this comment

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

The way these methods are seperated is a little awkward and inefficient because the same api ends up getting called 4 times when all of that information is available in the response from each call.

Choose a reason for hiding this comment

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

I understand the tradeoff though because it enables smaller, succinct methods, one for each attribute. You could accomplish both of these things by pulling the call to self.get out of these methods. If self.get were called directing from self.load_all, the results from self.get could be passed to each method like so:
result = self.get
self.get_names(result)
...

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