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

DeadGhost Betsy #15

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

DeadGhost Betsy #15

wants to merge 291 commits into from

Conversation

secretsharer
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 used pairing, slack for homework, and pivoted on TDD, organically course correcting as needed.
How did your team utilize git to collaborate? Branching, merging to local master, also merging separately to remove master. Git can die in the fires of hell.
What did your group do to try to keep your code DRY while many people collaborated on it? At all times. We tried. We discussed it a great deal. Success? hmmm
What was a technical challenge that you faced as a group? SESSIONS, remaking working after struggling with git issues.
What was a team/personal challenge that you faced as a group? We had little time over the weekend. Once of us was moving, one was out of town. Time was an issue.
What could your team have done better? We could have made better use of our Trello board. More organized task delegation at times.
What was your application's ERD? (include a link) https://www.lucidchart.com/documents/edit/9af9f68c-0e7b-45b3-9d06-74a9da6b6c44
What is your Trello URL? https://trello.com/b/Ob99wbzQ/ghosty
What is the Heroku URL of your deployed application? https://ghosty-app.herokuapp.com/

acgillette and others added 30 commits April 20, 2017 19:19
ssamant and others added 28 commits April 28, 2017 08:50
…atus and added migration to set order status default to pending
@kariabancroft
Copy link

bEtsy

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage with all members contributing Yes - seems like a good balance
Answered comprehension questions Yes
Trello board is created and utilized in project management Yes (though you mention you could've used it more)
Heroku instance is online Yes
General
Nested routes follow RESTful conventions Yes
oAuth used for User authentication Yes - it seems like at some point you created a table and a model for Sessions, though I don't think you're using it. Should be removed.
Functionality restricted based on user roles Yes
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 Yes
Order Functionality
Reduces products' inventory Yes
Cannot order products that are out of stock Yes
Changes order state Yes
Clears current cart Yes
Database
ERD includes all necessary tables and relationships Bad link in the comments
Table relationships Schema looks good
Models
Validation rules for Models Yes - I like the way you used a constant to validate the Order status
Business logic is in the models Not much - definitely room for improvement to move some controller and view code for aggregation, data filtering, etc into the model
Controllers
Controller Filters used to DRY up controller code Yes
Testing
Model Testing Yes (missing Order)
Controller Testing Yes
Session Testing Yes
SimpleCov at 90% for controller and model tests Yes?
Front-end
The app is styled to create an attractive user interface Yes
The app layout uses Foundation's Grid Yes - some. Though I think you could've used it a bit more across the board.
Content is organized with HTML5 semantic tags Yes
CSS is DRY Yes - not too much
Overall Overall you did a nice job with this. You have more opportunity to focus on minimal controllers with more logic in the models. You had some gaps still in testing, but you did cover the majority of major functionality. Don't forget contrast when you're creating user interfaces for you users. Nice job!


def product_status
product = Product.find_by_id(params[:id])
if !product.status

Choose a reason for hiding this comment

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

This should be simplified by doing product.status = !product.status


private

def review_params

Choose a reason for hiding this comment

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

eek! indentation in this controller needs work


#creates an array of 2-element arrays where the first element is the category name and the second is the category id.
#use this array in the form to create a product in a select helper where the names are what shows up in the view and IDs are the values that get passed to the create/update action
def category_names

Choose a reason for hiding this comment

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

This should come from the category model, not a controller

order = Order.create
session[:order_id] = order.id
end
end

Choose a reason for hiding this comment

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

This controller's indentation also needs work

@@ -0,0 +1,33 @@
class CategoriesController < ApplicationController

Choose a reason for hiding this comment

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

Good job on this one

acceptable.valid?.must_equal true
acceptable.errors.messages.wont_include :name

no_category = categories(:nocategory)

Choose a reason for hiding this comment

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

Seems like this is the duplicate code to above

merchant.valid?.must_equal false
merchant.errors.messages.must_include :username
#merchant with username is good
merchants(:dan).valid?.must_equal true

Choose a reason for hiding this comment

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

These should really be two separate tests. One is testing the validation and the other is testing that it would work


describe OrderItemsController do

it "should create an order item product" do

Choose a reason for hiding this comment

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

Indentation!


it "should create an order" do
proc {
Order.create

Choose a reason for hiding this comment

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

It seems odd that you can create an Order with nothing


<h3 class="centering">Ghosty Products for ghosty needs</h3>

<h3 class="columns large-12 small-12"><%= @product_filter %></h3>

Choose a reason for hiding this comment

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

If you have the diff sizes with the same values its not going to do you much good

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