Skip to content

Soviétski!#21

Open
jacquelynoelle wants to merge 301 commits intoAda-C10:masterfrom
leannerivera:master
Open

Soviétski!#21
jacquelynoelle wants to merge 301 commits intoAda-C10:masterfrom
leannerivera:master

Conversation

@jacquelynoelle
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? Jessie: Listing products and multiple categories. Leanne: User model testing and single table inheritence. Jackie: Cart and front-end!
Each team member: what is one thing you were primarily responsible for that you would like targeted feedback on? All: Controller testing, we felt like we struggled with that.
How did your team break up the work to be done? Leanne focused on Merchant/User, Jessie focused on Product/Category, Jackie focused on Order/OrderProduct
How did your team utilize git to collaborate? We had some rough patches the first week, but then got in a good rhythm of doing PRs and code reviews. Felt like good practice for industry!
What did your group do to try to keep your code DRY while many people collaborated on it? We used a lot of partial views.
What was a technical challenge that you faced as a group? Controller testing. Merge conflicts. Dependencies of things that we were working on.
What was a team/personal challenge that you faced as a group? We started off down a person, and then were hit by illness and other unexpected things this week. Felt like most days we had 2.5 people. Given that, we're pretty proud of where we got.
What was your application's ERD? (include a link) https://files.slack.com/files-pri/TASRJV6S2-FDGLCE59B/20181017_105049.jpg
What is your Trello URL? https://trello.com/b/DVwDnA2N/jackie-jessie-leanne-betsy
What is the Heroku URL of your deployed application? https://sovietski.herokuapp.com/

leannerivera and others added 30 commits October 18, 2018 14:37
flash working, application html fixed, can recognize user
Remove merge conflict markers from routes.rb
Fixed formatting in application view
Added basic navigation for testing purposes
Added basic functionality to add product to an order (aka cart)
jessiezhang2017 and others added 28 commits October 25, 2018 22:53
Completed order and orderproduct controller testing & includes resolutions from merge with Jessie's work
Small front-end tweaks, product show page design
@dHelmgren
Copy link

bEtsy

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage with all members contributing Yes
Answered comprehension questions Yes
Trello board is created and utilized in project management yes
Heroku instance is online yes
General
Nested routes follow RESTful conventions
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, but as soon as I had an order to fulfill I could no longer access my dashboard.
Errors are reported to the user usually! I hit a bug where
Order Functionality
Reduces products' inventory Yes, but there is a significant bug, see comments
Cannot order products that are out of stock Two separate windows can fill their carts with the max number of items for a specific product, check out one at a time, and both complete their orders. You should always check available stock before you allow a user to complete an order!
Changes order state Yes
Clears current cart Yes
Database
ERD includes all necessary tables and relationships ERD link goes to slack, not an actual image, so I can't evaluate.
Table relationships see above
Models
Validation rules for Models Yes
Business logic is in the models Yes
Controllers
Controller Filters used to DRY up controller code At least one spot could have used more filters, see comments.
Testing When I run them, 5 of your tests fail. See comments on each test.
Model Testing good!
Controller Testing To be honest, many of these look good, though some are failing. What exactly is going on might be due to yml files having changed, but I suspect you have a more insidious bug. When I created my merchant, I noticed that their ID was 540, which seemed VERY high. Also, if there is any concrete suggestion I would give, it's look at your coverage and figure out what sorts of calls you need to make to hit more of the code. the User_controller_test.rb would be a good place to start.
Session Testing
SimpleCov at 90% for controller and model tests Looks like your simplecov is set up incorrectly and artificially inflating your coverage rate. Also, some very important modules (e.g. users_controller) have worryingly low coverage rates.
Front-end
The app is styled to create an attractive user interface Yes!
Content is organized with HTML5 semantic tags I think this can improve, see comments.
CSS is DRY Yes!
Overall This project is good, but it does have some rough edges. If there is anything I'd make sure to review it would be the testing for the users and the session because while I didn't have time to dive into it, my gut says something is very slightly broken, and it's having ripples. I know the division of labor was along model lines, but ideally you are all responsible for making sure the app is well tested. After that I would take a look at making better use of semantic HTML, as yours currently doesn't say much about the types of information on any page.

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


<p>
<%= image_tag(@product.photo_url) %>
</p>

Choose a reason for hiding this comment

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

Not sure why this is a

tag. This would be better incapsulated by a

tag

</p>
<%end%>

<p><%= @product.description%></p>

Choose a reason for hiding this comment

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

Could this be a <details> or <aside> tag instead of a <p> tag? the <p> tells me almost nothing.

font-size: 1rem;
}

div.form-check-inline > input {

Choose a reason for hiding this comment

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

Nice job sorting you CSS!

@@ -0,0 +1,26 @@
class CategoriesController < ApplicationController
def new

Choose a reason for hiding this comment

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

no controller filter for login, so I can see the new categories page without authorization

end

def update
if @current_order.update(order_user_params) && @current_order.submit_order

Choose a reason for hiding this comment

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

This seems like the path that you use for checkout, but it doesn't try to confirm that all the products in the cart are still available.

end

it "Can log in a new user with good data" do
# Arrange

Choose a reason for hiding this comment

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

This test fails with the following error

test_0002_Can log in a new user with good data                  FAIL (0.03s)
        "User.count" didn't change by -1.
        Expected: 5
          Actual: 9
        test/controllers/sessions_controller_test.rb:22:in `block (3 levels) in <top (required)>'

describe "create" do
#remember merchants are a subclass of user, so will be included in count as well
it "Can log in an existing user" do
# Arrange

Choose a reason for hiding this comment

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

This controller test is very similar to the one in your user's path, and also fails

test_0001_Can log in an existing user                           FAIL (0.04s)
        "User.count" didn't change by 0.
        Expected: 7
          Actual: 9
        test/controllers/sessions_controller_test.rb:11:in `block (3 levels) in <top (required)>'

describe ReviewsController do
# it "must be a real test" do
# flunk "Need real tests"
# end

Choose a reason for hiding this comment

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

:'(

end

it "will not create a new merchant if type is nil" do
not_a_merchant = Merchant.new(

Choose a reason for hiding this comment

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

This test fails with the following error

test_0005_will not create a new merchant if type is nil         FAIL (0.00s)
        Expected #<Merchant id: nil, created_at: nil, updated_at: nil, name: "Newbie", address: nil, cc_num: nil, type: nil, cc_exp: nil, bill_zip: nil, cc_csv: nil, uid: 890, provider: "github", email: "newbie@newbs.com", status: "active"> to not be an instance of Merchant.
        test/models/merchant_test.rb:35:in `block (2 levels) in <top (required)>'

let(:user) { users(:cc_user) }

it "changes a user instance into a merchant instance if type is set to Merchant" do
#before!

Choose a reason for hiding this comment

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

this test fails with the following error

test_0001_changes a user instance into a merchant instance if type is set to Merchant FAIL (0.01s)
        Expected #<User id: 1069983167, created_at: "2018-11-09 00:09:42", updated_at: "2018-11-09 00:09:52", name: "hannah", address: "123 Main St USA", cc_num: "8790451276789084", type: "Merchant", cc_exp: "2009-08-22 00:00:00", bill_zip: 10012, cc_csv: 678, uid: 97532187, provider: "github", email: nil, status: "active"> to not be an instance of User.
        test/models/user_test.rb:64:in `block (3 levels) in <top (required)>

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.

4 participants