Ports - Alex and Amy by amythetester · Pull Request #10 · Ada-C11/slack-cli

amythetester · 2019-03-22T22:24:23Z

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 it was awesome! The API method documentation was really helpful to know all of the methods for the API. Both of these resources will be helpful for our next API project because we now know how to look through API documentation, and Postman helps get some quick wins without writing code that you can then incorporate into your program.
Give a short summary of the request/response cycle. Where does your program fit into that scheme?	In the request/response cycle the client sends a request for information to the server, the server processes that request and then sends a response back to the client. In our case the client was our CLI program and the server was the server(s) that runs the Slack API.
How does your program check for and handle errors when using the Slack API?	There are several times where we check the value of the "ok" key in the JSON response. Slack always returns 200 OK codes so relying on that to tell us if we got a bad API response is not helpful. So, we checked to see if "ok" value was false. If so, then we expected to get an error.
Did you need to make any changes to the design work we did in class? If so, what were they?	Yes, we ran into time constraints with the inheritance component of the project. We understand the usefulness of inheritance and if given more time would implement the Recipient class. Additionally, on the last day of the project, we were informed that our approach works, but misses on using instances of the Channel and User classes. We understand how doing this would make our code more stable if the data supplied to the program changed in the future, but determined that there was not enough time to refactor and accommodate this expectation.
Did you use any of the inheritance idioms we've talked about in class? How?	No we didn't, please see answer above for reasons why!
How does VCR aid in testing a program that uses an API?	VCR records API calls on cassettes so that tests can continue to run even when the program does not have access to the API. This also means that you cut down on the total number of calls made to the API, so if API calls cost money then the overall cost is lower.

…ough JSON

… no recipient was selected

…sette

CheezItMan · 2019-03-31T17:55:11Z

slack.rb

What We're Looking For

Feature	Feedback
Core Requirements
Git hygiene (no slack tokens)	Check, well done
Comprehension questions	Check, sorry you didn't have time to look at inheritance, but it's understandable.
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	Chheck
Inheritance model matches in-class activity	No, but comprehension questions explained why
Inheritance idioms (abstract class, template methods, polymorphism) used appropriately	Not really, you have a collection of class methods and workspace is doing a lot of work which might be better moved to the `User` and `Channel` classes, if you had instances of them. These classes could have a lot of functionality that `Workspace` could use. See the instructor reference design.
Methods are used to break work down into simpler tasks	For the most part
Class and instance methods are used appropriately	You are definitely overusing class methods, you missed a bit of opportunity to create instance of `User` and `Channel`.
Overall	Overall nice work, you hit the main learning goals and the app works well. That said your design is rather rigid, and hard to change. You are mixing the display and business logic roles in classes and you didn't take advantage of polymorphism, composition or inheritance. Please review the instructor reference design and let me know if you have questions on how it would work and why your design is a bit more awkward.

CheezItMan · 2019-03-31T17:42:19Z

lib/channel.rb

+  class SlackError < StandardError; end
+
+  class Channel
+    url = "https://slack.com/api/channels.list"


I would make these constants instead of just regular variables.

CheezItMan · 2019-03-31T17:44:08Z

lib/channel.rb

+    end
+
+    def self.list(channels_list)
+      puts "\nHere are a list of your channels for this Workspace:"


You should try to separate the "display" functions of your app with the functionality and business logic. This "model-like" class shouldn't have to worry about printing things to the screen, instead it should work with the api and return data back.

CheezItMan · 2019-03-31T17:45:15Z

lib/user.rb

+  class SlackError < StandardError; end
+
+  class User
+    url = "https://slack.com/api/users.list"


Again, you should default to using constants unless you want to change the values of these variables.

CheezItMan · 2019-03-31T17:46:04Z

lib/user.rb

+    url = "https://slack.com/api/users.list"
+    key = ENV["SLACK_API_TOKEN"]
+
+    def self.user_api(url, key)


This method name could use a bit of work, rather than call it user_api I would call it, get_users. Also if key is already saved above, why do you need it as an argument?

CheezItMan · 2019-03-31T17:47:10Z

lib/user.rb

+    end
+
+    def self.list(users_list)
+      puts "\nHere are a list of your users for this Workspace:"


Again, I would separate out the presentation with the business logic of your application.

