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

Bakesy #23

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

Bakesy #23

wants to merge 264 commits into from

Conversation

elle-johnnie
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
Each team member: what is one thing you were primarily responsible for that you're proud of? Laura: checkout process, Jazz: session tests when not every user logs in, Trang: Shopping Cart works and helped her understand session integration, Liz: Merchant dashboard functions
Each team member: what is one thing you were primarily responsible for that you would like targeted feedback on? Laura: where code should go Jazz:dependencies Trang: tests and how to be Metz compliant Liz: db queries and optimization
How did your team break up the work to be done? daily stand-ups helped us prioritize
How did your team utilize git to collaborate? implemented branching of features to minimize major git-tastrophies.
What did your group do to try to keep your code DRY while many people collaborated on it? we used a method of commenting things out then waiting until the end to clean most of the unused code
What was a technical challenge that you faced as a group? git merging initially, but we developed a good workflow as we continued with the project
What was a team/personal challenge that you faced as a group? Understanding how a real-life working situation works when people typically work more independently of each other and handle a major project. We missed a stand-up and that day we all found ourselves struggling a bit that evening to know what to prioritize and accomplish for the day.. In hindsight we realized that we should have done a midpoint huddle to reorganize and check back in on our original plan.
What was your application's ERD? (include a link) https://www.lucidchart.com/documents/view/0ce65827-ac9c-4f15-b1b7-4f9278f6a9af/0
What is your Trello URL? https://trello.com/b/PGJtzRSW/bakesy
What is the Heroku URL of your deployed application? https://bakesy.herokuapp.com/

@elle-johnnie
Copy link
Author

elle-johnnie commented Nov 9, 2018 via email

@dHelmgren
Copy link

dHelmgren commented Nov 9, 2018 via email

@dHelmgren
Copy link

bEtsy

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage with all members contributing yes
Answered comprehension questions yes, though it would have helped me to have more detailed answers to the first two questions. I had a hard time giving targeted feedback because several answers were so vague.
Trello board is created and utilized in project management yes
Heroku instance is online yes
General
Nested routes follow RESTful conventions Yes
oAuth used for User authentication Yes
Functionality restricted based on user roles yes
Products can be added and removed from cart yes
Users can look up past orders by ID 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 There's a loophole in your logic that lets people order things that are out of stock, see comments.
Changes order state Yes
Clears current cart Yes
Database
ERD includes all necessary tables and relationships Yes
Table relationships Yes
Models
Validation rules for Models Good
Business logic is in the models Good
Controllers
Controller Filters used to DRY up controller code yes!
Testing 6 of your tests fail outright and 25 produce errors!
Model Testing Missing validations on custom logic, see comment.
Controller Testing Malformed paths in controller tests, see comment.
Session Testing Good.
SimpleCov at 90% for controller and model tests No, you're hovering at 85%
Front-end
The app is styled to create an attractive user interface Yes, but your dashboard has fonts and colors that don't match the rest of the site.
Content is organized with HTML5 semantic tags There are a lot of semantic tags that could make your HTML much more readable, see comments!
CSS is DRY Yes
Overall Given your overall level of work on this project, I would say you are meeting standard. Your project presents itself well and does a good job of preventing basic errors. However, your testing overall leaves a lot to be desired. The number of tests that are not working combined with the low coverage from simplecov means your tests not the level of work that we expect from you. Not to drive the point too hard, but test driven development requires writing tests alongside your code, not after it, and that as things change in your code, you make sure the tests change with them.

Only the person who submitted the PR will get an email about this feedback. Please let the rest of your team know about it.

<% end %>
</div>
<div class="col-md-3 add-review-btn">
<span><%= link_to "Write A Review", new_product_review_path(@product), class:"btn btn-danger" %>

Choose a reason for hiding this comment

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

This would be a really good place to try the <aside> tag!

<div class="row">
<div class="col-md-12">
<p class="description">
Tasting Notes: <%= @product.description %>

Choose a reason for hiding this comment

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

Things like tasting notes here might be a good candidate for the <details> tag.

get cart_path(order_id)

must_respond_with :not_found

Choose a reason for hiding this comment

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

Tests for your cart specifically might try:

  • Does it handle things being added correctly?
  • Do items get removed properly?
  • Can I over-order items by having two carts check out the last of a particular item?


describe CategoriesController do
it "should get index" do
get categories_index_url

Choose a reason for hiding this comment

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

categories_index_url is not the correct path name, so none of these tests are doing what you would like them to do, which means your tests don't adequately test your code. Making sure that the named path actually matches the route that you want to call is important! You wanted either categories_path or categories_url

it "must be invalid without a customer name " do
expect(order.valid?).must_equal false
expect(order.errors.messages).must_include :cust_email
end

Choose a reason for hiding this comment

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

I'm pretty sure that this end is premature, and it breaks every test that follows.

value(response).must_be :success?
end

it "should get create" do

Choose a reason for hiding this comment

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

Your create path uses the Post verb, so this test is malformed.



end

Choose a reason for hiding this comment

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

Where are the tests for your custom logic? Most of your models have some sort of custom logic, but the methods aren't getting tested at all.

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