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 - Heather and Mina #12

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

Ports - Heather and Mina #12

wants to merge 36 commits into from

Conversation

minams
Copy link

@minams minams 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 and the provided Slack documentations to explore the Slack API. Using this combination will be helpful for the next project as well.
Give a short summary of the request/response cycle. Where does your program fit into that scheme? The program requested from the Slack API information about the User, Channel, and to Post Message. The Slack API responded with details on User and Channel. In regards to the Post Message, we received information on the posted message.
How does your program check for and handle errors when using the Slack API? The program API responded with a true instead of false. We wrote tests to ensure that user inputted value returns what we expected from the API, i.e. "general" channel returned name and ID.
Did you need to make any changes to the design work we did in class? If so, what were they? We partially implemented the parent class. We wanted to inherit the get method from the parent class but didn't yet.
Did you use any of the inheritance idioms we've talked about in class? How? Yes, in the initialize method of the child class. If we had more time, our next step was to inherit the get class as well.
How does VCR aid in testing a program that uses an API? It was able to run faster since it didn't need to make a request to the Slack API. Also, we could run the tests offline.

minams and others added 30 commits March 18, 2019 16:58
@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. No Slack token in git too!
Comprehension questions Check
Functionality
List users/channels Check
Select user/channel Check, but see my inline notes about how it's awkward
Show details Check
Send message Check
Program runs without crashing Check
Implementation
API errors are handled appropriately Check
Inheritance model matches in-class activity For the most part, your Workspace class is really just a collection of class methods, and you missed some opportunities for inheritance with User and Channel.
Inheritance idioms (abstract class, template methods, polymorphism) used appropriately Check
Methods are used to break work down into simpler tasks Check
Class and instance methods are used appropriately You've overused class methods in Workspace.
Overall Well done, you hit the learning goals of the project nice work. See my inline notes and let me know any questions you have.

case selection
when "list channels"
puts Slack::Workspace.all_channels_details
when "select channel"

Choose a reason for hiding this comment

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

The way you have this here means the channel doesn't stay selected. It also means you have a lot of duplicate code in selecting users and channels and sending messages etc.

def self.get
url = "https://slack.com/api/users.list"
params = {
token: ENV["KEY"],

Choose a reason for hiding this comment

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

In general don't name your environmental variable "KEY", make the name a bit more specific.

Also both the token and the URL would make great constants.

@topic = topic
end

def self.get

Choose a reason for hiding this comment

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

You could implement this method in Recipient and inherit the behavior.


message_request = HTTParty.post(url, query: params)
if message_request["ok"] == false
raise ArgumentError, "Request is unsuccessful"

Choose a reason for hiding this comment

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

You should also include the error message.

Dotenv.load

module Slack
class Workspace

Choose a reason for hiding this comment

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

This works as a collection of class methods, but if you're using all class methods, you might as well just make Workspace a module. Instead I would suggest making Workspace a class with the attributes @users, @channels and @selected, which means you could keep track of the selected user or channel in your main app.

expect(receiver).must_be_instance_of Slack::Recipient
end
end

Choose a reason for hiding this comment

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

You should also test that your abstract method raises an error.

@@ -0,0 +1 @@

Choose a reason for hiding this comment

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

??

@@ -0,0 +1,106 @@
require_relative "test_helper.rb"

Choose a reason for hiding this comment

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

In this file you also need to have some negative-case tests with invalid arguments.

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