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

Ampers: Fur-eign Keys #27

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

Ampers: Fur-eign Keys #27

wants to merge 358 commits into from

Conversation

2020dream
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? We divided work by MVC. We have one Model/DB person, one View person, and two Controller people. Most of the individual work were done at home. Whenever there was a need to collaborate or communicate, we did it at Ada as a whole group or pairs.
How did your team utilize git to collaborate? All of us had our own branches on local and/or github, especially for features we were not sure about. We learned about how to resolve merge conflicts, and realized that no merge conflicts doesn't mean no errors during merging. Although we planned to use PR in our project, we didn't try it because of time constraint. We should definitely use it in our future group project.
What did your group do to try to keep your code DRY while many people collaborated on it? It's hard... We kinda prioritize our work to make the app work, rather than writing beautiful codes. We did some cleanup when the app was 90% ready for demo. We removed unused routes, views, and controller methods that were found through team reviews and failed controller tests.
What was a technical challenge that you faced as a group? Cart logic. After discussions with tutor and PM, we realized that there were several ways to implement it. We compared them and chose one based on how comfortable we were with the different options. It still took us a long time to implement that function.
What was a team/personal challenge that you faced as a group? We believed that all members in the team did their best to make our app not just work but better than we expected.
What could your team have done better? We probably should write controller tests first next time.
What was your application's ERD? (include a link) https://www.lucidchart.com/documents/edit/619b0172-f884-42d1-91fe-ea00101d45c1/0
What is your Trello URL? https://trello.com/b/2tpzc7EV/puppsy
What is the Heroku URL of your deployed application? https://puppsy.herokuapp.com/

MonalisaC and others added 30 commits April 22, 2018 22:12
… and an additional instance of OrderItem is not created.
… method for the cart controller. Nothing is completed or functional yet w/re this.
…e updated and working. Methods within the cart controller for add_product, access_cart, and the private create_cart are also working.
2020dream and others added 28 commits April 26, 2018 21:14
…to make new instances for anyone other than the logged in user.
…nued_products_orders_orderitem_reviews_controller_testing
…nued_products_orders_orderitem_reviews_controller_testing
@CheezItMan
Copy link

bEtsy

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage with all members contributing Check
Answered comprehension questions Check, it's not surprising you didn't have time to refactor your code as well as you wanted. I noticed I don't have access to your ERD on lucidchart.
Trello board is created and utilized in project management Check
Heroku instance is online Check
General
Nested routes follow RESTful conventions For the most part, you have one unused route for authentication. Also I'm not sure what the new_session_path serves.
oAuth used for User authentication Check
Functionality restricted based on user roles Check, except that using Postman, I can create products without logging in! No require_login filter! I just hacked your site!
Products can be added and removed from cart
Users can view past orders
Merchants can add, edit and view their products I'm able to add a product and edit it. I do notice if I make the quantity or price too big it crashes. Nice work with "retiring" products and reactivating them.
Errors are reported to the user Yep, nice work reporting errors next to the fields. Good front-end validations.
Order Functionality
Reduces products' inventory Check
Cannot order products that are out of stock Check
Changes order state Check
Clears current cart Making an order clears the cart!
Database
ERD includes all necessary tables and relationships Can't see your ERD
Table relationships Your models have relationships
Models
Validation rules for Models Check, both on front-end and on the model-end
Business logic is in the models Nice work here, you've got a lot of meaty work in the models.
Controllers
Controller Filters used to DRY up controller code Instead of also having skip_before_action :require_login, only: [:create], you can have before_action :require_login, except: [:create], although this filter in products is a bug (see above).
Testing
Model Testing Nice through testing here
Controller Testing One failing test in the products controller. You are also not testing to see what happens when unauthenticated users try to access orders etc. if you have a filter to prevent guest users from accessing something you need to test it to make sure it's working!
Session Testing Check
SimpleCov at 90% for controller and model tests Nice job in coverage, above 90% in controllers and 100% in models!
Front-end
The app is styled to create an attractive user interface I really like the look, very smooth interface with some responsiveness!
Content is organized with HTML5 semantic tags Good semantic HTML
CSS is DRY Not especially, but that's not a huge issue. Some selectors have similar rules, but that's not high priority
Overall Really nice work. You did a great job getting the site built to where it is with lots of through tests. You do need to write tests to verify that the controllers are doing the proper actions for both guest users and authenticated users. That's a big gap in your testing! Overall you hit the learning goals, but in API-Muncher if you get to putting OAuth in, make sure you test your permissions setup!

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

@product_2 = products(:product_2)
@product_2.user_id.must_equal @user_2.id

modified_2 = Product.find_by(user_id: @user_2.id)

Choose a reason for hiding this comment

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

You should be finding the product by the product ID not user.id. This test is failing due to that!


get '/auth/:provider/callback', as: 'auth_callback', to: 'sessions#create'

get '/auth/github', as: 'github_login'

Choose a reason for hiding this comment

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

This route shouldn't be here. Omniauth is adding this route to redirec the user to github.

<%= f.select :id, Category.all.map { |category| [category.name, category.id] }, :prompt => 'Select a category' %>
</div>
<div class="filter-button">
<%= f.submit "Sumbit", class: "button" %>

Choose a reason for hiding this comment

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

submit? This button won't show as you aren't making a submit button.


def update_cart_info
@cart = Order.find_by(id: session[:cart_order_id])
if @cart && @cart.id == session[:cart_order_id]

Choose a reason for hiding this comment

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

Maybe strong params would be useful here.

before_action :require_login
skip_before_action :require_login, only: [:create]

def new

Choose a reason for hiding this comment

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

What purpose is this route/action serving?

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