List/Add/Remove Organization Admins

[craig-riecke]

Addresses #96 and #97. I didn't write tests for these yet, but if there are massive changes that need to be made, I will put them into this pull request. Otherwise, I will do them separately.

[jaydorsey]

Taking a look at this right now

[jaydorsey]

I'll take a 2nd look at this later tonight, but was curious about the use of a querystring to pass parameters vs. using RESTful routes & separate controllers for the admin & membership management.

Currently, a route looks like this: http://localhost:3000/memberships?org_admin=false&organization_id=1

• Changing true to false gives a different list
• We've got all the logic for what is really two different actions listed in a single controller

I'd maybe consider a setup like:

/organizations/1/admins (Returns a list of all admins for the organization)
/organizations/1/members (Returns a list of all members for the organization)

Maybe even something like /members/admin and `/members/employees'.

I'd route both of these to an index method on different controllers.

Questions I still have related to above:

• Can we get the organization ID somewhere else (session) or does it need to be in the route?
• Should we get the route somewhere else--like maybe the application should be multi-tenant and we get it from the domain name (e.g. alders-gate.localhost/members/admin, or maybe even organization.localhost/members/admin for the organization management screens)

I like the latter option but it's slightly more work and we need to think about how a volunteer center & organization appear in the domain (is it organization.volunteer-center.sharechristmas.foo?). If you're open to using sub-domains I think it's worth addressing now so we can make the routes & logic cleaner

[craig-riecke]

I rearranged the routes like that to get away from this pattern:

resources :organizations, only: [:index, ...] do
  members do
    get :admins
    ...
  end
end

Which Jay made the comment on #98 was not REST-ful. But I'm OK with sticking it back to the above if it's there are no issues doing so. (We could do nested resources, but I had bad experiences with them in past versions of Rails. )

As for subdomains, it sounds good - organization.volunteer-center.foo (I certainly wouldn't put the campaign in the domain - didn't know if that's what you meant by sharechristmas.foo above. Just so you know, we're gonna Heroku-ize this for production, since we got free stuff from them. Don't know if that includes domain management, but I'll ask Sean.

Model and Helper tests are covered at 100% now, and I'm hoping to get controllers tonight. We'll see.

[jaydorsey]

Heroku does support wildcard domains so I think we should be OK. Doing so might be a premature optimization at this point, so I'd withdraw my suggestion that we look at it now.

What I'm envisioning is even more simplistic than what you noted above:

  # remove active_admin from the app first
  namespace :admin do
    resources :organization, :volunteer_center, :members
  end

Regardless, I'd punt on re-doing the routes now too now that I think about it. What if we get this PR merged in first? That will give me some time to think about it.

A functioning application has more value than what might (or might not) be a more Rails-like way to do it

[craig-riecke]

Thanks Jay! I took all of your suggestions except changing the test data to "MyStuff". I find that when the model methods pretty much copy data from one place to another, it's more common to botch the field names. In such cases, it's really helpful to see expected 'Willy' but got 'Norfolk, VA 13456'. It also makes the tests read more like documentation.

Rolify really simplified the code, so I applied it to both Volunteer Center and Org Admin roles. But I had to do a massive restructuring of the org admin list/add/delete because it no longer deals with memberships. The resulting code is much shorter and clearer.

I also added controller specs. We are technically at 97% code coverage now.

Sorry for taking so long. I sweated over some of the details too much, and I hope to keep the next pull requests much smaller.

[jaydorsey]

No worries. I'll take a look at this right now.

I'm glad rolify looked good to you

[jaydorsey]

All tests are passing & everything looks good. I had a bunch of comments (again) but they're mostly superficial. I think there's a few things that we could simplify re: sessions & the relationships, but nothing that appears to be a show stopper

I'll let you make the final decision, but LGTM

[craig-riecke]

Good deal! I'm gonna merge this in for now, and probably take most of your advice on the next pull request I work on (which will be much smaller in scope, and shorter turnaround time.)