-
Notifications
You must be signed in to change notification settings - Fork 15
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 Puffy Shirts bring you (parens) #37
base: master
Are you sure you want to change the base?
Conversation
Merchant show and edit
Guests controller
Products branch
Merchant show and edit
What helped us stay organized and working together?
What should you have been doing differently?
What would you recommend to C7?
What went well?
What was most challenging?
How do you feel about your project overall?
|
What helped you stay organized and working together? What should you have been doing differently? One piece of advice for c7. What went well technically? What was most challenging? Overall I think we did a mashing job of it. Not only did we make a really cute site, that works, we also laughed together and voiced appreciation for each other along the way. |
Team Work:
Technical:
Overall: |
How did your team work together:
Technical
How do you feel about your project overall? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@livmaria7891 @wilsab30 @ninamutty @miriam-cortes
Hello Puffy Shirts! I've put in comments & a summary in your weekly progress reports. Overall well done. I particularly liked the Cart, Model Validations & custom methods and the styling.
Things to work on:
- Tests (check negative cases especially)
- Drying up Views with View Helpers & Partials
- Working on access control (preventing users from getting to things they shouldn't.
# test "the truth" do | ||
# assert true | ||
# end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also test if the name is set correctly it's valid.
@@ -0,0 +1,48 @@ | |||
require 'test_helper' | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might also include tests for merchants who don't have any orders, or products, etc.
test "Products cannot have a rating less than 0 or greater than 5" do | ||
assert products(:glitter).rating = 6 | ||
assert_not products(:glitter).valid? "Glitter should no longer be valid" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work, isn't rating a method?
return lesser_categories | ||
end | ||
|
||
def current_cart |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's interesting that you have a different cart for merchant or guest. That will work and it lets you merge carts when they log in finally. Tricky.
end | ||
|
||
def show | ||
@category = Category.find(params[:id].to_i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about when you can't find the Category, it's been deleted etc?
@@ -0,0 +1,59 @@ | |||
module OrdersHelper | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another good use of a view helper
@@ -0,0 +1,134 @@ | |||
# Merchant Model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very very well done. Good use of validations & business logic here!
@@ -0,0 +1,44 @@ | |||
require 'test_helper' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On your controller tests you should also include some negative cases, what if the user tries to see a category that's been deleted or never existed. What if someone without permission tries to edit a category?
@@ -0,0 +1,41 @@ | |||
require 'test_helper' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably also want to test the response if a user goes directly to the callback url without going through github.
@@ -0,0 +1,61 @@ | |||
<div class="small-12 columns buttons_container row"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work on responsiveness. You could DRY up your views a bit by using partials, but well done!
From the people who brought you the "Charles in Charge" meme, we bring you http://notyetsy.herokuapp.com/