Rosters #43

Anbranin · 2017-04-14T17:11:47Z

In testing your code I reviewed it as well, so I think it's fine for now. I fixed some things I found. What do you think of the tests?

Need migration and then V/C

…tions

Validations need work

Still need a way of adding a user to the rotation who's already in another rotation.

Just need to fix Twilio, and then write specs.

Since that's how we moved the method in question as well

…ation

Closes #37.

dfaulken · 2017-04-14T17:59:28Z

Will do, looks like there are some Rubocop issues.

dfaulken

Minor things.

dfaulken · 2017-04-14T18:01:21Z

app/controllers/rosters_controller.rb

+    roster_params = params.require(:roster).permit(:name)
+    roster = Roster.new roster_params
+    if roster.save
+      flash[:message] = 'roster has been created.'


Initial capitalization

dfaulken · 2017-04-14T18:01:35Z

app/controllers/rosters_controller.rb

+
+  def destroy
+    @roster.destroy
+    flash[:message] = 'roster and any assignments have been deleted.'


Initial capitalization

dfaulken · 2017-04-14T18:01:54Z

app/controllers/rosters_controller.rb

+  def update
+    roster_params = params.require(:roster).permit!
+    if @roster.update roster_params
+      flash[:message] = 'roster has been updated.'


Initial capitalization

dfaulken · 2017-04-14T18:06:42Z

spec/controllers/assignments_controller_spec.rb

-      user_1 = create :user
-      user_2 = create :user
-      user_3 = create :user
+      user_1 = create :user, rosters: [@roster] 


Do you think it would be worth writing a spec_helper method like create_user which you pass a roster, just to cut down on the repetition between lines here?

No, because there can't be any users without rosters.

I don't understand your answer.

My question was whether it would be worth writing a method e.g.:

def roster_user(roster) create :user, rosters: [roster] end

So that we could cut down on the repetition here, by calling e.g.:

user_1 = roster_user(@roster)

Then we'd literally never be calling create :user, except to test that it doesn't work without having a roster. My point is there are no users that aren't roster_users.

And my point is that the current code is extremely repetitive.

That is, there are only a few instances when I want a user to belong to a specific roster, so it's repetitive in that case, but very few.

Okay, well, I felt like I was reading the exact same code a bunch but if that's not the case then whatever.

FWIW

grep -r "rosters: \[@roster\]" . | wc -l

gives me 23.

IMHO there's something to be said for WET tests cases. It's great to have total isolation for each example, even if comes at the expense of repetitive LOC.

That being said, if a macro makes sense that that's a way to go here. I think the danger there is that said macro can (and almost always will) grow moving more and more setup logic out of the individual examples making it harder to understand (in the future) why tests fail.

In principle that's true. This example is pretty trivial isolation. If this 'macro' fails, we've got bigger problems.

dfaulken · 2017-04-14T18:07:26Z

spec/controllers/assignments_controller_spec.rb

-        .with @user_ids, Date.today, Date.tomorrow, @starting_user_id
+      # remove all other instances of roster so 'any instance' definitely
+      # refers to our @roster instance
+      Roster.where.not(id: @roster.id).delete_all


OK but why are there any if we're not creating them here

when_current_user_is creates a user. Creating a user creates a roster, since there can never be any users unless there are rosters.

dfaulken · 2017-04-14T18:08:33Z

spec/controllers/rosters_controller_spec.rb

+      expect(Roster.where(id: @roster.id)).to be_empty
+    end
+    it 'destroys the roster' do
+      expect_any_instance_of(Roster)


Why not a single line?

dfaulken · 2017-04-14T18:08:52Z

spec/controllers/rosters_controller_spec.rb

+
+  describe 'GET #new' do
+    it 'renders the new template' do
+      expect(get :new).to render_template :new


Weird parentheses

What do you mean by 'weird'

Eh, never mind, it's not a big deal and Rubocop didn't flag it. Disregard.

dfaulken · 2017-04-14T18:09:15Z

spec/controllers/rosters_controller_spec.rb

