Earth - Anya/Emily #21
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CheezItMan
reviewed
Oct 15, 2020
There was a problem hiding this comment.
Slack CLI
Major Learning Goals/Code Review
| Criteria | yes/no, and optionally any details/lines of code to reference |
|---|---|
Practices best practices working with APIs. The .env is not checked into git, and no API token was directly used in the Ruby code without ENV. |
✔️ |
| Practices error handling with APIs. For all pieces of code that make an API call, it handles API requests that come back with errors/error status codes appropriately. | ✔️ |
Implements inheritance and inheritance idioms. There is a Recipient class. User and Channel inherit from Recipient. In Recipient, there are appropriate methods defined that are used in both User and Channel. Some may be implemented. Some may be template methods. |
✔️, nice work, but I recommend adding send_message to recipient (see note inline). |
Practices clean code. lib/slack.rb only interacts with Workspace to show a separation of responsibilities. Complex logic is broken into smaller helper methods. |
✔️, Good helper methods |
| Practices instance methods vs. class methods appropriately. The methods to list all Channels or Users is likely a class method within those respective classes. | ✔️ |
| Practices best practices for testing. The project has and uses VCR mocking when running tests, and can run offline. | ✔️ |
Practices writing tests. The User, Channel, and Workspace classes have unit tests. |
✔️ |
Practices writing tests. There are tests for sending messages (the location of these tests may differ, but is likely in Recipient) |
|
| Practices git with at least 15 small commits and meaningful commit messages | ✔️ |
Functional Requirements
| Functional Requirement | yes/no |
|---|---|
| As a user of the CLI program, I can list users and channels | ✔️ |
| As a user of the CLI program, I can select users and channels | ✔️ |
| As a user of the CLI program, I can show the details of a selected user or channel | ✔️? |
| As a user of the CLI program, when I input something inappropriately, the program runs without crashing | ✔️? |
Overall Feedback
| Overall Feedback | Criteria | yes/no |
|---|---|---|
| Green (Meets/Exceeds Standards) | 7+ in Code Review && 3+ in Functional Requirements |
Code Style Bonus Awards
Was the code particularly impressive in code style for any of these reasons (or more...?)
| Quality | Yes? |
|---|---|
| Perfect Indentation | ✅ |
| Elegant/Clever | ✅ |
| Descriptive/Readable | ✅ |
| Concise | ✅ |
| Logical/Organized | ✅ |
Summary
This was really excellent work. You hit all the learning goals with a good design. Well done.
Comment on lines
+23
to
+25
| def self.from_response_hash(record_hash) | ||
| return Channel.new(record_hash['name'], record_hash['id'], record_hash['topic']['value'], record_hash['num_members']) | ||
| end |
| @@ -0,0 +1,39 @@ | |||
| require 'dotenv' | |||
|
|
|||
| class Recipient | |||
There was a problem hiding this comment.
I love the use of template methods here. Nice work!
Comment on lines
+64
to
+83
| def send_message(selected_slack_id, user_response) | ||
| url = "https://slack.com/api/chat.postMessage" | ||
|
|
||
| response = HTTParty.post(url, | ||
| headers: { 'Content-Type' => 'application/x-www-form-urlencoded' }, | ||
| body: { | ||
| token: ENV["SLACK_TOKEN"], | ||
| channel: selected_slack_id, | ||
| text: user_response | ||
| }) | ||
|
|
||
| unless response.code == 200 | ||
| raise InvalidAPIError, "Error! Status code: #{response.code}" | ||
| end | ||
|
|
||
| unless response["ok"] | ||
| raise InvalidAPIError, "Your message #{user_response} did not go through. Error: #{response["error"]}" | ||
| end | ||
| return true | ||
| end |
There was a problem hiding this comment.
I would suggest adding send_message to the recipient class thus removing responsibility for dealing with the API from the workspace class. Separating concerns.
| end | ||
| end | ||
|
|
||
| describe "send_message" do |
There was a problem hiding this comment.
You should also have tests for sending messages to invalid user/channel.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Assignment Submission: Slack CLI
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection
Give a short summary of the request/response cycle. Where does your program fit into that scheme? | We send a request through the URL, which Slack API receives by validating our token. It then fetches the data that we requested and sends back a response.
How does your program check for and handle errors when using the Slack API? | Our program handles errors by raising a custom error when the status code isn't 200 and when the response["ok] is false.
How did the design and organization of your project change over time? | We started out by writing all of our code in slack.rb just to make sure we grasped the API concepts. We then refactored that code and put the methods into their relevant classes.
Did you use any of the inheritance idioms we've talked about in class? How? | Yes! Many of our classes inherited from Recipient.
How does VCR aid in testing a program that uses an API? | VCR aides in testing by saving the data into a folder so that it limits API calls.