Class Parameters

[kbrezina]

Class parameter changes were tweaked to be in compliance with Rails 3.

[sodabrew]

GitHub says that this cannot be automatically merged -- you might need to rebase up to current rails3 branch?

[kbrezina]

Hmm, I'll need to rebase again. There are a few more commits.
I'll do that on Monday.

[jeffweiss]

Does it help to check for the Boolean true in addition to its String representation?
We could also DRY this a bit with

[:force_create, :force_update, :force_delete].each do |param|
  self.class.send(:define_method, (param.to_s+"?").to_sym) do
    !params[param].nil? && (params[param] == "true" || param[param] == true)
  end
end

I'm not sure if this detracts from readability though...

[kbrezina]

• I simplified the conditions. The "!params[:force_xyz].nil?" was redundant.
• if possible I'd keep the "def force_xyz? ... end" structure. IMHO, it's more readable than your version with 'self.class.send' and it's easier to find definition of such method. However, if you insist on it I can change it. :-)

[kbrezina]

Why do you think the Boolean true should be checked in addition to its String representation? "params[:force_xyz]" should always contain a String.

[jeffweiss]

The update is much better.

[jeffweiss]

I've just done a cursory examination of the content. I'll dive in today.

[jeffweiss]

At first glance, it feels like we have a lot of duplication between NodeClassMembershipsController#update, NodeClassMembershipsController#destroy, NodeGroupClassMembershipsController#update, NodeGroupClassMembershipsController#destroy, NodeGroupsController#create, NodeGroupsController#update, NodeGroupsController#destroy, NodesController#create, NodesController#update, and NodesController#destroy.

I'm going to spend a bit digging in to determine if that's just perception or if we can maybe reduce duplication.

[jeffweiss]

@fhrbek @kbrezina I opened a pull request fhrbek#1 against this one with a commit that reduces a great deal of the redundancy between the various controllers.

[jeffweiss]

I will sometimes get this error when trying to delete a node from a group

ArgumentError (wrong number of arguments (2 for 1)):
  lib/has_many_through_dependent.rb:3:in `delete_records'
  lib/find_from_form.rb:29:in `block (2 levels) in assigns_related'
  app/controllers/node_groups_controller.rb:76:in `block in update'
  app/controllers/node_groups_controller.rb:73:in `update'


  Rendered /Users/jeff/.rvm/gems/ruby-1.9.3-p392@dear_lord_why/gems/actionpack-3.2.13/lib/action_dispatch/middleware/templates/rescues/_trace.erb (1.1ms)
  Rendered /Users/jeff/.rvm/gems/ruby-1.9.3-p392@dear_lord_why/gems/actionpack-3.2.13/lib/action_dispatch/middleware/tem
...

This does not happen with the normal rails3 branch.

[fhrbek]

The problem with lib/has_many_through_dependent has been fixed in #207 and that's already merged in rails3. This PR should be rebased and the problem will be gone.

[jeffweiss]

ah cool, ok.

[jeffweiss]

Confirmed that merge does indeed fix the problem; however, now I seem to get

{"status":"ok","valid":"true","redirect_to":"http://localhost:3000/nodes/1"}

as the content returned when I edit a node...

[kbrezina]

Jeff, I cannot reproduce the problem with the displayed json content.

Could you provide exact steps to reproduce it?
What browser are you using? It works fine on FF 19.0.2 & Chrome 26.0.1410.43 (Fedora17).

[jeffweiss]

@kbrezina the problem started when I locally merged the PR into the rails3 branch. It didn't occur solely with this PR's branch. I was using Chrome, but I don't think that matters.
I'll try again first thing in the morning.

[jeffweiss]

Turns out I had stale precompiled assets from before merging. Accurately detecting conflicts now. I'm gonna do one more once over on the code.

[sodabrew]

Congrats!! May we close #142 and #182 now that this feature has landed?

[fhrbek]

Yes, #142 and #182 have become obsolete since all their features have already been merged into the target rails3 branch. It's time to clean up.

[ryanycoleman]

@fhrbek @kbrezina, I saw this demoed by @jeffweiss today. This is awesome! Thank you so much for getting this in.