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

Emilce, Jamila, Brenda, & Luxi - pEtsy - Octos #20

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

Conversation

Lindseyls
Copy link

bEtsy

Congratulations! You're submitting your assignment! These comprehension questions should be answered by all members of your team, not by a single teammate.

Comprehension Questions

Question Answer
How did your team break up the work to be done? We set the baseline together and set individual tasks at the beginning. Then, because some of the tasks were a bigger responsibility than others, we split up into two pairs to work on tasks together.
How did your team utilize git to collaborate? Each time we created and worked on our own branches to not interfere with master. We made to to merge all of our branches together multiple times a day.
What did your group do to try to keep your code DRY while many people collaborated on it? We used as many helper methods as possible to DRY up our code.
What was a technical challenge that you faced as a group? Git merging, calling objects through model relationships, design of the whole project, and sessions.
What was a team/personal challenge that you faced as a group? It was hard to determine the strength that each team members had so that we knew how to distribute the work. Also, knowing how to be an effective standup leader and task manager.
What could your team have done better? Executing the plan for the day or the whole project could have been done better.
What was your application's ERD? (include a link) https://www.lucidchart.com/documents/edit/a78c26bc-a1ca-4087-bd00-e54c6784307f/0
What is your Trello URL? https://trello.com/b/lIaYfLVi/petsy
What is the Heroku URL of your deployed application? http://petsybetsy.herokuapp.com/

emilcecarlisa and others added 30 commits April 23, 2018 09:38
…e. Updated the controller and model for review
@droberts-sea
Copy link

bEtsy

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage with all members contributing yes
Answered comprehension questions yes
Trello board is created and utilized in project management yes
Heroku instance is online yes
General
Nested routes follow RESTful conventions yes, for the most part - good work
oAuth used for User authentication yes
Functionality restricted based on user roles
Products can be added and removed from cart yes
Users can view past orders yes
Merchants can add, edit and view their products no
Errors are reported to the user mostly, but in several places it failed silently
Order Functionality
Reduces products' inventory yes
Cannot order products that are out of stock no
Changes order state yes
Clears current cart yes
Database
ERD includes all necessary tables and relationships yes
Table relationships yes
Models
Validation rules for Models yes
Business logic is in the models yes - There are a couple places where logic should be moved down to the model, but in general this looks pretty good.
Controllers
Controller Filters used to DRY up controller code you've missed many opportunities to do this - see inline
Testing
Model Testing yes
Controller Testing Tests for basic CRUD operations are there, but you're missing test cases for many of the more interesting interactions. See inline comments.
Session Testing yes
SimpleCov at 90% for controller and model tests no
Front-end
The app is styled to create an attractive user interface some - feels like you ran out of time for this somewhat
Content is organized with HTML5 semantic tags yes
CSS is DRY yes
Overall Good work overall. You've built a fully functional web store from top to bottom, everything short of payment processing. This represents a huge amount of work, and you should be proud of yourselves!

Most of the issues I see fall into two categories. The first is places where there are bugs or missed features because you ran out of time. This is more-or-less expected on this project, and not a cause for concern. The second is around code organization and programming fundamentals (including testing). Keeping the fundamentals up on a big project like this is not easy, but the scale makes it that much more important, and this is definitely something to focus on in future projects. I've tried to outline as many problems as I could find below.

bEtsy is a huge project on a very short timeline, and this feedback should not at all diminish the magnitude of what you've accomplished. Keep up the hard work!

Only the person who submitted the PR will get an email about this feedback. Please let the rest of your team know about it.

<%= f.text_area :cc_cvv %>

<%= f.label :bill_zip, "BILLING ZIP CODE" %>
<%= f.text_area :bill_zip %>

Choose a reason for hiding this comment

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

Why are these text_areas? You don't need the extra space, and this will prevent the form from submitting with <enter>.

<div class="container">

<h2>Merchant: <%= link_to @product.user_id, user_path %></h2>

Choose a reason for hiding this comment

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

This link points to the wrong place - since you don't pass a parameter to user_path, it takes the ID from the current page, but that's the product ID. Instead you probably want: link_to @product.user.name, user_path(@product.user)

def new
if session[:user_id]
@product = Product.new(user_id: params[:user_id])
else

Choose a reason for hiding this comment

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

On line 16 you check for the user ID in the session, but on line 17 you try to pull it out of the params. This means that the product isn't getting assigned to the user correctly, because params[:user_id] is nil, which breaks a lot of the other functionality of the site.

def create
@category = Category.new(category_params)
@category.save
redirect_to users_path

Choose a reason for hiding this comment

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

You don't check the return value of save here. What if the user entered a blank category name and it fails your validations?

Choose a reason for hiding this comment

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

You're also not checking that the user is logged in here, which means that an unauthenticated user with a tool like Postman could create as many categories as they want.

def new
if current_user
@category = Category.new
else

Choose a reason for hiding this comment

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

Instead of checking that the user is logged in manually, you should use a controller filter like we did in class. That will both help keep this code DRY, and prevent you from accidentally letting a user do something they shouldn't (like with create below)

it "sends success if the order exists" do
order = Order.first
orderitem_data = { product_id: Product.first.id, quantity: Product.first.stock, order_id: order.id }

Choose a reason for hiding this comment

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

You should probably have tests for both adding a new product to the cart, and adding a product that's already in the cart (update the quantity)


it "does not procees the order if the customer data is incomplete" do
orderitem = {product_id: Product.first.id, quantity: Product.first.stock}
post order_items_path, params: {order_item: orderitem}

Choose a reason for hiding this comment

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

You should also test that it doesn't go through if orderitem quantities are invalid, and that it reduces the stock of each of those products.


def product_params
params.require(:product).permit(:name, :stock, :price, :description, :pet_type, :photo_url, :user_id, category_ids: [])
end

Choose a reason for hiding this comment

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

You should take the user ID from the session, not from the form data. That would allow a logged-in user to add products for some other user, or even to change which user a product is associated with.


describe "show" do
it 'sends success if the product exists' do
get product_path(Product.first)

Choose a reason for hiding this comment

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

Missing tests for edit, update and destroy. These are particularly interesting ones, since there are 3 key test cases around authorization:

  • Guest user
  • Wrong user logged in
  • Right user logged in

describe "create" do

it "it won't create a review with bogus data" do
product = Product.create(name:"cat rug",price: 10, user: users(:one), stock: 15)

Choose a reason for hiding this comment

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

What if you try to create a review when logged in as that product's owner?

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.

5 participants