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 - Amy & Ngoc #11

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

Ports - Amy & Ngoc #11

wants to merge 40 commits into from

Conversation

lebaongoc
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 utilized Postman to create get and post request and examined the JSON structure of API responses to guide our code.
Give a short summary of the request/response cycle. Where does your program fit into that scheme? The first part of the request-response cycle is the request. A request is made by the browser to the server. Once the server has finished processing a request, it must send back a response. This lets the client know that the work is done, and gives them any information they requested. Our program used a self.get method to request information about users and channels from the Slack API, then used the self.list method to format and create User and Channel objects. We used post requests to send messages to users and channels via the CLI within our workspace, then we checked the response of that request to let the user know whether their message was sent correctly.
How does your program check for and handle errors when using the Slack API? It raises an ArgumentError when there's invalid input or parameters, then use rescue blocks to handle the error in the CLI.
Did you need to make any changes to the design work we did in class? If so, what were they? Yes, we didn't use a parent/inherited Recipient class.
Did you use any of the inheritance idioms we've talked about in class? How? No.... :)
How does VCR aid in testing a program that uses an API? It records previous API requests and responses to reduce the cost of those actions on the program.

@CheezItMan
Copy link

slack.rb

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene (no slack tokens) Good commit messages and no slack token in cassettes!
Comprehension questions Check, sorry you didn't get to try inheritance.
Functionality
List users/channels Check
Select user/channel Check
Show details Check
Send message Check
Program runs without crashing Check
Implementation
API errors are handled appropriately Check
Inheritance model matches in-class activity Nope
Inheritance idioms (abstract class, template methods, polymorphism) used appropriately Somewhat no use of inheritance, but using composition is done appropriately
Methods are used to break work down into simpler tasks Check
Class and instance methods are used appropriately Check
Overall Nicely done, you hit the main learning goals for the project. See my inline notes about a few minor issues, but the program works well and does what's required. Read my inline feedback and let me know any questions. Also let your partner know I reviewed the project.

require "table_print"
require "httparty"
require "colorize"
require_relative "workspace"

Choose a reason for hiding this comment

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

You end up having some circular references here. It's important that you try to avoid having file A require file B and then file B requiring file A. That will give you a warning and it's bad form as if the interpreter wasn't well written it could lead to an infinite loop.

puts "Welcome to the Ada Slack CLI!"
work_space = Workspace.new
puts "Welcome to the Ada Slack CLI!".colorize(:color => :blue, :mode => :bold)
puts "\nPlease Choose from the following options:\n1. List Users\n2. List Channels\n3. Select User\n4. Select Channel\n5. Details\n6. Send Message\n7. Quit".colorize(:color => :blue, :mode => :bold)

Choose a reason for hiding this comment

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

Just a note, if you put numbers in front of the options, people want to use the number to select the option.

if channel_info["ok"] == false
raise ArgumentError, "The error code is #{channel_info.code} and the reason is: #{channel_info.message}"
end
return channel_info

Choose a reason for hiding this comment

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

Instead of returning the hash about the channel, I suggest returning a new Channel instance.

Something like:

return Channel.new(channel_info["name", channel_info["id"], channel_info["topic"], channel_info["members"])

Also is this method necessary? You already have a method to get a list of Channels, so why have a method to get the details about one?

if @user_info["ok"] == false
raise ArgumentError, "The error code is #{@user_info.code} and the reason is: #{@user_info["error"]}"
end
return @user_info

Choose a reason for hiding this comment

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

Similar questions to the method in channel.rb

it "selects a known user by username" do
VCR.use_cassette("Workspace") do
response = Workspace.new
response.select_user("ngocle")

Choose a reason for hiding this comment

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

These values that you are using over and over again would make great constants to have at the top. That way if you changed slack orgs you could just change a few constants and the tests would work.

end

describe "Workspace#show_details" do
it "shows details of the selected channel" do

Choose a reason for hiding this comment

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

You should also have a test that tests what happens when details is called and nothing has been selected.

end
end
describe "Workspace#send_message" do
it "can send a valid message to a channel" do

Choose a reason for hiding this comment

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

You should also test send_message if no user/channel has been selected.

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