-
Users_Controller CRUD expects param[:id] to create User instance. With Orders_Controller, I’d like to retrieve a list of users who have ordered. I know Orders_Controller expects param[:id] to be for creating Order instance. So does this mean if I want to retrieve a list of users who have ordered, I should create a method called ‘get_orders’ in Users_Controller?
-
Can user’s id be passed as the param[:id] for controllers other than Users_Controller? I find that it seems to make CanCan hard to maintain.
-
There seem to be too many methods in Users_Controller (e.g. deactivate, change_role, etc.). How do you organise them/reduce them?
1. Users_Controller CRUD expects param[:id] to create User instance. With Orders_Controller, I'd like to retrieve a list of users who have ordered. I know Orders_Controller expects param[:id] to be for creating Order instance. So does this mean if I want to retrieve a list of users who have ordered, I should create a method called 'get_orders' in Users_Controller?
2. Can user's id be passed as the param[:id] for controllers other than Users_Controller? I find that it seems to make CanCan hard to maintain.
3. There seem to be too many methods in Users_Controller (e.g. deactivate, change_role, etc.). How do you organise them/reduce them?
It seems there are a lot of basic things that you have not yet got the hang of. I suggest that you start by working right through a good tutorial such as railstutorial.org (which is free to use online) which will show you the basics of rails, so that you will be able to answer most questions yourself.
Colin
Thanks for that Colin, after revisiting a few chapters there I understand it better now.
But it seems that User controller will be pretty FAT over time.
What are the strategies to DRY it up?
Thanks for that Colin, after revisiting a few chapters there I understand it better now.
But it seems that User controller will be pretty FAT over time.
Why?
...
But it seems that User controller will be pretty FAT over time.
Why?
Because User has a classic tendency to become a God Object. 
Brandon, sorry if I'm repeating stuff that came earlier, I didn't pay much attention to this thread until the above exchange caught my eye.
The User class tends to accumulate a lot of crud (not to be confused with CRUD), becoming what we call a God Object, one that sees, knows, and tells all.
To combat that tendency, remember the Single Responsibility Principle, and look for clear dividing lines to separate responsibilities. What I usually do is keep User responsible only for logging in and out, so it usually contains only username, encrypted password, and (only for purposes of sending password reset tokens and suchlike) an email address. Any other personal info, like a "real" name, address, phone number, biography, picture, etc. goes in a Profile object that belongs_to that User. Other info for specific aspects of using the system, like availability and desired salary if it's a job board, go in more specific profiles, like JobSeekerProfile or JobPosterProfile.
-Dave
Thanks Dave, does that mean I’m no longer a novice since I found God?
Yes I always though I was doing it correctly by putting everything related to say Order into Order. So I assumed that getting an Order list from User should be a method placed inside Order. Then I noticed it expects to get param[:id] as User.id so I was tempted to put it back to User (the God object). How do reckon I solve this Order list problem?
I’ve been reading a lot about Slim Controller, Fat Model and now looking at my codebase with new lenses but still pretty hard to move stuff into Model. I saw in one Railscast example, Send_password_reset is inside User.rb. Then I went WTF and then soon discovered the Model layer wasn’t meant to map database and can hold logic. Right now I still have a lot of UserMailer stuff in my Controller layer. But I see the tunnel light could be from here:
- design patterns - Fat models and skinny controllers sounds like creating God models - Stack Overflow
I’m also exploring model scope and Has_Scope gem to reduce the amount of methods in Controller. I’m trying to avoid STI and polymorphism stuff which seems to have me change current_user paths to some weird stuff.
And then there is ActiveSupport::Concern to extend the Controller
So I assumed that getting an Order list from User should be a method placed inside Order.
Not necessarily, though it will have to interact with Order at some point. Getting a User's Orders should be as simple as user.orders, assuming orders belong_to users.
Then I noticed it expects to get param[:id] as User.id
Things like this are usually changeable.
so I was tempted to put it back to User (the God object). How do reckon I solve this Order list problem?
IIRC you were after a list of users who have ordered, right? The immediately obvious solution would be something like "User.all.select { |u| u.orders.any? }", but that would have horrible performance as it would fetch the orders for each user in a separate query (N+1 problem). Better would be "User.where(id: Order.pluck(:user_id).uniq)", which would do one query to get the IDs and one to turn them into users. There's probably also some AR syntax to have the DB do the uniq'ing for you, without stooping to hand-rolled SQL, but I forget it offhand.
I've been reading a lot about Slim Controller, Fat Model and now looking at my codebase with new lenses
Here, let me grind down those lenses a bit more. The models
shouldn't be horribly fat either, it's just that it's a *better* place
for business logic than the controller, never mind the view. If a
model is getting unwieldy, you can possibly extract a service object,
or a data object, or some other thing, from it. Let the Single
Responsibility Principle be your guide.
-Dave
This is what my User/Create looks like after rethinking my controller. Does it need more work to make it slimmer?
Basically current_order_id and current_follow_id returns Session values from Application Controller. Basically site visitors can click place an order or follow someone and then after that they create their account as follows. I used CanCan so the loading and authorization is taken care of.
def create
user.updating_password = true
if user.save
sign_in user
if current_order_id.present?
order = Order.find(current_order_id)
order.link_user(user)
elsif current_follow_id.present?
other_user = User.find(current_follow_id)
user.follow_mutually(other_user)
elsif user.membership_plan.present?
UserMailer.plan_registered_notify_admin(user).deliver
else
UserMailer.client_registered_notify_admin(user).deliver
end
redirect_back_or root_url, flash => { :success => ‘Welcome!’ }
else
render ‘new’
end
end
I've seen (and even made) much worse, but this can be slimmed down fairly easily. The sign_in and that big if-statement, have nothing to do with what screen to show next, data to show there other than what's already in some already-used model, or other such things that properly belong in the controller. So, they can be extracted and put into the User model, though you may need to pass in the current_order_id and current_follow_id. You'd wind up with something like:
def create user.updating_password = true if user.save user.process_initial_session(current_order_id, current_follow_id) redirect_back_or root_url, flash => { :success => 'Welcome!' } else render 'new' end end
where user.process_initial_session (or whatever you choose to call it; could be welcome, set_up_stuff, link_to_order_or_followers, whatever, depending what else you may want to put in it) encapsulates all that extracted stuff.
-Dave
Thanks Dave, been spending days cleaning up my code based on your suggestions and pretty proud of it now.
I’ve a dilemma now with CanCan vs Nested Resources in Routes.rb:
In Routes.rb:
resources :users do
resources :orders do
collection do
get :payment_received
end
end
end
In orders_controller.rb:
def payment_received
@user = User.find(params[:user_id])
@orders = Order.where(seller_id: @user.id).order(“id ASC”)
render ‘payment_received’
end
In ability.rb:
can :payment_made, Order, :user_id => user.id
The problem
With the following route:
payment_received_user_orders GET /users/:user_id/orders/payment_received(.:format) orders#payment_received
Through CanCan, I can’t seem to enforce the “:user_id => user.id” whereby the current_user can only see his own payment_received (based on his own user_id) and not someone else’s payment_received.
Found the answer:
can :payment_made, Order, : seller_id => user.id
Then leave payment_received blank or make changes to @orders as follows.
def payment_received
@orders = @orders.order(“id DESC”)
end
This way the user can see his own and not other’s payment_received.
