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

Earth - Kal and Christina #18

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

Conversation

christinaminh
Copy link

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 liked using Postman for exploring API. It was good practice for doing GET and POST requests.
Give a short summary of the request/response cycle. Where does your program fit into that scheme? Our program is the client and Slack is the server. Our program, based on the user’s input, made a specific request to the server belonging to Slack. The server sent back a specific response with a payload, which in the case of a GET request returns data that our program can parse through and return to the user. In the case of a POST request, the client would expect to get a response stating whether or not the action was successful.
How does your program check for and handle errors when using the Slack API? It displays the error message from the API response to the user. To check for API call errors, the program is tested using the tool, VCR.
How did the design and organization of your project change over time? We ended up moving certain methods into classes for which they were each better suited. Since the driver code should only interact with the Workspace class, we called User and Channel methods in the Workspace Class so the driver code did not have additional dependencies.
Did you use any of the inheritance idioms we've talked about in class? How? We made an abstract class, Recipient, and template methods for User and Channel to inherit from, since a user and a channel are a kind of recipient.
How does VCR aid in testing a program that uses an API? Since every real API call has a cost, VCR allows us to make an AP call once, records data requested and returned, and allows us to use that as reference for any testing.

christinaminh and others added 19 commits October 6, 2020 17:53
…he loop. Changed the keyword arguments in Recipient class to positional arguments, which addresses ArgumentError that was raised from initializing user.
…d testing for Workspace#show_details and Workspace#send_message.Filtered slack.rb from SimpleCov.
…sage testing from Workspace to Recipient. Refactored slack.rb to reflect addition of selected attribute to Workspace class
Copy link

@kaidamasaki kaidamasaki left a comment

Choose a reason for hiding this comment

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

Excellent job! There were a few issues around attention to detail regarding the requirements (token names, checking in cassettes) but other than that everything looks great!

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. Did not check in cassette files.
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 ✔️
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

@name = name
end

API_KEY = ENV["SLACK_API_TOKEN"]

Choose a reason for hiding this comment

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

The README said that this should be named SLACK_TOKEN:

Suggested change
API_KEY = ENV["SLACK_API_TOKEN"]
API_KEY = ENV["SLACK_TOKEN"]

end

CHAT_URL = "https://slack.com/api/chat.postMessage"
BOT_API_KEY = ENV["SLACK_BOT_API_TOKEN"]

Choose a reason for hiding this comment

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

You should use a single token for getting and sending.

Suggested change
BOT_API_KEY = ENV["SLACK_BOT_API_TOKEN"]
BOT_API_KEY = ENV["SLACK_TOKEN"]

@@ -0,0 +1,34 @@
require_relative "recipient"

CONVERSATIONS_URL = "https://slack.com/api/conversations.list"

Choose a reason for hiding this comment

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

Good use of a constant!

Comment on lines +51 to +52
settings_json = File.read("bot_settings.json")
settings = JSON.parse(settings_json)

Choose a reason for hiding this comment

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

The bot settings file is a nice touch. 🤖

end

def show_details
raise ArgumentError.new ("No user or channel selected.") unless @selected

Choose a reason for hiding this comment

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

I think I would prefer this to be something like a NoRecipientSelectedError instead of an ArgumentError since nothing is actually wrong with the arguments to this method.

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