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

RegrEtsy - Octos - Kat/Ari/Maria/Anne #19

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

Conversation

annehwatson
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 tried to break up the user stories into waves on Trello board and then assigned ourselves to things based on interest.
How did your team utilize git to collaborate? We tried to have good git hygiene and branches and the check in dance.
What did your group do to try to keep your code DRY while many people collaborated on it? Partial views, before action filters in app controller and skip before action in other controllers
What was a technical challenge that you faced as a group? Git directory problems prevented a push to heroku and merging
What was a team/personal challenge that you faced as a group? Communication and splitting work and keeping trello up to date.
What could your team have done better? Prioritization
What was your application's ERD? (include a link) https://www.lucidchart.com/invitations/accept/ee17639d-a353-400d-91c3-e4f80c50dd37
What is your Trello URL? https://trello.com/b/H7FGNoog/regretsy
What is the Heroku URL of your deployed application? regretsy.herokuapp.com

arrriiii and others added 30 commits April 23, 2018 10:41
@kariabancroft
Copy link

bEtsy

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage with all members contributing Commits are not as even among team members as I'd like
Answered comprehension questions Yes
Trello board is created and utilized in project management You set up Trello and did a nice job moving cards from one list to another. One thing that was concerning was that there were many checklists in completed cards that didn't have updated checklists. Some tasks were granular and some were super generic, which makes it hard to track and ensure work isn't being duplicated by team members.
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 view past orders Yes
Merchants can add, edit and view their products Add & view - looks like normal editing is not built though
Errors are reported to the user Yes
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
Models
Validation rules for Models Yes - though i'd like to see more in the models vs form validations
Business logic is in the models Yes
Controllers
Controller Filters used to DRY up controller code Yes
Testing
Model Testing Disappointed in the amount of model testing, especially since there are a fair amount of custom methods in the models.
Controller Testing Disappointed in the controller tests overall. Nice job on the categories controller tests though.
Session Testing Yes - good
SimpleCov at 90% for controller and model tests No
Front-end
The app is styled to create an attractive user interface Yes
Content is organized with HTML5 semantic tags Yes - nicely structured using sections throughout. Struggling with many views that are not very readable.
CSS is DRY Yes - somewhat
Overall I think you did a good job getting the majority of the core functionality complete. I think if you focused on a TDD approach early on, it could've helped you with merge issues last on if everyone was always running the test suite when making changes.

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


resources :carts, only: [:show, :edit, :update]
post '/carts', to: 'carts#add_to_cart', as: :add_to_cart
post '/add', to: 'sessions#add_cart_product', as: :add_product

Choose a reason for hiding this comment

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

While this route makes sense once you the controller and the action, just "/add" by itself doesn't provide enough information about what it does


def finalize
@paymentinfo = Buyerdetail.new(payment_params)
@paymentinfo.assign_attributes(order_id: @order.id)

Choose a reason for hiding this comment

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

Using assign_attributes is great to use with a hash of data, but when you're setting a single property, you should just use the dot method. So @paymentinfo.order_id = @order.id instead.

@customer = Buyerdetail.find_by(id: @order.buyerdetail_id)
end

def order_status

Choose a reason for hiding this comment

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

If this is specifically for cancelling the orders, this could be named more explicitly

@product.assign_attributes(product_params)

if @product.save
redirect_to product_path(@product)

Choose a reason for hiding this comment

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

You should have some flash messages here and in the destory method below


create_table "reviews", force: :cascade do |t|
t.integer "rating"
t.text "review"

Choose a reason for hiding this comment

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

I would not create a column within a table with the same name, recommend using "description" or "details" which would better describe the columns data

<% end %>
<p>

<% if item.reviews.count == 1 %>

Choose a reason for hiding this comment

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

You should be able to conditional display "review" or "reviews" without duplicating the entire link_to

<td><%= number_to_currency product.price %></td>
<td>
<td><%=item.quantity%></td>
<% if quantity_edit?(request.fullpath) %>

Choose a reason for hiding this comment

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

The contents of this conditional is another example of encapsulating more than it needs. You should be able to change the submit_tag only, not the entire form, since the rest is the same.

<nav>
<ul class="menu">

<li class="menu-item-has-children"><%= link_to "Browse", root_path%>

Choose a reason for hiding this comment

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

I like the way you used these menus, its nice!

<%= link_to"Add a Category", new_category_path, class: 'hollow button' %>
</section>

<section class="tabset">

Choose a reason for hiding this comment

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

The tabs add a nice UI element

// You can use Sass (SCSS) here: http://sass-lang.com/


.parallax-1 {

Choose a reason for hiding this comment

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

Each of the parallax styles are the same except for the image. You should be using one general class that includes all of the common parallax features, and then add another class for each specific image.

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