[51] Add status enum to Draws with intent deadline #95

orenyk · 2017-01-06T06:17:35Z

Resolves #51

Add status enum to draws (current values 'draft' and 'pre-lottery', with 'lottery' and 'post-lottery' to be implemented in the future)
Add button to initiate draw on draw show view with appropriate route
Add DrawStarter to check validity of draw setup and handle transition to pre-lottery stage.

esoterik

a few comments!

also, I think that this should possibly add draw status validations to groups? that might be enough to be its own issue, though.

esoterik · 2017-01-08T03:58:10Z

app/helpers/draw_helper.rb

+module DrawHelper
+  # Check to see if a draw is startable by the current user. Returns true if the
+  # draw is in the 'draft' status and the user satisfies the Pundit policy,
+  # false otherwise.


I think this should be handled entirely in pundit

Ha, thanks for catching that. I forgot we could do more than assert on user in policies 😛. I'll update.

esoterik · 2017-01-08T04:00:52Z

app/services/draw_starter.rb

+  # Class method to permit calling :start on the class without instantiating the
+  # service object directly
+  #
+  # @attr draw [Draw] the draw in question


I think we should just enforce instantiation or make this a module instead of a class

I actually prefer this paradigm for service objects and I'm surprised that I forgot to mention it when we started implementing them. I picked it up from the Upcase forums when I was doing some of the Ruby challenges: this is the thread and here's a great article on it. Take a look and see if it changes your mind; the reason why we wouldn't want a module is because modules don't maintain state, making refactoring and method extraction difficult, and instantiation just feels unnecessary for single-use objects like this. I really think we should stick with it moving forward and potentially even refactor our controllers and existing service objects to follow that model.

esoterik · 2017-01-08T04:03:36Z

app/views/draws/show.html.erb

@@ -12,5 +12,8 @@
    </tbody>
  </table>
 </div>
+<% if can_start_draw?(draw: @draw, policy: policy(@draw)) %>


policy(@draw).start? should work given my suggested change to the policy above

yea it's what I originally had and then I starting thinking about @draw.status and for some reason forgot that the policy could include that as well 😕.

esoterik · 2017-01-08T04:05:03Z

spec/features/draws/draw_start_spec.rb

+
+      expect(page).not_to have_link('Begin draw process')
+    end
+  end


I'm hesitant to leave these permission-based feature specs long-term. I think they're good when behavior differs greatly between roles, but might be overkill for things like seeing a button.

Perhaps we want to have some view specs instead?

Hm, maybe? I don't have a lot of experience with View specs in Rails; I've done a bit of that in Phoenix but there everything is basically unit testable so it's really easy. I'll take a look and see about moving everything around.

esoterik · 2017-01-08T04:08:55Z

app/policies/draw_policy.rb

@@ -9,6 +9,10 @@ def update?
    user.rep? || user.admin?
  end

+  def start?
+    user.admin?


changing this to user.admin? && record.draft? eliminates the need for DrawHelper

Way to keep hammering the point home :-).

esoterik · 2017-01-08T04:16:00Z

spec/services/draw_starter_spec.rb

+  end
+
+  def create_mock_draw_starter(param_hash)
+    draw_starter = double('draw_starter')


I think that instance_spy is generally the preferred mock method (thoughtbot post), though there are trade-offs.

If this is kept as a standard mock, though, it should be a verifying double so it checks that the stubbed methods are present on the actual class/instance of said class

Good catch, I wasn't 100% sure why we kept using instance methods so figured I'd keep it simple for now, thanks for the reference.

Yea after reading the post I'm convinced. I'll switch them everywhere in the tests.

esoterik · 2017-01-08T04:18:15Z

app/controllers/draws_controller.rb

@@ -31,6 +33,11 @@ def destroy
    handle_action(**result)
  end

+  def start


this is super nit-picky but I feel like start is too ambiguous between opening up the draw to students and running the lottery -- open might be better for this transition?

Agreed; especially since starting the draw could mean starting the actual draw / lottery phase (this was already bothering me). How about initiate? open doesn't really work for me - I get that the implication is "open for business," so to speak, but it feels like weird verb to use DrawOpener vs DrawInitiator. Thoughts?

esoterik

just took a look at the spec persistence / mocking issues

esoterik · 2017-01-09T04:04:54Z

spec/services/draw_activator_spec.rb

@@ -0,0 +1,91 @@
+# frozen_string_literal: true


this is missing require rails_helper

Ha good catch. That's the second time I did that 😕

Wait, if that's the case, why is this test even running? Do we even need it?

it configures DatabaseCleaner and other things, so we need it

Yea good point.

esoterik · 2017-01-09T04:26:39Z

spec/services/draw_activator_spec.rb

+  # doesn't check for enum validity. I tried switching to FactoryGirl's
+  # `build_stubbed` but I ran into major issues with data being persisted or
+  # being unable to generate a valid draw for the service object without
+  # persisting all the things. Thoughts?


re: verifying doubles not verifying:
I tested this a bit locally and I'm surprised because I didn't run into any of this when refactoring specs for reservations. I did a small amount of research and you're right, it has to do with the enum methods being dynamically defined.

It does mean that we're opening ourselves up to some risk when mocking, but I think it's worth it overall to not persist data for testing method details. We'll still have integration tests to cover us on major behaviors, and I don't think there's a better solution for verifying the mocks.

re: issues with build_stubbed:
ActiveRecord associations do not play well when some of your data is mocked and some of it is real, I know that pain. Here I don't think we'll have to deal with it (I think I fixed the persisted spec above, though I could be wrong), but long-term we might want to pull over my Mocker class from reservations to make dealing with associations easier when we don't want to persist data.

Thanks for confirming my suspicions and validating that I wasn't being entirely stupid. I'm open to implementing the Mocker again if we feel we need it - I think we've been ok without the additional complexity and just manually dealing with instance_spys, but we can come back to this if it comes up.

esoterik · 2017-01-09T04:28:30Z

spec/services/draw_activator_spec.rb

+      result = described_class.activate(draw: draw)
+
+      expect(result[:obj]).to be_pre_lottery
+    end


I was able to get the following to pass without persistence:

draw = instance_spy('Draw', draft?: true, has_students?: true, has_enough_beds?: true) described_class.activate(draw: draw) expect(draw).to have_received(:update_attributes) .with(status: 'pre_lottery')

Which I think tests what we want it to?

Yea..... I don't think it solves my problem (the enum could change and this spec would still pass), but that may be fine as long as we have at least one integration test that would fail if the enum changed, which is what we want. I was implicitly testing this by stubbing update_attributes(status: 'pre_lottert') to return the updated spy with pre_lottery?: true stubbed, but your assertion is better as we're really testing the message sent out of the service object, not the outcome.

That said, should we? I feel like we should only send the invitation e-mails if that update is successful, so as a sanity check should we make that action dependent on the result of update_attributes?

orenyk · 2017-01-10T17:12:45Z

@esoterik ok I think this is ready!

orenyk · 2017-01-10T17:14:08Z

This is going to fail rubocop, btw, I'll fix those after the build.

orenyk · 2017-01-10T17:14:27Z

I'll also check out codeclimate in a bit.

orenyk · 2017-01-10T19:35:26Z

The only CodeClimate failures at this point should be some TODO comments in the code - can we disable that check? I'm kind of a fan of leaving notes to our future dev selves in the code where relevant. Let me know what you think!

esoterik

a few small comments!

esoterik · 2017-01-11T23:09:01Z

app/controllers/draws_controller.rb

+  # the intent metrics to render properly.
+  def calculate_metrics
+    @intent_metrics = IntentMetricsQuery.call(@draw)
+  end


I think that this is fine

esoterik · 2017-01-11T23:13:44Z

app/mailers/student_mailer.rb

+#
+# Mailer class for user e-mails
+class StudentMailer < ApplicationMailer
+  default from: 'no-reply@vesta.app' # SHOULD WE HAVE AN ADMIN ADDRESS?


we should probably have an admin address; it will probably make sense for students to want to respond to emails individually in some circumstances.

this can be a separate issue at this point I think

Yea, that's what I was thinking.

I also think it can be replaced with our eventual config model, so I'll open an issue to that effect.

esoterik · 2017-01-11T23:15:59Z

app/mailers/student_mailer.rb

+    @user = user
+    @intent_deadline = user.draw.intent_deadline
+    # TODO: figure this out - singleton class, e.g. app config, or do we add a
+    # class for now that we later use for multitenancy?


I think it should end up as an app config model; it'll work better when we have multi-tenancy

Yea exactly. I think that some kind of high-level model for the root-level user (similar to Department in Shifts) will work fine.

esoterik · 2017-01-11T23:17:41Z

app/models/draw.rb

+
+  # TODO: I had originally made this public but now I'm not sure it needs to be.
+  # Let me know if you'd like to keep it private or not and we can remove the
+  # YARD docs and the spec.


I think its fine for these to be private

Ok, should I remove the documentation? I'll definitely remove the test.

esoterik · 2017-01-11T23:18:44Z

app/services/draw_activator.rb

+    validate
+    activate_draw if valid?
+    send_emails if valid?
+    valid? ? success : error


~~I think it makes sense to wrap this all in one if~~

hmm, this isn't the biggest problem, but it's unclear that validity can change between the steps of this method. Not quite sure what to suggest, and it's also fine as it is -- just something I thought while reading through the class

changing the method name to currently_valid?

esoterik · 2017-01-11T23:26:48Z

