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

Caroline, Analisa, Wini, and Wenji - Betsy - Octos #28

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

Conversation

cmn53
Copy link

@cmn53 cmn53 commented Apr 27, 2018

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? The first week we focused on getting the model and validations set up. We worked on the overall design and building the model together and then divvied up tasks based on what we each wanted to work on and which tasks we thought we needed to pair on, namely the cart functionality and the merchant's role.
How did your team utilize git to collaborate? We mostly worked on branches when we were on our own -- we didn't adhere as well to this towards to the end. We generally tried to merge our code in the morning after standup and then at the the end of the day. Sometimes it was necessary to share in smaller time increments, but we maintained communication about when we would be pushing.
What did your group do to try to keep your code DRY while many people collaborated on it? This is an area that we could have improved on. We all wrote tests for code that we didn't write ourselves, and that created some opportunities for identifying redundant code.
What was a technical challenge that you faced as a group? We had some issues with what happens when you create a cart and do not checkout. This sometimes resulted in duplicate orders. We also don't feel that confident about session or using params in controller tests.
What was a team/personal challenge that you faced as a group? One challenge we initially had was being clear about assigning tasks, rather than asking what people want to work on. Another challenge was that we all had pretty strong opinions and had to find a way to compromise.
What could your team have done better? More testing, more often.
What was your application's ERD? (include a link) https://www.lucidchart.com/documents/edit/aac9add2-6343-400b-a6ef-f074d35334e7/0
What is your Trello URL? https://trello.com/b/2N6aZM18/ali-bon-bon
What is the Heroku URL of your deployed application? [email protected]

cmn53 and others added 30 commits April 21, 2018 11:47
…acon grease. Unpredictable and messy. Switching to finish Reviews Controller methods new and create
…ests are not passing. Need another set of eyes on it. Still working on homepage
… one, and if there was no cart session to create a ner cart
… not created tests for them yet. Created view helper for displaying prices and totals in dollars
…o links so that the orbital reloads upon redirect
@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 see inline
oAuth used for User authentication yes
Functionality restricted based on user roles mostly - I was able to access the edit page for a product I don't own (but could not update the product)
Products can be added and removed from cart yes
Users can view past orders yes
Merchants can add, edit and view their products yes
Errors are reported to the user mostly - there were a few cases where this did not happen
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's a couple places where things could be cleaned up, but in general you're making very good use of model methods!
Controllers
Controller Filters used to DRY up controller code yes
Testing
Model Testing mostly
Controller Testing mostly
Session Testing yes
SimpleCov at 90% for controller and model tests yes - good work!
Front-end
The app is styled to create an attractive user interface yes - great work!
Content is organized with HTML5 semantic tags yes
CSS is DRY yes
Overall Great work overall! You've built a fully functional web store from top to bottom, everything but payment processing. This represents a huge amount of work, and you should be proud of yourselves! Your code is pretty well organized all the way from the CSS to the database, and I am quite happy with this submission. There are definitely places where things could be cleaned up or refined, or where there are feature gaps you might like to fill - I've tried to outline some of these below. However 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.

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


get 'products/by_name', to: 'products#by_name', as: 'product_by_name'
resources :products, only: [:index, :edit, :show, :new, :create, :update]
resources :products, only: [:index, :show]

Choose a reason for hiding this comment

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

Why do you have resources :products twice here?

patch "/my_orders/:id/change_status" , to: "orders#change_status", as: "change_status"

get "/merchants/:id/show_products", to: "merchants#show_products", as: "show_products"

Choose a reason for hiding this comment

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

How is line 26 different from the display route above on line 10?


resources :merchants do
resources :categories, only: [:index, :new, :create, :show]
end

Choose a reason for hiding this comment

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

Do the category routes need to be nested here?

get 'merchants/by_name', to: 'merchants#by_name', as: 'merchant_by_name'
resources :merchants do
resources :products, only: [ :new, :create, :edit, :update]
end

Choose a reason for hiding this comment

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

None of these product routes should probably be nested. For new and create you should take the merchant ID from the session, rather than requiring the user to have it in the route, and for edit and update you can get the merchant ID from the product ID (making sure to check it against the one currently in the session).

end

patch "/products/retire/:id", to: "products#retire", as: "retire"
get "/merchants/:merchant_id/display-products", to: "merchants#display", as: "display"

Choose a reason for hiding this comment

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

Line 9 should probably be /products/:id/retire

it "renders forbidden and does not update the DB when product belongs to the merchant" do
product = Product.first
merchant = product.merchant
login(merchant)

Choose a reason for hiding this comment

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

What if you're logged in as someone other than the seller of this product?

if @order.save
@cart.cartitems.each do |cartitem|
product = Product.find(cartitem.product_id)
product.stock = product.new_stock(cartitem.quantity)

Choose a reason for hiding this comment

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

All of this work should probably be in a model method on Order.


flash.now[:status] = :failure
flash.now[:result_text] = "Could not place your order"
flash.now[:messages] = @order.errors.messages

Choose a reason for hiding this comment

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

There are many places in this app where you have code similar to this, setting the status to :failure and adding :result_text and :messages. Could you make this a helper method in the ApplicationController, something along the lines of:

def report_failure(result_text, model=nil, current_request: true)
  target = current_request ? flash.now : flash
  target[:status] = :failure
  target[:result_text] = result_text
  if model
    target[:messages] = model.errors.messages
  end
end

Then here you could call it with report_failure("Could not place your order", @order)

Copy link

Choose a reason for hiding this comment

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

That's a great idea!

before_action :find_order, only: [:update, :edit, :show, :my_order, :change_status]

before_action :find_cart_by_session, only: [:new]
before_action :find_cart_by_order, only: [:update, :show]

Choose a reason for hiding this comment

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

Good use of filters to keep this controller DRY.

def create
if session[:cart_id]
@cart = Cart.find_by(id: session[:cart_id])
@cartitem = Cartitem.new(cartitem_params)

Choose a reason for hiding this comment

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

This method is not very DRY. I would try and separate the cart creation logic out, something like this:

def create
  @cart = Cart.find_by(id: session[:cart_id])
  unless @cart
    # Create the cart or return an error
  end

  # If we get to this point then we definitely have a cart
  # stored in @cart

  @cartitem = @cart.cartitems.new(...)
  # Save the cartitem or return an error...
end

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