Skip to content

DeadGhost Betsy#15

Open
secretsharer wants to merge 291 commits intoAda-C7:masterfrom
secretsharer:master
Open

DeadGhost Betsy#15
secretsharer wants to merge 291 commits intoAda-C7:masterfrom
secretsharer:master

Conversation

@secretsharer
Copy link
Copy Markdown

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
@kariabancroft
Copy link
Copy Markdown

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
Copy link
Copy Markdown

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
Copy link
Copy Markdown

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
Copy link
Copy Markdown

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
Copy link
Copy Markdown

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
Copy link
Copy Markdown

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)
Copy link
Copy Markdown

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
Copy link
Copy Markdown

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
Copy link
Copy Markdown

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
Copy link
Copy Markdown

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>
Copy link
Copy Markdown

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