+      end
+      it 'redirects to the index' do
+        submit
+        expect(response).to redirect_to(


Why not a single line?

dfaulken · 2017-04-14T18:10:29Z

spec/factories/users.rb

@@ -5,5 +5,6 @@
    sequence(:spire) { |n| n.to_s.rjust(8, '0') + '@umass.edu' }
    sequence(:email) { |n| "user#{n}@umass.edu" }
    sequence(:phone) { |n| '+1' + n.to_s.rjust(10, '0') }
+    rosters { [FactoryGirl.create(:roster)] }


Does one need to say FactoryGirl?

Also, holy brackets, 🦇 👨

dfaulken · 2017-04-14T18:10:42Z

spec/factories/assignments.rb

@@ -1,6 +1,7 @@
 FactoryGirl.define do
  factory :assignment do
-    user
+    roster
+    user { FactoryGirl.create(:user, rosters: [roster]) }


Does one need to say FactoryGirl?

hmm guess not.

fermion · 2017-04-14T20:34:35Z

spec/controllers/assignments_controller_spec.rb

-      user_1 = create :user, rosters: [@roster] 
-      user_2 = create :user, rosters: [@roster]
-      user_3 = create :user, rosters: [@roster]
+      user_1 = roster_user @roster


This is really no better or worse than before

dfaulken · 2017-04-14T20:39:33Z

.rubocop.yml

@@ -1,2 +1,5 @@
 require: umts-custom-cops
 inherit_from: .rubocop-no-umts-cops.yml
+
+HasAndBelongsToMany:


Personally, I like to add comments explaining why we're disagreeing with Rubocop.

dfaulken added 30 commits April 3, 2017 13:24

Update seed file and Gemfile, and write rotation model

76d5b96

Need migration and then V/C

Merge branch 'master' of github.com:umts/screaming-dinosaur into rota…

d24b339

…tions

Add needed columns

5bb0716

Passing specs

533e9a8

Developer login and index work

2b43bad

Rotation edit page works

a49faab

Validations need work

Fixed overlap validation

6a03af1

User edit works

acee8bb

Remove fallback validation from user

74bad9c

New user works

759daf3

Basic rotations index

b8ddc2f

Rotations edit works

f172150

Still need a way of adding a user to the rotation who's already in another rotation.

Initial login logic

44a96c1

Show links for each rotation a person is in

5d76162

Just display calendar for people in only one rotation

9d0bc49

Adding users to multiple rotations

0d67bc6

Generating rotations works

4e95b50

Just need to fix Twilio, and then write specs.

Remove old method

24a2a57

Twilio actions are scoped per-rotation

71c15c1

No special user validations anymore

43acf34

Remove tests for disused model method

4250a08

Move specs from Assignment to Rotation

1226cf3

Since that's how we moved the method in question as well

Fix final failing assignment model test

f09b62c

Complete assignment model tests

8bed699

Make sure Assignment#current is scoped to the target assignments' rot…

78571c1

…ation

Finish porting the previous rotation generation specs

79e0595

Finish rotation model specs

41e3183

Test SessionsController

c14aa68

Test Twilio controller

bf14c3f

Current coverage

4e0a564

dfaulken and others added 17 commits April 5, 2017 09:58

Fix making new assignments and validation

1129106

Closes #37.

Extract links into a partial

2dbbf08

Some links CSS, and show current rotation more obviously

8ef75d7

The end of the styling tweaks

a9cd43b

use correct path

95968d0

fix tests

e40a8ec

change factories to reflect correct attributes

852a130

find rotation before every controller action

883275f

called already before all controller actions

622fbbc

do not override application controller method

9a027e2

called already before all controller actions

39a8e03

fixed specs

610d15a

change spec to be passing

383a843

started new spec file

f2ea6df

rename rotation to roster

dd46bf2

testing rosters

ea9975f

bring test coverage up to 100

23f7b86

Anbranin requested a review from dfaulken April 14, 2017 17:11

dfaulken suggested changes Apr 14, 2017

View reviewed changes

Anbranin added 3 commits April 14, 2017 16:10

write method to create user with specific roster

4684d59

rubocop failures

2280b51

rubocop tweaking

f0603cf

fermion reviewed Apr 14, 2017

View reviewed changes

dfaulken reviewed Apr 14, 2017

View reviewed changes

dfaulken approved these changes Apr 14, 2017

View reviewed changes

dfaulken merged commit e4499d1 into master Apr 14, 2017

dfaulken deleted the rotations branch April 14, 2017 20:42

dfaulken mentioned this pull request Apr 17, 2017

Handle multiple rotations #38

Closed

werebus mentioned this pull request Mar 17, 2021

Destroying a user leaves assignments wilth nil users #28

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rosters #43

Rosters #43

Anbranin commented Apr 14, 2017

dfaulken commented Apr 14, 2017

dfaulken left a comment

dfaulken Apr 14, 2017

dfaulken Apr 14, 2017

dfaulken Apr 14, 2017

dfaulken Apr 14, 2017

Anbranin Apr 14, 2017

dfaulken Apr 14, 2017 •

edited

Loading

Anbranin Apr 14, 2017

dfaulken Apr 14, 2017

Anbranin Apr 14, 2017

dfaulken Apr 14, 2017

dfaulken Apr 14, 2017

fermion Apr 14, 2017 •

edited

Loading

dfaulken Apr 14, 2017

dfaulken Apr 14, 2017

Anbranin Apr 14, 2017

dfaulken Apr 14, 2017

dfaulken Apr 14, 2017

Anbranin Apr 14, 2017

dfaulken Apr 14, 2017

dfaulken Apr 14, 2017

dfaulken Apr 14, 2017

dfaulken Apr 14, 2017

dfaulken Apr 14, 2017

Anbranin Apr 14, 2017

fermion Apr 14, 2017

dfaulken Apr 14, 2017

Rosters #43

Rosters #43

Conversation

Anbranin commented Apr 14, 2017

dfaulken commented Apr 14, 2017

dfaulken 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

dfaulken Apr 14, 2017 • edited Loading

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

fermion Apr 14, 2017 • edited Loading

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

dfaulken Apr 14, 2017 •

edited

Loading

fermion Apr 14, 2017 •

edited

Loading