Skip to content
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

Ports - Faiza & Stephanie #19

Open
wants to merge 43 commits into
base: master
Choose a base branch
from
Open

Conversation

Faiza1987
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 used Postman to figure out how to access the JSON data and that the order in which we access information is important.
Give a short summary of the request/response cycle. Where does your program fit into that scheme? We're using our self.get method to access data from the API and we're using our send_message methods to post messages to Slack.
How does your program check for and handle errors when using the Slack API? We check if users input matches what data is included in the API. We also check that if the response code is not 200, to raise a custom error.
Did you need to make any changes to the design work we did in class? If so, what were they? Yes we did! We initially did not use a recipient class, but implemented one in order to complete wave 3.
Did you use any of the inheritance idioms we've talked about in class? How? User and Channel are inheriting from Recipient. And we used super keyword.
How does VCR aid in testing a program that uses an API? VCR records an instance of API calls so that we don't have to keep making calls to the API while we're testing.

Faiza1987 and others added 30 commits March 19, 2019 14:20
…eated classes Channel, User and Workspace. Added Initialize method in Channel and wrote a corresponding test.
@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, but you didn't give a summary of the request/response cycle.
Functionality
List users/channels Check, but it's awesome print printing the objects.
Select user/channel Check, but if I give an invalid name, the details and send message methods crash!
Show details Check, but it's awesome print printing the object and see above
Send message Check
Program runs without crashing It crashes if given invalid input.
Implementation
API errors are handled appropriately See above about the app crashing,
Inheritance model matches in-class activity You have inheritance, but have issues with your details method
Inheritance idioms (abstract class, template methods, polymorphism) used appropriately Check, but see how you overrode the details method.
Methods are used to break work down into simpler tasks Check
Class and instance methods are used appropriately Check, well done here
Overall You hit the learning goals for the project, and have a good design. However you have some problems with implementation. You use ap to just print users and channels to let the user see them, which is... less than ideal. You also need more error checking in your UI. Lastly you have some issues with Recipient#details, Channel#details and User#details. See my inline comments and let me know if you have questions.

end

def channel_details
super.push("topic", "member_count")

Choose a reason for hiding this comment

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

Clever, except that the Recipient class doesn't have a channel_details method. This method should be named details.

return response
end

def send_message(channel, text)

Choose a reason for hiding this comment

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

Well done

end

def details
["name", "slack_id"]

Choose a reason for hiding this comment

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

This method just returns an array with the strings "name" and "slack_id" in it... This isn't useful data. I would suggest:

def details
  return "name: #{name}\nID: #{slack_id}"
end

end

def select_user(name_or_id)
users.each do |user|

Choose a reason for hiding this comment

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

You can use users.find here to simplify it.

return @selection
end

def select_channel(name_or_id)

Choose a reason for hiding this comment

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

Do you notice how this and select_user are almost identical... that's a good sign you can dry it up with a helper method.

@@ -0,0 +1,14 @@
require_relative "test_helper"

Choose a reason for hiding this comment

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

Incomplete testing here

user = Slack::User.list

expect(user).must_be_kind_of(Array)
# check at index that its an instance of user

Choose a reason for hiding this comment

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

Just need to verify that all elements are instances of User.

end

describe "Show details method" do
it "returns details" do

Choose a reason for hiding this comment

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

incomplete tests

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