CheezItMan · 2019-03-31T17:49:14Z

lib/workspace.rb

+    end
+
+    def select_channel(user_input)
+      @channels.each do |channel|


You could also use the .find method as well.

CheezItMan · 2019-03-31T17:56:19Z

specs/channel_spec.rb

+        key = ENV["SLACK_API_TOKEN"]
+        channels_list = SlackApi::Channel.channel_api(url, key)
+
+        expect(channels_list.first["name"]).must_equal "general"


Different Slack orgs will return channels in different orders. I would instead try to verify that the list of channels contains "general" instead.

You can't make assumptions on the order that the API returns.

CheezItMan · 2019-03-31T17:56:40Z

specs/channel_spec.rb

+        key = ENV["SLACK_API_TOKEN"]
+        channels_list = SlackApi::Channel.channel_api(url, key)
+
+        expect(channels_list.first["id"]).must_equal "CH2RC3CNQ"


Since you're using this in several places, I would make the channel id a constant.

CheezItMan · 2019-03-31T17:57:42Z

specs/channel_spec.rb

+        key = ENV["SLACK_API_TOKEN"]
+        channels_list = SlackApi::Channel.channel_api(url, key)
+
+        expect(channels_list.first["num_members"]).must_equal 2


Because a slack app will constantly change the number of users, I would instead just verify that the field exists and is a number.

You are also reusing the same test name in several different tests.

CheezItMan · 2019-03-31T17:58:23Z

specs/channel_spec.rb

+        channels_list = SlackApi::Channel.channel_api(url, key)
+
+        channels_list.each do |channel|
+          name = "Not a Channel"


You don't need to write a test to verify that a channel that does not exist doesn't appear in the list.

amythetester and others added 27 commits March 19, 2019 14:01

api verification completed and initial user interface added

1b0fb11

added list and get methods to User and Channel classes

d3fdbef

implemented self.list for User and Channel in slack.rb file

3f7eafc

added quit functionality and reprompt loop to slack.rb file

fc5769e

updated test_helper file to include VCR configuration

0e2c0fa

implemented helper method to hold API information and parse JSON

607d41a

implemented helper method for user.rb to store API info and parse thr…

4b89f88

…ough JSON

wrote corresponding tests for 'list channels'

65f53bb

wrote corresponding tests for list users

3e7ac4a

added Workspace class and select user and select channel methods

05bee27

added show_details method and functionality for workspace

6498e56

edited cli formatting and updated self.details method to respond when…

c4697ce

… no recipient was selected

added test outlines for workspace

e3e34f4

added preliminary code for sending messages to recipients

c267404

edited workspace to initialize with all channels and users

5e5665e

changed class methods to instance methods in workspace.rb

f58f4f2

fixed VCR boo-boo (moved initialization of Workspace into VCR.use_cas…

5d541ed

…sette

select_user tests added

3b9e604

added tests for show_details for workspace class

d63ce76

added more tests for each workspace method

4379d22

edited UI for cleaner output

1af0227

edited and completed send_message functionality with corresponding tests

960edc3

added tests for listing users and channels

2d6a4f5

added tests to user and channel. updated CLI to get rid of extra puts

e429dfc

added API testing

8e24b97

project file clean up

20a9e5f

additional project clean up

4f0b1b9

CheezItMan reviewed Mar 31, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ports - Alex and Amy#10

Ports - Alex and Amy#10
amythetester wants to merge 27 commits intoAda-C11:masterfrom
amythetester:master

amythetester commented Mar 22, 2019

Uh oh!

CheezItMan commented Mar 31, 2019

Uh oh!

CheezItMan Mar 31, 2019

Uh oh!

CheezItMan Mar 31, 2019

Uh oh!

CheezItMan Mar 31, 2019

Uh oh!

CheezItMan Mar 31, 2019

Uh oh!

CheezItMan Mar 31, 2019

Uh oh!

CheezItMan Mar 31, 2019

Uh oh!

CheezItMan Mar 31, 2019

Uh oh!

CheezItMan Mar 31, 2019

Uh oh!

CheezItMan Mar 31, 2019

Uh oh!

CheezItMan Mar 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

amythetester commented Mar 22, 2019

slack.rb

Comprehension Questions

Uh oh!

CheezItMan commented Mar 31, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants