Clarifying Business Logic and Writing Clean Code (Part 2)

In summary this is my ongoing engineering log as I try to explore how I can clarify the business logic of our app in cucumber and in the application code. It is meandering I know - but I hope there might be some value in sharing my thought process as I work through code on the train, cut off from my prefered mode of work: pair programming ...

So following on from my earlier post on clarifying business logic and writing clean code I spent some time refactoring a number of related cucumber scenarios to make them a bit DRYer and more consistent. One thing I did was list out all the different scenario descriptions so that I could have a look at them independently of the steps. To refresh our memory of the feature, here is the high level description:

Feature: This is my organisation (TIMO)
  As an organisation administrator
  So that I could be set as an admin of our organisation (to edit details, post volunteer ops)
  I want to be able to request for the privilege through our organisation page

Here is the set of original scenarios, their titles extracted from the cucumber:

  1. I am a signed in user who requests to be admin for my organisation
  2. I am not signed in, I will be offered "This is my organisation" claim button (and be able to claim)
  3. I sign in incorrectly once and then sign in successfully after pressing TIMO (and claiming works)
  4. I am not a registered user, I will be offered "This is my organisation" claim button (and claiming works)
  5. I am not a registered user and I claim this is my organisation
  6. I am not a registered user and I sign up incorrectly and then correctly after pressing TIMO

I rewrote them as follows so that they had consistent terminology about the type of user, their goal, and any fails:

  1. I am a signed in user who requests to be admin of an organisation
  2. I am a not signed in user and I request to be admin of an organisation
  3. I am a not signed in user and I request to be admin of an organisation, but fail once on the sign in
  4. I am an unregistered user who requests to be admin of an organisation
  5. I am an unregistered user who requests to be admin of an organisation, but fail once on the sign in

I also removed one scenario that was tested by the subsequent scenario. I was conflicted about whether these were the right things to emphasise at this level. My approach was that I pretty much took the first scenario text and used that as the template. The previous scenario names tell us things specific to the interface, e.g. that we will be offered a button, use abbreviations like TIMO and maybe fail to emphasise what the user wants to achieve. However we then get a fair amount of replication in that in every scenario we are specifying the objective, which is specified at the feature level … so I went further and changed them to

  1. Signed in User
  2. Not Signed in User
  3. Not Signed in User Who Fails Signin Once
  4. Unregistered User
  5. Unregistered User Who Fails Signin Once

which seems more concise, at least viewed like this. Reviewing the individual scenarios I updated them to use the rewrite described in the previous article:

Scenario: Signed in User
    Given I am signed in as a non-admin
    And I click "This is my organisation" on the "Helpful Folk" page 
    Then my request for "Helpful Folk" should be persisted
    And I should see "You have requested admin status for Helpful Folk"
    And I should not see a link or button "This is my organisation"
    And an email should be sent to "admin@myorg.com" as notification of the request

and the four outcomes of the user request became explicit. I could imagine making this even more declarative, to the point that it is comparable with the feature description:

Given I am signed in as a non-admin
When I request admin status for "Helpful Folk"   <-- clicks on TIMO link, stays on page
Then "nonadmin@myorg.com"'s request status for "Helpful Folk" should be updated appropriately <-- request persisted in db, flash alert, TIMO button removed
And an email should be sent to "admin@myorg.com" as notification of request

I wondered briefly if scenario outlines are helpful for further DRYing of these multiple scenarios, but it doesn't feel like it. Since we wouldn't be able to see the components of this high level declarative scenario in the cucumber I decided to leave those steps in for the moment. I put off replacing them with a single step like: "Then my request is persisted, the admin alerted, I get a notification and the button is disabled".

The careful review and updating of all scenarios to be explicit about outcomes allowed me to discover that the Unregistered User scenario has the TIMO button still appearing after a request, which is presumably because we don't have a current_user object to reference against (although perhaps we should be checking the session instead).

I also renamed all the user emails to be more explanatory:

  | email                     | password       | admin | confirmed_at        | organisation | pending_organisation |
  | admin@helpfolk.com        | mypassword1234 | false | 2008-01-01 00:00:00 |              |                      |
  | pendingadmin@helpfolk.com | mypassword1234 | false | 2008-01-01 00:00;00 |              | Helpful Folk         |
  | admin@localsupport.org    | mypassword1234 | true  | 2008-01-01 00:00:00 |              |                      |

Specifically to distinguish the site admin (of LocalSupport) and the organisation admin (of hypothetical organisation 'Helpful Folk'). It also occurred to me that it might make sense to email the requesting user (as well as the site admin) as a confirmation that they have requested the organisation admin status, but decided that sort of change should be left until we see how frequently used this feature is. One of my concerns here was that this is one of a number of pathways for users into the site, and we are not clear if it's one that will be used frequently. The four ways to become an organisation administrator include:

  1. Pre-approved invitation
  2. Submitting a new organisation
  3. Pressing 'this is my organisation' (TIMO) claim button
  4. Signing up with an email that is already associated with an organisation

This reminded me that we have another scenario for the TIMO feature, which is that if the user registers with an email that is already associated with an organisation then they should be automatically approved as admin. Again perhaps this isn't so important to pursue at the moment. Looking back at the four pathways I reflected that we had previously spent a lot of time smoothing the 'pre-approved' invitation pathway, and that TIMO has received a lot of attention. We do also have a sign_up_link_to_org.feature that checks pathway 4 which also needs a review, particularly the interaction it might have with TIMO. However the pathway that is particularly under loved at the moment is 2, where we currently have an out of site google form for making new organisation submissions. Assuming TIMO is sorted, I was inclined to concentrate on that new organisation submission feature; although there had been a start on a volunteer opportunities posting feature which is what might draw more users in, so perhaps that should be higher priority?

Returning to the TIMO scenarios and finishing the review, I established that in the unregistered user's case that the site admin is not sent a notification email - which might be a feature in as much as the site admin might not be notified until the user successfully activates their account … I could argue either way depending on whether we are taking the site admin or user perspective. I also wondered if this might be sent after the confirmation link in the email was clicked - however I added steps to explicitly test that, but there was still no admin email sent, which indicated that we had a real bug there.

Unfortunately I still found myself being distracted by the unDRYness of the six TIMO scenarios in that they had the same four conclusion elements in 5 scenarios:

    Then I should see "You have requested admin status for Helpful Folk"
    And I should not see a link or button "This is my organisation"
    And "admin@helpfolk.com"'s request for "Helpful Folk" should be persisted
    And an email should be sent to "admin@localsupport.org" as notification of the request
    Then my request is persisted, the admin alerted, I get a notification and the button is disabled

As mentioned above I could have extracted these to one step to make the overall feature file a little shorter, but it would have been at the expense of clarity about the expected consequences of a user requesting an admin position; unless we could settle on something like "Then my request is persisted, the admin alerted, I get a notification and the button is disabled" (which might perhaps be made to work with more judicious use of instance variables?). I also noted that the sentence 'And I should not see a link or button "This is my organisation"' is at some level saying "and I should not be able to re-submit my request".

Anyway, the critical issue was clearly the lack of admin notification during the registrations process after TIMO. Digging through the extension to the RegistrationsController that allows us to insert extra functionality into the Devise registrations path I became worried that I didn't understand how this code was working, given that it was setting an instance variable rather than a session variable:

class RegistrationsController < Devise::RegistrationsController
  def create
    if params[:user]
      @pending_org_id = params[:user][:pending_organisation_id]
    end
    super
  end

in contrast with the related code in the SessionsController extension (which handles login rather than signup):

class SessionsController < Devise::SessionsController

  def create
    session[:pending_organisation_id] = params[:pending_organisation_id]
    super
  end
end

I couldn't immediately see how @pending_org_id was related to anything … however if I removed it we didn't get the correct redirect. I then discovered that the presence of @pending_org_id in the registration new page added the hidden form element to the sign up form … however in the sessions form we were storing that same information in the session. Replacing @pending_org_id with session[:pending_org_id] throughout all parts of the registration controller and views (in a precise mirror of the sessions controller) fixed the issue. Now the registration controller and view were both working with a session rather than an instance variable, and so as a result, the after sign in hook that relies on a session variable would send an email to alert the admin after both login and signup - and as a bonus I got to clear up some unused methods and comments that didn't seem to be needed, carefully checking for test failures as I went.

So I was left with the question about whether I should bother refactoring the application code given that things were now working? Particularly since over the weekend I received three updated pull requests from different devs … I decided to review those first - since they'll all be needing merges with my completed pull request for the email admin functionality.

So my outstanding questions include

1) What is the best practice in terms of maintaining a put request over a sign in operation?
2) Can I make use of the workflow gem that xtian suggested as an alternative to cancan and/or permissions matrix that I was imagining?
3) Are nested routes of relevance here here? <-- just to manage the pending_organisation_id parameter perhaps?
4) Should I really be refactoring the service to pull out the business logic and put emailing the admin in an after_save hook on the model?

comments powered by Disqus