Branches - Kelsey & Macaria by mdove92 · Pull Request #16 · Ada-C12/slack-cli

mdove92 · 2019-09-13T23:42:02Z

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 explored too much. The documentation was incredibly helpful in learning how to post and get from the API. We can now manipulate the API with variables. We can send cat pics with text overlays using the CATAAS API. (source: https://cataas.com)
Give a short summary of the request/response cycle. Where does your program fit into that scheme?	Our program send request to the API and the API responds with either a post, or info from a get. The post is a command and the get is query. Our slack.rb is made exclusively for a user making requests of the API.
How does your program check for and handle errors when using the Slack API?	We handle errors in two ways: if the user inputs an incorrect user name/id or channel name/id, we have the command line ask them to re-input it. If the user attempts to send a message or get details without a current recipient selected, then our program raises an argument error.
Did you need to make any changes to the design work we did in class? If so, what were they?	Our most significant change was eliminating the additional factory class that we had designed for our program. We realized it was redundant and that we could split up the work between workspace.rb and slack.rb.
Did you use any of the inheritance idioms we've talked about in class? How?	We used inheritance idioms in our Recipient class which are utilized by our Channel and User classes. We used `super` to call the idiomatic methods.
How does VCR aid in testing a program that uses an API?	VCR records the request to the API so that multiple requests aren't made. This is helpful with APIs that limit your calls from your unique token.

…wave 2 selection options

tildeee · 2019-09-21T06:32:51Z

slack.rb

What We're Looking For

Feature	Feedback
Core Requirements
Git hygiene (no slack tokens)	x
Comprehension questions	x ✨
Functionality
List users/channels	x
Select user/channel	x
Show details	Details actually don't get printed to the terminal ever, although the implementation is almost there-- just didn't return it or print it
Send message	x
Program runs without crashing	x, unless the intential argument error that was raised
Implementation
API errors are handled appropriately	doesn't have explicit code for this, but handles user errors
Inheritance model matches in-class activity	x
Inheritance idioms (abstract class, template methods, polymorphism) used appropriately	x
Methods are used to break work down into simpler tasks	x
Class and instance methods are used appropriately	x
Tests written for User functionality	mostly
Tests written for Channel Functionality	mostly
Tests written for sending a message	mostly
Overall

Great work on this project, Kelsey and Macaria!

The code is well-designed and well-written. It's great to see everything wrapped together so well-- great design, code style, tests, and implementation! Also, great work on the cat enhancement!

My thoughts for improvement are these:

In terms of your CLI program in slack.rb, some of the output that the user sees is very not-readable -- there's a tendency to just print out output like #<Channel:0x00007fde0717b6e0> or the literal method name, underscores and all select_channel. It'd probably be worth it to clean that up a bit.
Not everything is very thoroughly tested-- some methods don't have unit tests at all!
The test code style is good, but the assertions end up going for approximation. I'd encourage y'all to test for details when the details are possible! For example, if the test data is all there, and you know exactly what the users and channels are, you can test on more detail than things being Arrays or having strings that include certain properties-- you probably can get the literal answer that it should be every time! If you can, test on that! :)

Any other comment I have a minor one that I've attached. One of them is about handling API errors: we didn't go into handling API errors in depth during class, but I'd encourage you two to start thinking about: what happens if my response comes back with a status code that's not 200 OK? Your code currently doesn't handle that and would break in those cases! Food for thought.

Overall, I think that the code and project submission are wonderful! I'd love to see a little more attention to detail in tests and user-output in the future, but otherwise you two did a great job on this project!

tildeee · 2019-09-21T06:34:02Z

lib/channel.rb

+  end
+
+  def self.load_all(url, token)
+    recipients = HTTParty.get(url, query: { token: token })


What happens if this get request gives back an error response? What would happen when we did recipients["channels"] in the next line?

tildeee · 2019-09-21T06:34:23Z

lib/slack.rb

+      puts workspace.find_by_id_or_name("user", selection)
+    when "5" || "details"
+      puts "list_details_on_current_recipient"
+      workspace.list_details_on_current_recipient


This doesn't puts the details on the current recipient!

Also, even if you add puts here, it just prints the recipient. There's a bug in your list_details_on_current_recipient method!

tildeee · 2019-09-21T06:34:34Z