spec/features/draws/draw_start_spec.rb

+      visit draw_path(draw)
+      expect(page).not_to have_link('Begin draw process')
+    end
+  end


I still think we're better off using view specs for these kinds of things

I'll open an issue

esoterik · 2017-01-11T23:27:52Z

spec/models/draw_spec.rb

+  # TODO: should we keep this? I originally wrote this before I decided to make
+  # #student_count private, but we can make it public again if we think it's
+  # worth it.
+  #


might as well keep it, it doesn't persist data so it's fast, and we can always delete it later

I had to modify the expectation since RSpec fails if you call a private method directly, so instead I used send. I also left a comment that it should be removed if it ever fails.

esoterik · 2017-01-11T23:29:14Z

spec/services/draw_activator_spec.rb

@@ -0,0 +1,95 @@
+# frozen_string_literal: true
+require 'spec_helper'


notably our spec_helper and rails_helper configurations are different -- I've been using rails_helper everywhere because of the DatabaseCleaner config

Should DatabaseCleaner be in spec_helper since it's not Rails specific?

hmm. I think that any tests that persist data will be Rails-specific, so it's probably fine how it is?

esoterik · 2017-01-11T23:30:11Z

spec/services/draw_activator_spec.rb

+  end
+
+  def create_valid_mock_draw_with_students(num_students: 2)
+    students = Array.new(num_students) { |_n| instance_spy('user') }


The block argument isn't necessary here

good catch!

esoterik · 2017-01-11T23:45:39Z

spec/services/draw_activator_spec.rb

+    end
+  end
+
+  def create_mock_draw_activator(param_hash)


I think that the create_ prefix can be dropped from these method names

esoterik · 2017-01-11T23:50:36Z

re: codeclimate erroring on the TODOs:

I think we should keep the checks because otherwise TODOs end up getting lost in the codebase, but we can keep some of them in place.

Resolves #51 - Add status enum to draws (current values 'draft' and 'pre-lottery', with 'lottery' and 'post-lottery' to be implemented in the future) - Add intent deadline to draws - Add button to initiate draw on draw show view with appropriate route - Add DrawActivator to check validity of draw setup and handle transition to pre-lottery stage - Add StudentMailer with initial student invitation e-mail - Add DrawsHelper with method to generate a string describing the relative time since/until the intent deadline

orenyk · 2017-01-12T05:05:31Z

@esoterik ok I removed all but the one TODO we discussed and addressed the other issues. My only last concern is the rails_helper vs spec_helper question - see my note above about DatabaseCleaner. I'm fine to just use rails_helper everywhere, but I've read articles suggesting not to (here's one example). Let me know what you think.

Also squashed 😄. Thanks!

esoterik · 2017-01-13T18:53:54Z

this is good to go pending resolution of the DatabaseCleaner config

orenyk force-pushed the 51_draw_phase_intent_deadline branch from c644d0b to a4034ee Compare January 6, 2017 06:18

esoterik requested changes Jan 8, 2017

View reviewed changes

esoterik reviewed Jan 9, 2017

View reviewed changes

orenyk changed the title ~~WIP: [51] Add status enum to Draws with intent deadline~~ [51] Add status enum to Draws with intent deadline Jan 10, 2017

orenyk requested a review from esoterik January 10, 2017 17:12

orenyk added the 0 - needs review label Jan 10, 2017

esoterik requested changes Jan 11, 2017

View reviewed changes

esoterik removed the 0 - needs review label Jan 11, 2017

orenyk force-pushed the 51_draw_phase_intent_deadline branch from d4c1d86 to 065274c Compare January 12, 2017 04:58

orenyk mentioned this pull request Jan 12, 2017

Replace draw start feature spec for students with view spec #102

Open

orenyk added the 0 - needs review label Jan 12, 2017

esoterik approved these changes Jan 13, 2017

View reviewed changes

esoterik removed the 0 - needs review label Jan 13, 2017

orenyk merged commit 8091e70 into master Jan 13, 2017

orenyk deleted the 51_draw_phase_intent_deadline branch February 7, 2017 04:19

		@@ -0,0 +1,95 @@
		# frozen_string_literal: true
		require 'spec_helper'

[51] Add status enum to Draws with intent deadline #95

[51] Add status enum to Draws with intent deadline #95

Conversation

orenyk commented Jan 6, 2017 • edited

esoterik 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

esoterik 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

orenyk commented Jan 10, 2017

orenyk commented Jan 10, 2017

orenyk commented Jan 10, 2017

orenyk commented Jan 10, 2017

esoterik 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

orenyk Jan 12, 2017 • edited

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

esoterik commented Jan 11, 2017

orenyk commented Jan 12, 2017

esoterik commented Jan 13, 2017

orenyk commented Jan 6, 2017 •

edited

orenyk Jan 12, 2017 •

edited