loomio/loomio — pull request #625 by craigambrose (2013-04-02 09:23:41+00:00)
Title: Refactor email preferences

Here's a little refactoring that I did while I was off sick today. I looked for one of the issues reported by code climate to tackle. The problem methods that I decided to fix were group_email_preferences getter and setter inside user. Code climate is reporting that the setter is too complex, but also the User class in general is too complex.

I could see some ways to clear up that setter method, but in general this functionality didn't appear to belong in user. There is clearly some concept of email preferences that needed an object of it's own.

I haven't managed to succesfully squash my three commits, despite doing the rebase, perhaps because I created a feature branch after doing the commits. Anyway, if nothing else it demonstrates the steps I took.

Since I am now both reading and writing email preferences through an email preferences object, the controller needed to reflect that. The preferences edit form was an action in the UserController, so that was easy enough to add my object into. However, that form submitted to the general UserController#update action, and I needed to update a different object, not the user. I could have added an update_preferences action to the UserController, but I decided that this was a resource of it's own and deserved it's own controller.

I have preserved the route for the edit preferences page, and it still works if accessed by emails (great cucumber coverage there guys, well done). I have added a new route of course for handling that update, but it still redirects to the same place.

The only user noticable difference in this refactoring is that the flash message now says Your email settings have been updated." rather than just "Your settings have been updated.". Obviously that could have been left the same but since I now now which settings are being saved, it was an easy change to make.

My EmailPreference object has turned out to be something like a Decorator, but it's not a true implementation of the Decorator pattern as it doesn't let all messages go through to User, just specifically delegated ones. From the point of view of code using the EmailPreference object, it should ideally just appear to be an object that you can use to get and set email preferences, and only for that.

I've put it in the extras directory, which is where you guys seem to be putting POROs. However, I'm not sure I really endorse this structure. This class is clearly part of the application logic, and should be somewhere inside /app.