Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Made changes to instance add/edit code to remove AR Objects from @edit #139

Conversation

h-kataria
Copy link
Contributor

  • Changed code to add/edit Instance to not store AR Objects in @edit instead store required fields into array of hashes. Made some minor cleanup along the way to methods edited in this commit.
  • Changed view to use hashes to access data instead of AR Object.

https://bugzilla.redhat.com/show_bug.cgi?id=1114747
https://bugzilla.redhat.com/show_bug.cgi?id=1115652

@dclarizio please review/test.

@@ -840,7 +842,7 @@ def update_instance
@in_a_form = false
replace_right_cell
when "save"
if @edit[:new][:ae_inst].name.blank? || @edit[:new][:ae_inst].name == ""
if @edit[:new][:ae_inst]["name"].blank? || @edit[:new][:ae_inst]["name"] == ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why check for == "" if code is already checking for .blank? ?

- Changed code to add/edit Instance to not store AR Objects in @edit instead store required fields into array of hashes. Made some minor cleanup along the way to methods edited in this commit.
- Changed view to use hashes to access data instead of AR Object.

https://bugzilla.redhat.com/show_bug.cgi?id=1114747
https://bugzilla.redhat.com/show_bug.cgi?id=1115652
@h-kataria
Copy link
Contributor Author

@chessbyte made suggested changes.

@dclarizio
Copy link

@h-kataria Getting this when I press Save after making some changes to an instance:
[----] I, [2014-07-03T20:34:36.092221 #34212:3ff345c34ce0] INFO -- : Started POST "/miq_ae_class/update_instance/10000000000684?button=save" for 127.0.0.1 at 2014-07-03 13:34:36 -0700
[----] I, [2014-07-03T20:34:36.225084 #34212:3ff345c34ce0] INFO -- : Processing by MiqAeClassController#update_instance as JS
[----] I, [2014-07-03T20:34:36.225218 #34212:3ff345c34ce0] INFO -- : Parameters: {"button"=>"save", "id"=>"10000000000684"}
[----] F, [2014-07-03T20:34:36.293659 #34212:3ff345c34ce0] FATAL -- : Error caught: [NoMethodError] undefined method strip' for nil:NilClass /Users/dclarizio/dev/manageiq/vmdb/app/controllers/miq_ae_class_controller.rb:2398:inblock in set_instances_record_vars'
/Users/dclarizio/dev/manageiq/vmdb/app/controllers/miq_ae_class_controller.rb:2397:in `each'

Added a missing header column "On Error" in Instance grid, that was causing grid to display incorrect data in rows.

https://bugzilla.redhat.com/show_bug.cgi?id=1114747
https://bugzilla.redhat.com/show_bug.cgi?id=1115652
@miq-bot
Copy link
Member

miq-bot commented Jul 3, 2014

Checked commits h-kataria@5df1781 .. h-kataria@bc641b3 with rubocop 0.21.0
1 file checked, 0 offenses detected
Everything looks good. 👍

@martinpovolny
Copy link
Member

Do we need variable called @temp can't we have something more specific? And could not we refactor this bit to avoid the instance variable and pass values directly to the template? Just asking..

@dclarizio
Copy link

@martinpovolny We have used the @temp instance var to hold values needed by the views, but that don't need to be stateful (i.e. needed after the current transaction), knowing it will be thrown away at the end of the transaction.

Should we have something more specific? Perhaps on a case by case basis, but generally I don't think so due to the next answer.

Refactor to remove instance vars from the views? Yes, we should do this ongoing, perhaps some of the more resource intensive, data providing, values could be created in helper methods so the controller can build the values, but the views can call the helper methods to fetch a cached value when needed.

Open to other suggestions as well.
Thx, Dan

@dclarizio
Copy link

Tested automate explorer and verified session object never goes higher than 20K.

dclarizio pushed a commit that referenced this pull request Jul 7, 2014
…tance_editor

Made changes to instance add/edit code to remove AR Objects from @edit
@dclarizio dclarizio merged commit eeb8d6f into ManageIQ:master Jul 7, 2014
@dclarizio dclarizio deleted the removed_ar_objects_from_ae_instance_editor branch July 7, 2014 17:30
@martinpovolny
Copy link
Member

@dclarizio : yes, I would prefer something more specific basically in all cases. If we need an instance variable I would call it for example @temp_host or @temp_view whatever it holds.

Later this is sooo much easier to refactor, if you see just the corresponding occurrences when you git grep... and don't have to match the places where @temp is used for different purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants