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

m^gj*c #62

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

m^gj*c #62

wants to merge 504 commits into from

Conversation

canaanwest
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
How did your team break up the work to be done? One of our biggest issues was that we didn't really break up the work very well--we tried to each work on things we wanted to be working on, grabbing certain features or things we wanted to be doing, but that meant that we frequently overlapped on things. Eventually, we got to a point where we split by controllers
How did your team utilize git to collaborate? We used a pull request system; this was ver difficult for us. At times, we'd find that we were overwriting each other's files without realizing it until much later.
What did your group do to try to keep your code DRY while many people collaborated on it? We tried to utilize external helper methods so that we didn't have to repeat things (before actions, before permissions, render 404, etc.). We also reviewed each other's code to try to refactor.
What was a technical challenge that you faced as a group? GITHUB.
What was a team/personal challenge that you faced as a group? We started off working very informally, which made it hard to communicate when things got really stressful. We also struggled to stick to deadlines. We didn't break our trello board tasks up enough, which made it really hard in the beginning to get things done.
What could your team have done better? Setting up clear expectations around communication and tasks would be really helpful.
What was your application's ERD? (include a link) https://www.lucidchart.com/invitations/accept/2a64acb3-5af9-4879-b874-7986468175ea
What is your Trello URL? https://trello.com/b/M1VXKYRN/northwesty
What is the Heroku URL of your deployed application? https://aqueous-shore-87265.herokuapp.com/

canaanwest and others added 30 commits October 23, 2017 15:57
Can only create a category on the category/show page if you are logge…
some styling for the users#show page
products can now be created with categories
…testing in order, orderproducts and products tomorrow
@CheezItMan
Copy link

bEtsy

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage with all members contributing Good number of commits and commit messages, but it seems like a lot of the commits came from a few users.
Answered comprehension questions I'm sorry Github was such an issue. It sounds like planning was a problem. However I hope you all learned quite a bit.
Trello board is created and utilized in project management You got a lot done it seems, but some features you didn't manage to get to. It does seem like it might have been hard to track things on your trello board.
Heroku instance is online Check
General
Nested routes follow RESTful conventions Restful Routes where appropriate.
oAuth used for User authentication Check
Functionality restricted based on user roles The site crashes if I try to edit a product that isn't mine, but it doesn't give an error message or redirect.
Products can be added and removed from cart Updating a quantity in the cart results some test data being printed in the browser. Adding an element to the cart does the same.
Users can view past orders I'm not seeing past orders
Merchants can add, edit and view their products Check
Errors are reported to the user Some errors are reported, like ordering more items than available
Order Functionality
Reduces products' inventory Check
Cannot order products that are out of stock Check
Changes order state Check, although it's hard to spot the confirm order details message.
Clears current cart Check
Database
ERD includes all necessary tables and relationships Nicely done in Lucidchart
Table relationships Relationships in the models
Models
Validation rules for Models Check, well done here
Business logic is in the models Lots of methods in the models. Hopefully it dryed out your controllers a bit.
Controllers
Controller Filters used to DRY up controller code Good use of controller filters to find the user, product, order etc.
Testing
Model Testing Pretty good testing, some missed methods. I left some notes in the code.
Controller Testing Decent testing, I left some notes in the code, you've got a number of broken tests.
Session Testing Pretty standard tests.
SimpleCov at 90% for controller and model tests 92% coverage
Front-end
The app is styled to create an attractive user interface There's still some work to do on the appearance.
The app layout uses Foundation's Grid It's responsive, but the background is problematic.
Content is organized with HTML5 semantic tags Good semantic HTML
CSS is DRY It looks be pretty DRY.
Overall I did notice that there wasn't a way to see an average rating. Your group was pretty far along, but you're missing a few features. Overall not bad. There are also some gaps in testing and things you didn't get to.

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

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

A few comments

validates :credit_card_name, presence: true
validates :credit_card_number, presence: true
validates :credit_card_cvv, presence: true
validates :billing_zip_code, presence: true

Choose a reason for hiding this comment

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

More validations would be good, but you probably ran out of time.

class Item < ApplicationRecord
belongs_to :product
belongs_to :order, optional: true
validates :shipping_status, inclusion: {in: [true,false]}

Choose a reason for hiding this comment

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

It seems that you could combine shipping status and purchase status

validates :shipping_status, inclusion: {in: [true,false]}
validates :purchase_status, inclusion: {in: [true,false]}

def purchase(input_order_id)

Choose a reason for hiding this comment

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

Good that you're putting business logic in the model.


def has_billing_datum?
if self.order_status != "pending"
self.errors[:billing_datum_id] << "is missing. Please entire valid billing information." if !(self.billing_datum_id)

Choose a reason for hiding this comment

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

Please entire???

end

#gets called if check_for_duplicates returns true
#TODO: Julia removed below method. Does not appear to be applied in any part of the project.

Choose a reason for hiding this comment

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

Good idea to leave notes like this for future work.

@@ -0,0 +1,41 @@
require "test_helper"

describe SessionsController do

Choose a reason for hiding this comment

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

What about a test for when a user visits the callback path without logging in?

must_redirect_to product_path(products(:converse).id)
end

it "If correct info is not provided, render new" do

Choose a reason for hiding this comment

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

What is this doing in the controller test?


get product_path(products(:converse).id)

controller.instance_variable_get("@current_user_is_not_product_owner").must_equal false

Choose a reason for hiding this comment

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

This should probably just be a test for redirect

name: product.name + " updated"
}
}
patch product_path(product), params: product_datum

Choose a reason for hiding this comment

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

Shouldn't you use a must... something



describe "new" do
it "can reach a new category" do

Choose a reason for hiding this comment

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

What about guest vs logged in users?

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