-
Notifications
You must be signed in to change notification settings - Fork 31
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
Slack CLI - Fire - Gessica and Noor #10
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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. | I initially thought your assignment was incomplete because you are missing appropriate error handling code! see comments |
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. |
✔️ see comments! |
Practices clean code. lib/slack.rb only interacts with Workspace to show a separation of responsibilities. Complex logic is broken into smaller helper methods. |
✔️ I think you did OK but see comments |
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 ) |
couldn't find it |
Practices git with at least 15 small commits and meaningful commit messages | only 10 :'( |
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 | encountered a crash |
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 |
Additional Feedback
This is a fairly solid program, but I do want to highlight something important here: the biggest problem that this submission has is the lack of error handling! It caused several knock-on errors that would have been pretty easy to fix but instead dragged down your grade on an otherwise really good submission.
.floo | ||
.flooignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!!!!
require 'httparty' | ||
require 'dotenv' | ||
|
||
Dotenv.load | ||
|
||
BASE_URL_CONVERSATIONS = 'https://slack.com/api/conversations.list' | ||
BASE_URL_USERS = 'https://slack.com/api/users.list' | ||
BASE_URL_CHAT = 'https://slack.com/api/chat.postMessage' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty big deviation from the design you were asked to use! Most of the following comments talk about how this was expected to be in the original design!
return response["channels"] | ||
end | ||
|
||
def self.chat(message,channel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can just live in recipient as is! no changes! :D
@@ -25,5 +27,7 @@ | |||
} | |||
|
|||
# Don't leave our token lying around in a cassette file. | |||
|
|||
config.filter_sensitive_data("<SLACK_API_TOKEN>") do | |||
ENV["SLACK_API_TOKEN"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think calling this SLACK_API_TOKEN
instead of SLACK_TOKEN
is fine in principle, but I also think it might have scuttled the results of your tests in Learn.
def initialize | ||
@users = User.list_users | ||
@channels = Channel.list_channels | ||
@selected = nil | ||
end | ||
|
||
def select_user(user_input) | ||
@users = User.list_users | ||
@selected = @users.find {|user| user.user_name == user_input || user.slack_id == user_input} | ||
end | ||
|
||
|
||
def select_channel(user_input) | ||
@channels = Channel.list_channels | ||
@selected = @channels.find {|channel| channel.name == user_input || channel.slack_id == user_input} | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just popping in to say that this is really lovely to read. :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Devin :)
when "1", "list users" | ||
tp workspace.users, :real_name, :user_name, :slack_id | ||
when "2", "list channels" | ||
tp workspace.channels, :name, :topic, :member_count, :slack_id | ||
when "3", "select user" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a touch more readable!
when "1", "list users" | |
tp workspace.users, :real_name, :user_name, :slack_id | |
when "2", "list channels" | |
tp workspace.channels, :name, :topic, :member_count, :slack_id | |
when "3", "select user" | |
when "1", "list users" | |
tp workspace.users, :real_name, :user_name, :slack_id | |
when "2", "list channels" | |
tp workspace.channels, :name, :topic, :member_count, :slack_id | |
when "3", "select user" |
Etc!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I'm confused, I didn't understand what you meant by Etc
print "Please enter a channel's name or ID:" | ||
selection_option = gets.chomp | ||
selected = workspace.select_channel(selection_option) | ||
if selected | ||
puts "Channel found" | ||
else | ||
puts "Channel not found" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might even consider taking these longer pieces and putting them in helper methods!
describe "show details" do | ||
it "shows details selected user/channel" do | ||
VCR.use_cassette(" workspacce channels") do | ||
workplace = Workspace.new | ||
|
||
expect(workplace.selected).must_be_nil | ||
|
||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No test for sending messages?
begin | ||
puts workspace.show_details | ||
rescue WorkspaceError => error | ||
puts error.message | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
begin
and rescue
should also be used on anything that sends a message to slack, to better handle those errors!
Assignment Submission: Slack CLI
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection