Pipes-StarMart by lmarianarp19 · Pull Request #64 · Ada-C8/betsy

lmarianarp19 · 2017-10-27T21:02:44Z

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?	The first day we took time to plan and design our app carefully. We try to not work at the same file at the same time. When team work was needed we worked in pairs.
How did your team utilize git to collaborate?	We use branches. Each person has their own branches for their changes and periodically we merge our branches.
What did your group do to try to keep your code DRY while many people collaborated on it?	We move repeated methods into the application class. We also used helper methods and partial forms.
What was a technical challenge that you faced as a group?	Pushing to heroku, Oauth authentications.
What was a team/personal challenge that you faced as a group?	Understanding and communicating the specific details of a problem. Specially when the code was more complex.
What could your team have done better?	We should take more breaks and not work so much after school hours. Also we should update trello board more often.
What was your application's ERD? (include a link)	https://www.lucidchart.com/invitations/accept/27d5b7de-c0d8-44bf-a8f6-729f999bb77b
What is your Trello URL?	https://trello.com/b/D38i27LH/starmart
What is the Heroku URL of your deployed application?	https://starmart.herokuapp.com/

…essions_controller

…ed Irene and Bianca as legit merchants and gave them products and seed data

…ow view

…ests

droberts-sea · 2017-10-30T22:44:56Z

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	mostly - see inline comment
oAuth used for User authentication	yes
Functionality restricted based on user roles	some - can't edit another merchant's products, but can review my own
Products can be added and removed from cart	yes
Users can view past orders	yes
Merchants can add, edit and view their products	yes
Errors are reported to the user	yes - good work!
Order Functionality
Reduces products' inventory	yes
Cannot order products that are out of stock	yes
Changes order state	yes
Clears current cart	yes
Database
ERD includes all necessary tables and relationships	yes
Table relationships	yes - good work using `has_many through:`
Models
Validation rules for Models	yes
Business logic is in the models	yes - you do a good job keeping it out of the views, and I only noticed one place where it should be removed from the controller
Controllers
Controller Filters used to DRY up controller code	yes, but see inline comments (especially re: `restrict_merchant`)
Testing
Model Testing	yes - good work, especially on negative and edge cases
Controller Testing	yes - missing tests for `order_items#ship`, otherwise good work
Session Testing	yes
SimpleCov at 90% for controller and model tests	yes
Front-end
The app is styled to create an attractive user interface	yes - love the theme! And the favicon!
The app layout uses Foundation's Grid	yes
Content is organized with HTML5 semantic tags	yes
CSS is DRY	yes
Overall	Great job overall! This site is attractive, well-organized and easy to navigate, and as far as I could tell everything works. Test coverage is solid, and I was not able to get away with any malicious behavior. As with any large project there are a few places where things could be cleaned up, and I've tried to point those out inline, but in general this is a very strong submission. You've clearly put in a ton of effort on the project, and it's paid off. Keep up the hard work!

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

droberts-sea · 2017-10-30T21:50:46Z

app/controllers/application_controller.rb

+    @merchant = Merchant.find_by(id: expected_merchant_id)
+
+    if @login_merchant && @login_merchant.id == @merchant.id
+      unless @merchant


The format of this method is a little strange to me - I think you might be able to design a cleaner interface.

Also, line 53 (unless @merchant) will never happen, because if the @merchant is nil the the call to @merchant.id on line 52 will produce an error.

droberts-sea · 2017-10-30T21:52:29Z

app/controllers/merchants_controller.rb

+
+  before_action only:[:show] do
+    @try_merchant = Merchant.find_by(id: params[:id])
+    restrict_merchant(@try_merchant)


If you're restricting the URI parameter to only be the ID of the logged in merchant, then why have a parameterized URI at all? It would probably make more sense to always show the logged in merchant's details.

droberts-sea · 2017-10-30T21:56:47Z

app/controllers/orders_controller.rb

+  # TODO: TAKE ALOOK AT THIS. WHAT IS THE MERCHANT ID COMING FROM? SESSIONS? ALREADY A @LOGIN_MERCHANT
+  before_action only:[:index] do
+    restrict_merchant(params[:merchant_id])
+  end


I very much agree with that TODO - by my reading, restrict_merchant checks that the argument matches the logged in merchant's ID, which in this case will always be true.

droberts-sea · 2017-10-30T21:57:47Z

app/controllers/payments_controller.rb

+      order.products.each do |item|
+        order.order_items.each do |order_item|
+          item.inventory -= order_item.quantity
+          item.save


This would be a good bit of work to do in the Model, possibly as a custom method on Order.

droberts-sea · 2017-10-30T21:59:00Z

app/controllers/products_controller.rb

+  end
+
+  def create
+    if @login_merchant


You have this if @login_merchant many times in this controller, would it be possible to DRY this up with a filter?

droberts-sea · 2017-10-30T22:05:00Z

config/routes.rb

+
+  resources :orders, only: [:show, :index] do
+    resources :payments, only: [:index, :new, :create]
+  end


I'm not sure why the payments routes are nested under orders here. Seems to me that the routes for a payments ought not to be parameterized at all, but instead take cart info from the session.

droberts-sea · 2017-10-30T22:31:43Z

app/controllers/orders_controller.rb

+
+  def order_items_params
+    params.permit(:quantity, :product_id)
+  end


This helper method is never called! Be careful what you copy-paste!

idevera and others added 30 commits October 24, 2017 08:56

Add to sessions controller

7a6ee84

Pulling from origin master

572aeb1

Getting up to date with master branch

172029b

Begin payment show page - needs to develop address information

153d0f8

Merge branch 'inventory'

5815ae6

Merge branch 'master' of https://github.com/lmarianarp19/betsy into s…

d7bfe08

…essions_controller

Delete comments application view

b96c516

Merge branch 'master' of https://github.com/lmarianarp19/betsy

88bc04e

Deleting comments for heroku

fd9e523

draft of shipping info page

fad3125

minor style sheet change

23c45fd

Merge branch 'master' of https://github.com/lmarianarp19/betsy

f810e4b

minor change to style sheet

32f3e13

resolved merge conflict

41e0bc1

Added login constraint for orders show action in controller. Also add…

3deb654

…ed Irene and Bianca as legit merchants and gave them products and seed data

Add migration to include name and working on form for payment checkout

5d998be

Adds Mariana as a user and gives her products and down stream seed data

a764489

Add name to form, saving successfully

401bf5b

1 changes ship

5b43636

Fix merge conflict in scss

5a0e102

Merged conflicts in payment csv

a6fd857

Fix sessions and could not get two tests to pass

23ed76b

Merge branch 'sessions_controller'

6619a66

Add link to shipping information on orders index

15f1348

Merge branch 'master' of https://github.com/lmarianarp19/betsy

da191f9

Centered tables on orders index show

ca63e5e

Merge branch 'merchants'

ba0aa06

Commented out errors for application category drop down, known problem

0a32c2e

Add session.clear to create in payment controller

495fe93

Merge branch 'tuesday'

2af0b5e

lmarianarp19 and others added 27 commits October 27, 2017 10:24

Install yarn

581a39b

Change products index and review form

4cbf127

Add show method for the payment

480aa81

Merge branch 'master' of https://github.com/lmarianarp19/betsy

8d59c86

Adds the payments path link back to partial that feeds into orders sh…

2f1fde6

…ow view

hashed out products category validation for seeding purposes

b181484

Change color paid, pending

1faa7e4

adds more review seed data so we can see more cool icons

0bbac5a

Add categories validation test

b662fcb

Merge branch 'master' of https://github.com/lmarianarp19/betsy into t…

422552b

…ests

Merging conflict orders index

7cc5b05

Merge branch 'master' of https://github.com/lmarianarp19/betsy

a8ad420

Change create method for resetting session of order_id to nil

b5dd16f

Merge branch 'styling'

d8a44fe

vendor compiled assets

2d694ed

Changes to style

3435b1b

Merge branch 'master' of https://github.com/lmarianarp19/betsy

e3eb9f7

Adding style to heroku

edb5e8a

change font size

1a49ebc

Changes env

b36aff1

finished payments controller tests

932f810

fixed merge conflict in payments controller

fb520d7

Add final touches to starmart

b64f275

fixed a test in products but it greated another

17d4540

presentation time

4a4547b

Merge branch 'master' of https://github.com/lmarianarp19/betsy

8c612fd

vendor compiled assets

c985eed

droberts-sea added the Pipes label Oct 30, 2017

droberts-sea reviewed Oct 30, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pipes-StarMart#64

Pipes-StarMart#64
lmarianarp19 wants to merge 429 commits intoAda-C8:masterfrom
lmarianarp19:master

lmarianarp19 commented Oct 27, 2017

Uh oh!

droberts-sea commented Oct 30, 2017

Uh oh!

droberts-sea Oct 30, 2017

Uh oh!

droberts-sea Oct 30, 2017

Uh oh!

droberts-sea Oct 30, 2017

Uh oh!

droberts-sea Oct 30, 2017

Uh oh!

droberts-sea Oct 30, 2017

Uh oh!

droberts-sea Oct 30, 2017

Uh oh!

droberts-sea Oct 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

lmarianarp19 commented Oct 27, 2017

bEtsy

Comprehension Questions

Uh oh!

droberts-sea commented Oct 30, 2017

bEtsy

What We're Looking For

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

Uh oh!

droberts-sea Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

droberts-sea Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

droberts-sea Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

droberts-sea Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

droberts-sea Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

droberts-sea Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

droberts-sea Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants