Factory girl #25

bradgreen3 · 2017-02-27T02:21:16Z

About half of the tests were failing, so I fixed them all and got Factory Girl set up in the process.

Closes #18

lbraun · 2017-02-27T08:53:15Z

This looks great at a glance @bradgreen3, thanks so much. I'll do a real review soon. So happy to have your interest in the project and would love to chat sometime about what brought you to it. If you're interested, please join us on waffle.io and assign tickets to yourself as you pick them up: https://waffle.io/lbraun/thanksgiving/join

lbraun

This is fantastic @bradgreen3, thanks. I hope all my comments aren't overwhelming. I like to do thorough reviews, especially at first, not to have control but just to get on the same page with collaborators. Please let me what is helpful and what isn't.

One minor thing I couldn't add as an inline comment: please remove spec/features/.DS_Store from your PR. You may want to add .DS_Store files to your global gitignore.

Lastly, I would appreciate your help as a new contributor to improve our documentation. Did you find any of it particularly useful when getting started? How could we make it better? Feel free to update it as you go!

Thanks again, and just let me know if you have any questions.

-Lucas

lbraun · 2017-02-27T10:35:57Z

spec/support/feature_helpers.rb

@@ -1,7 +1,9 @@
 module FeatureHelpers
  def login_new_user(options = {})
    visit "/"
-    click_link "Sign in"
+    within(:css, ".super-button") do
+     click_link "Sign in or sign up"


Please indent with two spaces

lbraun · 2017-02-27T10:42:55Z

spec/factories/users.rb

+    sequence :name do |n|
+       "Fake User#{n}"
+    end
+    approval_at "2017-02-25 23:28:13"


From my experience with Factory Girl, the less you do in the main factory, the better. My rule of thumb is to only define attributes that are required (by a validation, for example). So if needed, we could add an approved_user factory, but I would take this line out here. What do you think?

That makes perfect sense! Default approval status removed 😄

lbraun · 2017-02-27T10:43:02Z

spec/factories/users.rb

+FactoryGirl.define do
+  factory :user do
+    provider "google_oauth2"
+    sequence :uid do |n|


Could you write this using a more compact single-line block instead?

lbraun · 2017-02-27T10:45:05Z

spec/features/users_spec.rb

+  context "when an admin visits the users index" do
+    let!(:admin) { login_new_admin_user }
+    let!(:users) { create_list(:user, 2) }
+    let!(:unapproved_user) { create(:user, approval_at: nil)}


Add a space after nil)

lbraun · 2017-02-27T10:46:42Z

spec/features/users_spec.rb

-        }
-      )
+    it "sees a table with proper headers and rows" do
+      expect(page).to have_css("#user_#{admin.id}", :text => "#{admin.name}")


Please use newer hash syntax instead of hash rockets:
https://github.com/bbatsov/ruby-style-guide#hash-literals

lbraun · 2017-02-27T12:42:13Z

spec/support/factories.rb

@@ -0,0 +1,3 @@
+FactoryGirl.define do
+  # <factories for each model go here>


I think we can get rid of this file since we are creating separate files for each model, don't you?

Yep, good call!

lbraun · 2017-02-27T12:52:57Z

spec/support/feature_helpers.rb

@@ -1,7 +1,9 @@
 module FeatureHelpers
  def login_new_user(options = {})
    visit "/"
-    click_link "Sign in"
+    within(:css, ".super-button") do


Thanks for fixing the specs: I really shouldn't have pushed my last update without running them. Do you practice TDD? I'd like to make it the convention for this project.

CSS is the default selector for Capybara, so you can leave it out in cases like this: within(".super-button") do

No problem! It was a good way to get familiar with the code base! I do practice TDD, so I'll certainly use it if I add any new features.

Good catch. I'm not sure why I put that in there 😕

lbraun · 2017-02-27T12:54:17Z

spec/factories/users.rb

+      n
+    end
+    sequence :name do |n|
+       "Fake User#{n}"


Please indent with two spaces.

lbraun · 2017-02-27T12:55:14Z

Gemfile

@@ -39,4 +39,5 @@ group :development, :test do
  gem 'capybara'
  gem 'launchy'
  gem 'rspec-rails'
+  gem 'factory_girl_rails'


lbraun · 2017-02-27T13:05:24Z

spec/features/users_spec.rb

-
-      before { visit users_index_path }
+    it "sees a table with proper headers and rows" do
+      expect(page).to have_css("#user_#{users.first.id}", :text => "#{users.first.name}")


There's no reason to use interpolation when you want to use the whole string ("#{x}" == x)

Yep, you're absolutely right.

…factory-girl

Brad added 4 commits February 25, 2017 18:55

Add Factory Girl

38f895e

Fix login new user helper method

7343b58

Fix a bunch of failing tests

37d6b10

Fix remaining failing tests with Factory Girl implementation

c3e0095

lbraun requested changes Feb 27, 2017

View reviewed changes

Brad added 11 commits March 1, 2017 15:24

Merge branch 'master' of https://github.com/lbraun/thanksgiving into …

847f5b3

…factory-girl

Change incorrect test name

4117ffb

Add dynamic class to approve user button

0bf74f6

Change nested context test description to begin with word and

4bd48ab

Make quote use more consistent

4019c6e

Remove unnecessary interpolation

4a0efab

Change test to test proper order of data in table

a2f7881

Remove hash rockets in favor of newer syntax

cfcc469

Change it blocks to refer to feature subject instead of context subject

5c23318

Remove default approval from user factory

e989528

Add minor stylistic changes

47f0343

lbraun changed the base branch from master to main August 8, 2020 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factory girl #25

Factory girl #25

bradgreen3 commented Feb 27, 2017

lbraun commented Feb 27, 2017

lbraun left a comment

lbraun Feb 27, 2017

bradgreen3 Mar 2, 2017

lbraun Feb 27, 2017

bradgreen3 Mar 2, 2017

lbraun Feb 27, 2017

bradgreen3 Mar 2, 2017

lbraun Feb 27, 2017

bradgreen3 Mar 2, 2017

lbraun Feb 27, 2017

bradgreen3 Mar 2, 2017

lbraun Feb 27, 2017

bradgreen3 Mar 1, 2017

lbraun Feb 27, 2017

bradgreen3 Mar 2, 2017

lbraun Feb 27, 2017

bradgreen3 Mar 2, 2017

lbraun Feb 27, 2017

lbraun Feb 27, 2017

bradgreen3 Mar 1, 2017

		@@ -0,0 +1,3 @@
		FactoryGirl.define do
		# <factories for each model go here>

Factory girl #25

Are you sure you want to change the base?

Factory girl #25

Conversation

bradgreen3 commented Feb 27, 2017

lbraun commented Feb 27, 2017

lbraun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment