Rails controller problems

[Brandon]

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?

 

[Colin Law]

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


>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Ruby on Rails: Talk" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to rubyonrails-ta...@googlegroups.com.
> To post to this group, send email to rubyonra...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/rubyonrails-talk/18a42aa1-a4fb-425b-a182-1db5faff0a6f%40googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.

[Brandon]

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?

[Colin Law]

On 3 March 2014 09:18, Brandon <wong...@gmail.com> wrote:
> 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?

> https://groups.google.com/d/msgid/rubyonrails-talk/3b5396b5-45e0-4f08-a7f4-705e4d31653e%40googlegroups.com.

[Dave Aronson]

On Mon, Mar 3, 2014 at 5:36 AM, Colin Law <cla...@gmail.com> wrote:

> On 3 March 2014 09:18, Brandon <wong...@gmail.com> wrote:

...

>> 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

--
Dave Aronson, the T. Rex of Codosaurus LLC (www.codosaur.us);
FREELANCE SOFTWARE DEVELOPER, AVAILABLE AS OF MARCH 1st 2014;
creator of Pull Request Roulette, at PullRequestRoulette.com.

[Brandon]

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:

- http://stackoverflow.com/questions/14044681/fat-models-and-skinny-controllers-sounds-like-creating-god-models

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

http://stackoverflow.com/questions/18745021/the-best-way-to-split-up-a-large-rails-controller

[Dave Aronson]

On Mon, Mar 3, 2014 at 11:14 AM, Brandon <wong...@gmail.com> wrote:

> 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.

[Brandon]

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

[Dave Aronson]

On Mon, Mar 3, 2014 at 4:01 PM, Brandon <wong...@gmail.com> wrote:

> This is what my User/Create looks like after rethinking my controller. Does
> it need more work to make it slimmer?

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.

[Brandon]

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.

[Brandon]

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.