lib/user.rb

+    @username = username
+  end
+
+  def self.load_all(url, token)


Seems like there isn't a test for this?

tildeee · 2019-09-21T06:34:47Z

lib/workspace.rb

+    @channel_url = "https://slack.com/api/conversations.list"
+    @user_url = "https://slack.com/api/users.list"
+    @message_url = "https://slack.com/api/chat.postMessage"
+    @channel_member_url = "https://slack.com/api/conversations.members"


Consider making these constant variables!

tildeee · 2019-09-21T06:34:55Z

lib/workspace.rb

+    end
+  end
+
+  def find_by_id_or_name(recipient_type, search_arg)


This method is great!

tildeee · 2019-09-21T06:35:08Z

lib/workspace.rb

+      recipient_list = @users
+    else
+      recipient_list = @channels
+    end


Possible refactor: Consider recipient_list = recipient_type.downcase == "user" ? @users : @channels -- your tests still pass!

tildeee · 2019-09-21T06:35:23Z

lib/workspace.rb

+    if @current_recipient == nil
+      raise ArgumentError, "No recipient is currently selected."
+    else
+      @current_recipient.load_details


This line evaluates into a string with some details, but it doesn't return anything meaningful overall; it doesn't puts anything either. In the end, this method just returns @current_recipient below

I wish your tests checked on this behavior!

tildeee · 2019-09-21T06:37:53Z

lib/workspace.rb

+                                    "blocks" => cat_block,
+                                  })
+    else
+      new_message = HTTParty.post(@message_url,


What happens if this POST request comes back with an error response? That is not handled!

tildeee · 2019-09-21T06:38:05Z

lib/workspace.rb

+
+  def list_channels
+    channel_list = HTTParty.get(@channel_url, query: { token: TOKEN })
+    channel_list["channels"].map do |channel|


I'd like a more explicit return statement here so it's more readable!

tildeee · 2019-09-21T06:38:44Z

test/user_test.rb

+  describe "load_details" do
+    it "returns an accurate username" do
+      expect @user.load_details.must_be_instance_of String
+      expect @user.load_details.must_include "ID:"


Because you know all of the details about the test data (what's in the VCR casettes), you could probably check that the string is exactly what you're looking for (in this case, the literal string "User Details\nID: UMURH3TBM\nName: Mackie Dove\nUsername: m.dove")

tildeee · 2019-09-21T06:39:17Z

test/workspace_test.rb

+    end
+    it "gets details on a selected recipient" do
+      @workspace.find_by_id_or_name("channel", "CN85SCME3")
+      expect(@workspace.list_details_on_current_recipient).must_be_instance_of Channel


Is this actually what you want to do?

tildeee · 2019-09-21T06:39:26Z

test/workspace_test.rb

+    it "can send a message with given text" do
+      expect @test_message["message"]["text"].must_equal "POTTYMOUTH"
+    end
+    it "allows for inclusion of a cat_block" do


m.dove@live.com and others added 9 commits September 10, 2019 16:30

Added class files. Modified slack.rb to contain user interface

b119be4

Wrote list users method

70b8b4a

added Workspace tests and removed factory as a class

461ce4a

Added method to find recipient by id or name; updated CLI to include …

861f301

…wave 2 selection options

Made one main instance of Workspace

66734ae

Added method to load details of the current recipient

ac5ab7e

Created initial method for sending a message

9ab14a7

Added working tests. Increasing coverage

062a3fa

Added cassettes, added more complete coverage. Edited slack.

074a60f

tildeee reviewed Sep 21, 2019

View reviewed changes

lib/user.rb

@username = username

end

def self.load_all(url, token)

Copy link

tildeee Sep 21, 2019

Choose a reason for hiding this comment

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

Seems like there isn't a test for this?

tildeee reviewed Sep 21, 2019

View reviewed changes

lib/workspace.rb

end

end

def find_by_id_or_name(recipient_type, search_arg)

Copy link

tildeee Sep 21, 2019

Choose a reason for hiding this comment

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

This method is great!

tildeee reviewed Sep 21, 2019

View reviewed changes

Conversation

mdove92 commented Sep 13, 2019

slack.rb

Comprehension Questions

Uh oh!

tildeee commented Sep 21, 2019

slack.rb

What We're Looking For

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants