Skip to content

Feedback commit - #1

Open
case-eee wants to merge 1 commit into
masterfrom
feedback
Open

Feedback commit#1
case-eee wants to merge 1 commit into
masterfrom
feedback

Conversation

@case-eee

@case-eee case-eee commented Nov 2, 2016

Copy link
Copy Markdown
Collaborator

hey @meganft - great work with this project. don't forget to handle errors and write tests for every single method you write in your models! let me know if you have questions about my comments or the scores 🎉

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

  • 3: Some logic may leak into the controllers that would more appropriately exist in a model/PORO. The developer may pass more instance variables than necessary to the view.

4) ActiveRecord

  • 4: ActiveRecord methods are used in models to supply all appropriate functionality. Methods exist on the appropriate model, and developers are not referencing other classes or self in models unnecessarily. Ruby enumerables are not used where ActiveRecord methods could provide the necessary functionality. The developer can explain the ActiveRecord methods they used and the relationships between ActiveRecord 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

  • 3: Project has a running test suite that tests and multiple levels but fails to cover some features

@@ -0,0 +1,53 @@
class CategoriesController < ApplicationController
before_action :set_category, only: [:show, :edit, :update, :destroy]

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

👍


def create
@job = Job.find(params[:comment][:job_id])
@comment = @job.comments.create(comment_params)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

what if a user enters a blank comment? will they see an error?

def index
@companies = Company.all
if params[:location]
@city = params[:location]

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

no need to set this as an instance variable, we can access it via one of the companies in the view


def create
@company = Company.find(params[:contact][:company_id])
@contact = @company.contacts.create(contact_params)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

same here, what if a user fails to enter a contact's email? can they still create a contact? (i don't think the specs asked for this, but it's something to keep in mind!)

Comment thread app/models/company.rb
has_many :jobs
has_many :contacts

def self.location_of_jobs_counter

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

👍

Comment thread app/models/job.rb
group(:level_of_interest).count
end

def self.sorted_by_interest_level

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

i suggest breaking up the sorting by interest level and the grabbing the top three. this way, if you want to reuse the list of sorted jobs, you can!


<%= form_for @comment do |f| %>

<%= f.hidden_field :job_id, :value => @job.id %>

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

we already talked about how nesting our routes would help us avoid the need to send hidden data 👍

<a class="navbar-brand" href="#">Job Tracker</a>
</div>
<ul class="nav navbar-nav navbar-right">
<li><a href="/companies">Companies</a></li>

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

leverage your rails helper methods here and use link_to and your path helpers!

require 'rails_helper'

describe "User edits an existing category" do
scenario "a user can edit a category" do

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

don't forget sad paths!

Comment thread app/models/job.rb
belongs_to :category
has_many :comments

def self.level_of_interest

Copy link
Copy Markdown
Collaborator Author

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 custom model methods. don't forget to test every method you write!

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.

1 participant