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

Ampers - Kate, Steffany, Brittany, Lily #22

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

Conversation

lillers1122
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? Originally we broke the work up by files, and then as we got further along we broke it up by features.
How did your team utilize git to collaborate? Quite a lot, as you may see...Lily was especially confused 💯 and says that git was a big area of learning in positive ways.
What did your group do to try to keep your code DRY while many people collaborated on it? We reviewed each others code each time before we merged a branch to master. Agreement all around for happiness using git and much appreciation to Kate for holding our hands through the process.
What was a technical challenge that you faced as a group? Git. Also routes, database, and Heroku...currently category-products table doesn't seem to be migrated everywhere? Like for Heroku and on some of our computers....huh.
What was a team/personal challenge that you faced as a group? Git. How to split up/delegate work. Communication. Difficulty getting in sync.
What could your team have done better? Design early. Database set up....blind leading the blind problems.
What was your application's ERD? (include a link) https://www.lucidchart.com/documents/edit/25af7958-b9f1-4ec2-8029-8eecbe39297a?shared=true&
What is your Trello URL? https://trello.com/b/rAMH4ln9/petsy
What is the Heroku URL of your deployed application? https://petsy-app-ampers.herokuapp.com/
Alternate Heroku URL (re-deployed) https://warm-thicket-59782.herokuapp.com/
Other https://docs.google.com/document/d/1z5tjCLFFtG8QXeCV2OBwYyvX45KUJiuPs-fZ5arT6Ac/edit

brittanyrjones and others added 30 commits April 21, 2018 10:20
…troller so that a new pending order is created for each new guest
…oducts

Feature_Adds more db seeds and db updates
steffnay and others added 28 commits April 26, 2018 22:11
…ugh I did make one for self.merchant_products
Category controller tests + new form
hotfix for showing mechant shops while logged out
changed new method in product controller
@CheezItMan
Copy link

CheezItMan commented May 4, 2018

bEtsy

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage with all members contributing Check
Answered comprehension questions Check
Trello board is created and utilized in project management Check
Heroku instance is online You have two, but neither is working properly
General
Nested routes follow RESTful conventions check
oAuth used for User authentication DOES NOT WORK on Heroku, but I was able to get it to work on my localhost.
Functionality restricted based on user roles NOT IMPLEMENTED
Products can be added and removed from cart Yes, but I notice that simply adding an item to my cart changes the quantity available. So if I add an armadillo to my cart, the selling can never sell it until I checkout, or clear my cart?
Users can view past orders I can't even see my cart after adding an item and browsing away.
Merchants can add, edit and view their products I added a pet rock to the store, but I can't edit the product and it doesn't show up on my merchant profile.
Errors are reported to the user For product, it just says, "Could not create ...." There are similar issues with category.
Order Functionality
Reduces products' inventory Simply adding it to a cart reduces inventory... Also there's no way for me to see my cart and clear items.
Cannot order products that are out of stock Check
Changes order state Check
Clears current cart No way to perform this action
Database
ERD includes all necessary tables and relationships I don't have access to your ERD Diagram.
Table relationships You have them in the models
Models
Validation rules for Models Check
Business logic is in the models Well done here
Controllers
Controller Filters used to DRY up controller code You have some filters, but nothing to enforce security.
Testing
Model Testing Categories has almost no testing, You have some tests for custom methods in order_item_test.rb, but no tests for validations. product_test is a little better. Overall model testing is really skimpy. You should know better by now!
Controller Testing This is also problematic. You are not testing guest vs authenticated users and have a number of broken tests. I have inline notes on the testing.
Session Testing See my inline notes
SimpleCov at 90% for controller and model tests Probably related to the light testing, there is a serious lack of coverage here.
Front-end
The app is styled to create an attractive user interface The site looks nice and has responsive features, nice!
Content is organized with HTML5 semantic tags Good semantic HTML
Overall Note, your deployed sites won't let me log in. You're using the wrong OAuth key for one of your apps, as it redirects me to localhost. You also have serious issues with your controller tests. You haven't tested for authenticated users vs guest users at all and you have a number of errors/failing tests. I encourage you all to review controller testing and work on it a bit in API-Muncher if you can, or at least review the tests you've written here so that you have a better idea on what you're missing. You also have a lot of broken functionality, and missing features. As a guest I can't even find my cart. Your team obviously learned a lot and there's a lot of work here, but you didn't manage to demonstrate a mastery of the learning goals which include using OAuth, testing code and working together to build the features including an online cart. Reviewing these features would be a good thing to spend some time on and I'm happy to sit with any of you and look at them again.

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

end

describe "new" do
it "succeeds" do

Choose a reason for hiding this comment

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

Shouldn't you be making sure that guest users can't create a new category? Guests shouldn't be able to get to the form.

end
end

describe "create" do

Choose a reason for hiding this comment

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

You also need to verify that guest users can't create categories, so you're missing some tests.

@@ -0,0 +1,38 @@
class Product < ApplicationRecord

Choose a reason for hiding this comment

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

Why is this model in the test/controllers folder?


describe "new" do
it "succeeds" do
user = users(:ada)

Choose a reason for hiding this comment

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

Guest users shouldn't be able to get to the form to create a new product.


describe "create" do
it "creates a product with valid data" do
Product.count.must_equal 3

Choose a reason for hiding this comment

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

You also haven't ensured that people who haven't logged in can create products.


describe "show" do
it "succeeds" do
get product_path(products(:cat))

Choose a reason for hiding this comment

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

This test fails because it can't find the merchant.

proc {
put order_path(order.id), params: { order: order_info }
product = Product.find(item.product_id)
}.must_change 'product.stock', -2

Choose a reason for hiding this comment

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

This test is failing, probably because it's not creating an orderItem properly.

@@ -0,0 +1,76 @@
class ProductsController < ApplicationController

Choose a reason for hiding this comment

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

In these controllers you have no security. Nothing prevents guest users from getting into mischief.

<div class="body-wrapper">
<header class="clearfix">
<section class="site-title">
<h1>Welcome To Petsy</h1>

Choose a reason for hiding this comment

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

Shouldn't this header be a link to the homepage.

</head>

<body>
<div class="body-wrapper">

Choose a reason for hiding this comment

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

What purpose does the body-wrapper div element perform. Why not just style the body element?

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