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

Amy Kintner's job-tracker #12

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

Conversation

akintner
Copy link

No description provided.

has_many :contacts


def self.top_3_by_average_interest
Copy link
Owner

Choose a reason for hiding this comment

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

i don't see tests for these model methods

belongs_to :category
has_many :comments

def self.count_by_interest
Copy link
Owner

Choose a reason for hiding this comment

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

same here - don't forget to tests these methods!


def create
@contact = Contact.new(contact_params)
@contact.company_id = params[:company_id]
Copy link
Owner

Choose a reason for hiding this comment

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

leverage your associations here rather than setting the foreign key manually

<div class="form-group">

<% if @category.errors.any? %>
<h2><%= pluralize(@category.errors.count, "error") %> prohibited this record from being saved:</h2>
Copy link
Owner

Choose a reason for hiding this comment

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

nice use of pluralize 👍

<br/>
<%= f.label :email %>
<%= f.text_field :email %>
<%= hidden_field_tag :company_id, @company.id %>
Copy link
Owner

Choose a reason for hiding this comment

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

how can we avoid passing hidden fields in our forms?


<br/>
<%= f.label :job_category %>
<%= f.collection_select :category_id, Category.order(:title), :id, :title, include_blank: false %>
Copy link
Owner

Choose a reason for hiding this comment

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

avoid querying your database in your views (Category.order(:title))

<h3>Jobs in Each Location</h3>
<% if self.count_by_location %>
<ul class="by_location list-unstyled">
<li><%= <% Job.order(:city) %></li>
Copy link
Owner

Choose a reason for hiding this comment

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

remove this from the view - let your controller get this data 👍

<ul class="nav navbar-nav">
<li class="nav navbar-nav"><a href="/dashboard">Job Dashboard</a></li>
<li class="nav navbar-nav"><a href="/companies">Companies</a></li>
<li class="nav navbar-nav"><a href="/categories">Job Categories </a></li>
Copy link
Owner

Choose a reason for hiding this comment

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

leverage your rail's helpers - link_to and your path helpers

@@ -0,0 +1,45 @@
FactoryGirl.define do
Copy link
Owner

Choose a reason for hiding this comment

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

wooo factories 👍

require 'rails_helper'

describe "User creates a new category" do
scenario "a user can create a new category" do
Copy link
Owner

Choose a reason for hiding this comment

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

be sure to test your sad paths

@case-eee
Copy link
Owner

case-eee commented Jan 2, 2017

hey @akintner - overall, good work with this. i made a few comments. be sure to remember to commit more often as well 👍

Rubric

1) Database, Relationships, and Migrations

  • 4: The database has appropriate tables and appropriate columns to create relationships between tables. Foreign keys are indexed to increase database performance. Tables and columns are appropriately named.

2) Routes

  • 3: The developer has routes for all functionality that they provide, but may include routes that are not used in the application.

3) Controllers

  • 4: The developer has moved logic out of the controllers and into the models/POROs where appropriate. The developer uses strong params in a private method. Instance variables being passed to views are appropriately named and limited in number. The developer can speak to each choice made when questioned.

4) ActiveRecord

  • 3: ActiveRecord methods are used appropriately in the database, but some Ruby enumerables may also be used. The developer uses ActiveRecord relationships appropriately, and does not call on other classes in their models.

5) Views

  • 3: Limited logic that could be moved elsewhere remains in the views and/or controllers and developers are able to identify potentially opportunities to refactor.

6) User Experience

  • 4: The application has been styled and the user can easily navigate between different portions of the application without manually entering the URL into the nav-bar or using the back button on their browser.

7) Testing

  • 2: Project has sporadic use of tests and multiple levels

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.

2 participants