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 - Ringo & Jasmine #17

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

Earth - Ringo & Jasmine #17

wants to merge 26 commits into from

Conversation

jasyl
Copy link

@jasyl jasyl 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 started with the end points conversations.list, users.list, chat.postMessage. and explored from there checking scopes and methods. Reinforcing the idea of what the base URL is along with the query parameters. Testing things in postman before coding to understand inputs and outputs.
Give a short summary of the request/response cycle. Where does your program fit into that scheme? Our program is the client that makes a request to the slack API. We get a response from the API that we then interpret and manipulate into something readable for the user of our program. For example, when we call User#list or Channel#list we're making a GET request to the Slack API that returns information about all users or all channels of the Workspace.
How does your program check for and handle errors when using the Slack API? The program will raise a SlackApiError if the response back from the API doesn't have true for the value of 'OK'.
How did the design and organization of your project change over time? We maintained the general design of the classes that we created at the beginning of the project. We did move around a few methods that made sense elsewhere. We also found places where we implement helper methods.
Did you use any of the inheritance idioms we've talked about in class? How? We implemented template methods(details & list) inside a abstract class (recipient). That way the child classes had methods with the same signature that accomplished different things based on the attributes of those classes (polymorphism).
How does VCR aid in testing a program that uses an API? You don't have to call the API every time. With VCR, the request gets saved into a cassette that gets replayed every time you run the tests.

jasyl and others added 26 commits October 5, 2020 19:05
@jmaddox19
Copy link

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 ✔️ For all of these, there is a caviet that I needed to make the change to user.rb to get the program to run successfully.
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

Additional Feedback

Great work!! Y'all made a whole program that interacts with Slack in real time from the command terminal!!
I hope y'all have gotten to celebrate and appreciate how fun of an accomplishment that is!!

As I mentioned in an inline comment, an area for growth is to run the program manually to test it out as well, in addition to running the tests. Unless the workspace you were experimenting with actually had all users with all of those optional fields defined, it should fail to run in its current state. That's what happened to me when I ran it against the C14 workspace. It is possible to have fairly thorough tests that all pass and still see the product break when you try to run it end-to-end.

But again, great work! The program all works together as expected and the code is very readable!

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

puts "Please select user or channel, first."
end

when "customize bot"

Choose a reason for hiding this comment

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

Fun addition!


def initialize(slack_id, username, real_name, status_text, status_emoji)

raise SlackApiError if [slack_id, username, real_name, status_text, status_emoji].include? nil

Choose a reason for hiding this comment

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

When I try to actually run the program by calling the main method in slack.rb, the program fails because not all users have all of these attributes. The only one that all users have is slack_id.

In the midst of TDD, it's important to still manually run your code to make sure it all works together for the user, rather than relying only on the tests.

Choose a reason for hiding this comment

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

Suggested change
raise SlackApiError if [slack_id, username, real_name, status_text, status_emoji].include? nil
raise SlackApiError if slack_id.nil?

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