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

Gale, Maria, Sara, Kimberley - Carets #72

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

Conversation

galestorm
Copy link

@galestorm galestorm 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? We started with assigning people to Trello cards. the models and controllers for testing and creating the methods. We set up the tasks in waves on Trello and as we completed the tasks, we moved on to different tasks in the waves.
How did your team utilize git to collaborate? We used to git to work on different branches based on features and tests. We then did a collaborative merge, pull, push after each day's stand up.
What did your group do to try to keep your code DRY while many people collaborated on it? We tried switching focus areas, such as products and orders, when we started our testing, that way we were reviewing other team member's models and controllers and would essentially proof each other's code to DRY it up. We also used before actions and custom methods that helped us dry the code up.
What was a technical challenge that you faced as a group? Deploying to Heroku. We had trouble logging in with git and with our database. We reviewed the Heroku logs and StackOverflow to determine the solutions.
What was a team/personal challenge that you faced as a group? We think we did a good job overall with ask and guess culture. It was a challenge having a group member out for 2 days, but as a group, we feel we communicated well over Slack to move forward with the project.
What could your team have done better? Deployed earlier to work out those issues.
What was your application's ERD? (include a link) We will Slack the photos of the ERD. We did it on the white board as opposed to electronically.
What is your Trello URL? https://trello.com/b/d42fqbbE/whythehexnots
What is the Heroku URL of your deployed application? http://sourceress.herokuapp.com/

kimberleyamackenzie and others added 30 commits October 18, 2017 14:29
@tildeee
Copy link

tildeee commented Nov 2, 2017

bEtsy

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage with all members contributing 🔮
Answered comprehension questions 🔮
Trello board is created and utilized in project management 🔮
Heroku instance is online 🔮
General
Nested routes follow RESTful conventions 🔮
oAuth used for User authentication 🔮
Functionality restricted based on user roles non-merchants can access new category page (and can make a new category), new product page (and cannot make a new product), and merchant view of their own products (http://localhost:3000/merchants/USER_ID/products)
Products can be added and removed from cart 🔮
Users can view past orders 🔮
Merchants can add, edit and view their products 🔮
Errors are reported to the user When creating an invalid product, no flash messages on the webpage. You display your error messages with a window alert, noooo!!!!!!!. No specific messages for unauthorized user, too
Order Functionality
Reduces products' inventory 🔮
Cannot order products that are out of stock 🔮
Changes order state On the Merchants->My Orders->Show Shipped Orders page, there is still a button to "ship" the order, and it gives a pop up for shipping unavailable on that order
Clears current cart 🔮
Database
ERD includes all necessary tables and relationships I'll reach out for an ERD soon
Table relationships 🔮
Models
Validation rules for Models 🔮
Business logic is in the models Some stuff I would prefer if you moved out of your model and into your view. Specifically, on the Product model for average rating, if there are no ratings, then it returns the string "Not yet rated!". The average rating should return the numerical value of the average rating, and then your view should handle the formatting of it, which would be a good place to create a custom view helper.
Controllers
Controller Filters used to DRY up controller code Used it most of the time, but forgot to use it in some places like ProductController#edit and ProductController#add_to_cart
Testing
Model Testing covers validations very well, I'm adding a note below about some of the model testing
Controller Testing no testing on authentication for CategoriesController and ProductsController, good job on negative tests
Session Testing
SimpleCov at 90% for controller and model tests 95.84% ✨
Front-end
The app is styled to create an attractive user interface 🔮, though I will comment that the visual aspects of the app severely slow it down and make it lag.
The app layout uses Foundation's Grid nope
Content is organized with HTML5 semantic tags sometimes not the right choice for the tag. Particularly, every instance where you used the classes "index-banner" and "home-link" I would have preferred gone with some header tag like an h1 instead of a span
CSS is DRY pretty impressed with the css parallax and animation, but it makes the site so sloooooooow 😢
Overall 🔮

IMPORTANT: Whoever submitted the PR (and thus will get the notification about this feedback) should share this with their teammates.

ProductControllerTest gives an error when you run the tests: Your products model fixture missing_name has no name. In the test ProductsControllerTest "succeeds when there are products," it fails because it loads all of the product fixtures, including missing_name, and when the test tries to get the products_path view, the view tries to call .upcase on the nil name of missing_name (views/products/index.html.erb:18).

It's best to have every fixture you define be valid, and when testing for invalidness on your model tests, to take a specific fixture and then set the target attribute to nil, like so:

it "must be invalid if there is no name" do
  product = products(:valid_product)
  product.valid?.must_equal true
  
  product.name = nil
  product.valid?.must_equal false
end


.dropdown:hover .dropdown-content-cart {
display: block;
}
Copy link

Choose a reason for hiding this comment

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

there are a couple of cases in this stylesheet that suffer from copy/paste and should be all on the same line, such as this:

.dropdown:hover .dropdown-content-browse, .dropdown:hover .dropdown-content-cart, .dropdown:hover .dropdown-content-cart {
  display: block;
}

order.products << product1
order.order_total.must_equal (9.99 * 2)
order.products << product2
order.order_total.must_equal (9.99 * 2) + 8.99
Copy link

Choose a reason for hiding this comment

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

What happens if the test data changes? Instead of using magic numbers/hardcoded numbers, it would be better if you use the data like product1.price instead of 9.99

<span class= "index-image"><%= link_to image_tag("http://www.visitbuffaloniagara.com/content/uploads/sv-event-images/434cfb09-bb34-449b-ac6c-d400b6a53f1d.jpg"), product_path(product.id), alt: "A product image placeholder" %></span>
<% end %>
<span><em><%= link_to product.name.upcase, product_path(product.id) %></em></span>
<span><%= product.price %></span>
Copy link

Choose a reason for hiding this comment

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

Your product.price needs to be formatted to a correct dollar format. Otherwise numbers appear like 100.0 instead of 100.00.

The other locations for this that I could find are:

  • Product detail page
  • Every Merchant order page

| What was your application's ERD? (include a link) | |
| What is your Trello URL? | |
| What is the Heroku URL of your deployed application? | |
| How did your team break up the work to be done? | We started with assigning people to Trello cards. the models and controllers for testing and creating the methods. We set up the tasks in waves on Trello and as we completed the tasks, we moved on to different tasks in the waves. |
Copy link

Choose a reason for hiding this comment

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

Also, these questions shouldn't be answered like this within this file!

github uses this file to automatically populate the text box for the description when you open a new Pull Request. Just edit that PR description. I went ahead and copied and pasted this into your PR for you :P

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