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

J'All (Jocelyn Gonzalez, Averi Kitsch, Laura Robertson, Lauren Cardella) -- Carets #60

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

Conversation

enigmagnetic
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 did a lot of the initialization of models, controllers, and database migrations as a group. We identified tasks that were more suitable for pairing and those more suited for individual work, and assigned them primarily by interest in working on those tasks.
How did your team utilize git to collaborate? Locally each team member used branches for features, which were first merged to master locally before being pushed to the remote master. Merge conflicts were handled individually as needed.
What did your group do to try to keep your code DRY while many people collaborated on it? At first we focused on getting functionality, then once there was a good chunk of work done with tests, some of the team reviewed others' code and refactored. We did this process a few times before project completion.
What was a technical challenge that you faced as a group? Figuring out how to build the routes and validate the models for order creation and order_product creation took us a couple of days as a team to conceptualize. It was difficult because an order needs to have an order product, but an order product needs to have an order id, to be initialized. We had to decide which needed to be created first (order), how the routes should work, and how to make sure that an order could not be submitted/saved in the database without all of its necessary attributes.
What was a team/personal challenge that you faced as a group? How to work individually on elements that should look or work cohesively, i.e. HTML structure for the pages, and routes that rely on other parts of the application to work. Also, Lauren found the order fulfillment page challenging in getting the correct model data for each user, which Averi was able to help review and refactor for a cleaner, more robust solution.
What could your team have done better? We could have focused more on the 'correct' workflow of testing first, which we did not do a great job in the first phase of the project. We did some pair test-building after the first sprint to fill out the tests. However, we are generally very pleased with the pace we were able to keep and the product we made.
What was your application's ERD? (include a link) https://www.lucidchart.com/documents/edit/744a2e8c-59ae-4f39-851a-9c803303fa42?shared=true&
What is your Trello URL? https://trello.com/b/Lzr1zUP5/spooksy
What is the Heroku URL of your deployed application? http://spooksy.herokuapp.com/

LauraAddams and others added 30 commits October 20, 2017 12:55
…ontroller tests for edit, update, and delete. Add edit action to order_products_controller. Edit action test passing, other tests failing.
@CheezItMan
Copy link

bEtsy

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage with all members contributing Good number of commits and good messages. I like how you used branching.
Answered comprehension questions Check, it sounds like things went well.
Trello board is created and utilized in project management Check, it look like you accomplished almost everything you planned to do.
Heroku instance is online Check
General
Nested routes follow RESTful conventions RESTful routes where appropriate
oAuth used for User authentication OAuth is working.
Functionality restricted based on user roles Check, can't edit a product that doesn't belong to you. I nearly died laughing when I tried to edit a category I didn't have access to, "uh-oh, looks like a ghost.."
Products can be added and removed from cart Check
Users can view past orders Check
Merchants can add, edit and view their products Check
Errors are reported to the user Check
Order Functionality
Reduces products' inventory Check
Cannot order products that are out of stock Check, not even if I try to add it twice
Changes order state I click on the shipped link, but the order stays status, paid and not complete.
Clears current cart Check
Database
ERD includes all necessary tables and relationships Check
Table relationships Check
Models
Validation rules for Models Nice custom validations as well.
Business logic is in the models The average review should be done in the model.
Controllers
Controller Filters used to DRY up controller code Good use of find_user and categories_all, also good use of the render_404 method as well as find_op
Testing
Model Testing Lots of model testing here. Good. I left a few note in the code.
Controller Testing Nice work, I left a few notes in the code
Session Testing Good standard session testing, no problems that I noticed.
SimpleCov at 90% for controller and model tests 99% great!
Front-end
The app is styled to create an attractive user interface The app looks really good. I love the ghost image.
The app layout uses Foundation's Grid It's a responsive site and looks pretty good at a variety of screen sizes and on my phone.
Content is organized with HTML5 semantic tags Pretty good semantic HTML
CSS is DRY Some DRYing up could be done, but not a lot.
Overall This is a pretty outstanding project. Y'all should be proud. Make sure to add this to your portfolios and if asked about important projects you've worked on, show them this. Very nice!

IMPORTANT: Whoever submitted the PR (and thus will get the notification about this feedback) should share this with their teammates.

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

Very nice work, a few comments below.

def cancel
if find_user
if @order_product.product.user.id == session[:user_id]
if !@order_product.shipped

Choose a reason for hiding this comment

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

This looks like a good candidate of content to move to the model.

describe Category do
let(:category) { Category.new }
describe "validations" do
it "category without name is invalid" do

Choose a reason for hiding this comment

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

It's also useful to verify that category.errors.keys.must)include(:name)

end

it "doesn't create a new category with different capitalization" do
proc {Category.new(name: "PoTions")}.must_change 'Category.count', 0

Choose a reason for hiding this comment

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

You're missing a save here!

cat = Category.first
cat.must_respond_to :products
cat.products << prd
cat.products[0].must_equal products(:one)

Choose a reason for hiding this comment

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

Just to make this more generic I would use:

cat.products[0].must_include products(:one)

@order.valid?.must_equal false
end

["09/18","11/20","9/18"].each do |num|

Choose a reason for hiding this comment

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

It would be a good idea for a generic test that uses the current date rather than hard-coded dates.

@@ -0,0 +1,158 @@
require "test_helper"

describe OrderProductsController do

Choose a reason for hiding this comment

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

There are some negative tests you're missing here. Like if the item being shipped or deleted is already gone.

end

describe "order show" do
it "should get show if logged in" do

Choose a reason for hiding this comment

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

Good testing here, but this looks like 3 tests not one.

must_respond_with :not_found

end
it "should get show" do

Choose a reason for hiding this comment

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

Some of your it blocks should be more specific, like this one which should say, "will redirect guest users on show actions."


Order.last.status.must_equal 'pending'

put order_path(Order.last), params: {order:

Choose a reason for hiding this comment

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

Given the two arrays above you could use a loop to set up this hash.

must_respond_with :success
end

it "can get found" do

Choose a reason for hiding this comment

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

This test makes no sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants