hackerschool/community — pull request #234 by davidbalbert (2014-09-24 20:36:52+00:00)
Title: Auto subscribe new Hacker Schoolers to Welcome and Housing subforums

This pull request ended up being a bit bigger than I thought it was going to be. I think it is best reviewed commit by commit. Everything but the last commit is small.

Here's what the code does:

1. Auto-subscribers Hacker Schoolers whose batch hasn't started yet to the Welcome and Housing subforums.

A bit about how subscriptions work: A `Subscription` has a `user`, a `subscribable` (either a subforum or a thread), a boolean called `subscribed` which tells us if the user is subscribed or not, and a `reason` (some text to display with the subscription status. If there is no subscription in the database for a given user/subscribable pair, that's also treated as if the user is not subscribed. When we auto-subscribe a user to a subforum, we only do it if there is no subscription in the database. That means if they have ever subscribed or unsubscribed from that subforum, auto-subscribing is a no-op.

2. Fixes a bug where subscriptions weren't getting created on import for new accounts. This was because we were creating subscriptions before saving the new user. This wasn't a huge deal because the subscriptions would get created 24 hours later when the importer ran again and the users were no longer new, but it's still a bug and it may have been causing me to get duplicate subforums (see the next list item).

3. Ensure that subforums must have unique names. Zach and I decided on this a while ago, but we never put it into code. While I was working on the main feature in this pull request (the first item in this list), I ended up with 5 subforums per name. I haven't been able to reproduce the bug since, but now that the code validates uniqueness of subforum names, we should learn about it pretty quickly. This involved changing the seed data a bit.

4. Refactored account creation logic into a separate class. Before, it all lived in `User.create_or_update_from_api_data`. Now it is in the `AccountImporter` class. The old function was getting big, imperative, and hard to reason about. I wanted some helper methods, so I moved the logic into its own class. I think it's much nicer to read, but it means this PR is a good bit bigger than it would have been :/. The refactor is in the final commit. In addition to the refactor, the final commit also includes a few more changes:

1. The way we do subforum lookup by name for auto-subscribing has changed. We used to look each one up individually with `first!` to insure that we got an error if any of them didn't exist. Adding Welcome and Housing added two more queries, which isn't a big deal for account import, but we also import account creation on login, so having 1 query instead of 4 is better. We now throw a custom `ImportError` if the number of subforums we get from the database isn't the same as the number of names we asked for.

2. The user info update and the subforum auto-subscription is now wrapped in a transaction. That means if either of them fails, none of them will succeed. There is no special significance to the fact that the `transaction` method is called on `User`. It may as well have been called on `ActiveRecord::Base`. Transactions are connection specific and there is one connection for the lifetime of a request.

3. The criteria for being added to Welcome and Housing changed during the final commit. The first way I did it was checking to see if the user was a new record. This was a problem because I didn't know if Mary had already created the new users on hackerschool.com. If she had created them yesterday, they would have already been imported into Community last night, and when `AccountImporter.import_all` gets run tonight, they would already be in the database and not get auto-subscribed correctly. The new way is to auto-subscribe Hacker Schoolers whose batch hasn't started yet. I think this is less brittle.