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

TEAM GLITZY- Tanja, Jan, Severin, and Amy #65

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

Conversation

ayjlee
Copy link

@ayjlee ayjlee commented Oct 27, 2017

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? Task leads came up with work plan, delegated assignments. We group planned on design of MVC and database schema, and had a week 1 goal of completing functionality and tests for Guests, and week 2 goal for finishing Merchant functionality and tests. Tasks were divided into pair programming or solo assignments depending on difficulty, and interconnectedness, as well as individual strengths and goals.
How did your team utilize git to collaborate? We primarily used git branches and pull requests.
What did your group do to try to keep your code DRY while many people collaborated on it? We left time at the end to identify as a group areas for refactoring, then divide up refactoring tasks to DRY up code.
What was a technical challenge that you faced as a group? Deploying to Heroku utilizing Paperclip and AWS authentication keys, checking for dependencies between gem versions before deploying, handling mismatches between local schema and Heroku schema in case of dropped migrations.
What was a team/personal challenge that you faced as a group? At various points, team members faced frustrating episodes of learning how to use git better, the team rallied together to make sure that no one was left behind because everyone having a functional version of the repository was essential to moving forward.
What could your team have done better? As a group, coming to a consensus about overall design and wireframes earlier in the process to avoid ambiguity and empower people to move forward style decisions based on the frameworks given.
What was your application's ERD? (include a link) https://github.com/ayjlee/betsy/blob/master/erd.pdf
What is your Trello URL? https://trello.com/b/8aCdTueZ/glitzy
What is the Heroku URL of your deployed application? https://glitzy-store.herokuapp.com

s-wigg and others added 30 commits October 20, 2017 11:33
…m non-logged in users cannot update or delete a review
…been updated to include user info and status name(used to order_one now pending_order; order_two now paid_order)
ayjlee and others added 28 commits October 27, 2017 00:32
…oduct show page, and updated the order of table columns
@CheezItMan
Copy link

bEtsy

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage with all members contributing Good number of commits and a large number of commit messages.
Answered comprehension questions Check, I'm glad branching helped and that you pulled together to keep everyone up.
Trello board is created and utilized in project management It looks like you got most of what you wanted done!
Heroku instance is online Check, nice
General
Nested routes follow RESTful conventions Good RESTful routes where appropriate. You do have some unused routes still in your routes.rb file, like the users index action.
OAuth used for User authentication Check
Functionality restricted based on user roles As a merchant I can, by changing the URL edit and change another merchant's product. See here. The use of a filter would help ensure this.
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, but I can add a product twice allowing me to check out with more item than were in inventory.
Cannot order products that are out of stock Check
Changes order state Check
Clears current cart Check
Database
ERD includes all necessary tables and relationships The ERD isn't using standard notation, but has the models used and their relationships.
Table relationships Relationships present.
Models
Validation rules for Models Check, I like how you used the constant to list the possible order status states. I also like how you add validations for the product images with PaperClip.
Business logic is in the models A good amount of business logic is in the models.
Controllers
Controller Filters used to DRY up controller code Good use of the render_404 method and filters like find_user and current_order. Some more DRYing up could be done however, like in the orders controller.
Testing
Model Testing Lots of testing. I left some notes in the code
Controller Testing Left some notes in your code. Good overall, you need to do more checks of status codes and flash messages.
Session Testing Good session testing!
SimpleCov at 90% for controller and model tests Almost exactly 90%
Front-end
The app is styled to create an attractive user interface Very nicely done.
The app layout uses Foundation's Grid Somewhat responsive. Some pages like fulfillment have issues with small sizes
Content is organized with HTML5 semantic tags Some areas (product index and order index) still have divs where semantic tags would do better.
CSS is DRY Some DRYing could be done, color layouts etc.
Overall Very nicely done. You had almost all the functionality and a few bugs, but overall outstanding work.

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.

Left you some notes, overall nicely done!

food_category.products.first.must_be_kind_of Product

food_category.products << products(:tears)
food_category.products.count.must_equal 2

Choose a reason for hiding this comment

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

you should have a food_category.save here ensuring that no validations etc get broken.


it "requires that quantity is a positive integer " do
item1.quantity.must_be_kind_of Integer
item1.quantity.must_be :>, 0

Choose a reason for hiding this comment

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

These two seem redundant.

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

Choose a reason for hiding this comment

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

No test for available? ???

Remember to test custom methods.


describe "validations" do

it "an order requires a status" do

Choose a reason for hiding this comment

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

I would also test if status is nil.

end
end

describe "custom model methods" do

Choose a reason for hiding this comment

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

Shouldn't the total_cost method be tested in here as opposed to outside?

delete order_item_path(soap2.id)
#Assert
OrderItem.find_by(id: soap2.id).must_equal soap2
must_redirect_to order_path(paid.id)

Choose a reason for hiding this comment

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

Also some flash error message and status code should be tested.

let(:merchant_order_item) {order_items(:orderitem4)}

it "ensures a paid order_item's default shipping status is: not shipped" do
login(merchant)

Choose a reason for hiding this comment

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

Why login?

puts "order status is #{order.status}"
order.status.must_equal "incomplete"
patch place_order_path, params: order_data
order.status.must_equal "incomplete"

Choose a reason for hiding this comment

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

There should also be a status code and flash message to check.


describe "edit" do
it "will get the edit form for an existing product" do
get edit_product_path(Product.first)

Choose a reason for hiding this comment

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

What about if the user isn't logged in!!!! OMG! Or if the user isn't the owner!

describe "update" do

it "updates with valid data and an existing product " do
product = products(:chocolate)

Choose a reason for hiding this comment

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

Ditto!

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