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

Water - Iris and Jessica #7

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

Water - Iris and Jessica #7

wants to merge 94 commits into from

Conversation

jwinchan
Copy link

@jwinchan jwinchan commented Oct 9, 2020

Assignment Submission: Slack CLI

Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.

Reflection

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 looked through the different methods and their corresponding documentation. We also played around with Postman to interact with the API. I learned to have a base URL in the get and then just add on endpoints as necessary.
Give a short summary of the request/response cycle. Where does your program fit into that scheme? A client sends a request to a server. The server will then send a response back to the client. In this case, our program is the client where we send get and post requests to the Slack server. Slack's API will then send a response in the form of JSON.
How does your program check for and handle errors when using the Slack API? Our program reads the response JSON and we created a custom SlackAPIError to handle any errors that might pop up.
How did the design and organization of your project change over time? We started coding our methods into slack.rb and workspace.rb but we would refactor each wave so that more of the methods were handled by the User and Channel objects.
Did you use any of the inheritance idioms we've talked about in class? How? We used template methods for send_message, list_all, get, etc. We used polymorphism for the list_all and details method.
How does VCR aid in testing a program that uses an API? In our send message tests, it prevents the program from sending a new message every time we rake. It limits the number of API calls in general so we don't get our tokens revoked. It also prevents any errors from occurring if there was an issue with Slack's server.

Copy link

@beccaelenzil beccaelenzil left a comment

Choose a reason for hiding this comment

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

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. ✔️
Practices clean code. lib/slack.rb only interacts with Workspace to show a separation of responsibilities. Complex logic is broken into smaller 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

Excellent work on this project. This code is well-written and well-tested. It is clear to me that the learning goals around understanding the request/response cycle, consuming an API, and implementing a design using inheritance from scratch were all met.
Furthermore, the user experience of your CLI is fantastic. Keep up the hard work!

| Overall Feedback | Criteria | yes/no |✔️
| --- | --- | --- |
| Green (Meets/Exceeds Standards) | 7+ in Code Review && 3+ in Functional Requirements |
| Yellow (Approaches Standards) | 6+ in Code Review && 2+ in Functional Requirements |
| Red (Not at Standard) | 0-5 in Code Review or 0,1 in Functional Reqs, or assignment is breaking/doesn’t run with less than 5 minutes of debugging |

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

Comment on lines +57 to +59
unless response.code == 200 && response.parsed_response["ok"]
raise SlackAPIError.new("Error: #{response.parsed_response["error"]}")
end

Choose a reason for hiding this comment

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

You use this code a couple places. Consider breaking out this functionality into a helper method.


def initialize
@users = User.list_all
@channels = Channel.list_all

Choose a reason for hiding this comment

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

Consider include an instance variable @selected such that show_details and send_message would not need to take recipient as a parameter but could use the object stored in @selected


# TODO project
puts "Welcome to the Ada Slack CLI!\n\n"

Choose a reason for hiding this comment

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

The user experience is fantastic. Great work breaking this CLI down into helper methods and gracefully handling bad user input.

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