Fix error when viewing timelines in Optimize-Bottlenecks in IE #3529

bmclaughlin · 2015-07-20T14:32:58Z

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

epwinchell · 2015-07-21T19:07:03Z

@dclarizio Fix verified in IE9

dclarizio · 2015-07-22T14:01:06Z

@bmclaughlin Can we try to consolidate the duplicated code into a common method? Thx, Dan

bmclaughlin · 2015-07-22T14:08:36Z

@Fryguy, @tenderlove, do you have any recommendations on DRY'ing up the getTLdata method between an application controller concern and two other controllers?

https://github.com/ManageIQ/manageiq/blob/master/app/controllers/application_controller/timelines.rb#L130

https://github.com/ManageIQ/manageiq/blob/master/app/controllers/dashboard_controller.rb#L597

https://github.com/ManageIQ/manageiq/pull/3529/files#diff-8c3427b37f1658d1cc09ffb1b7d975e5R67

tenderlove · 2015-07-22T14:40:25Z

Yes, we should definitely dry these up. Are they exactly the same? Just at a glance they look very similar. We should extract a function with the similar things, then extract the different bits. Sorry, that's a little vague, but essentially we should use the Extract Method technique a couple times. I can show you a patch to get you started if that helps!

bmclaughlin · 2015-07-22T14:48:40Z

Not exactly the same, I found a one line difference that will be easily solved with one parameter. I wasn't sure where the best home for the extracted method would be (application helper or application controller?)?

tenderlove · 2015-07-22T14:52:18Z

@bmclaughlin well it looks like this method creates XML that is only to do with VM instances. I would either move it to a VM related model, or a VM related presenter ("presenter" being code word for "plain old ruby object"). If we don't have a VM related presenter, I don't see why we shouldn't add one! :)

chessbyte · 2015-07-24T21:23:51Z

app/presenters/vm_presenter.rb

@@ -0,0 +1,12 @@
+class VmPresenter
+  def initialize(controller, session)
+    @controller = controller


Why do we pass in the controller into this class if we do not use it? Is that some sort of pattern for presenters?

In reading here and here and here, this object should be initialized with a Vm, but instead, is getting a controller and session. It is not using the controller and I don't think it should be using the session. What does session[:tl_xml_blob_id] represent?

@tenderlove @matthewd @Fryguy Please comment on how we should use Presenters.

I missed removing the passed in controller when refactoring the original code, thanks for catching that. The original code also made use of a Vm but that turned out to be dead code and was removed. Now in hindsight I see that I can pass the session value directly to the function as well. I'll make these changes momentarily.

Having been reduced to "get the thing from the DB object", this is no longer feeling like the place to have a presenter discussion, to me. Let's just inline the BinaryBlob.find back into the callers.

dclarizio · 2015-07-27T20:25:57Z

@bmclaughlin need to correct the failing spec.

bmclaughlin · 2015-07-27T20:58:33Z

@chessbyte @Fryguy @dclarizio @matthewd, recommended changes implemented, please review.

matthewd · 2015-07-27T21:07:28Z

if session[:tl_xml_blob_id] != nil should probably be spelled if session[:tl_xml_blob_id]

... but is it even useful? AFAICS, if it's not true, we'll fail to render, and (given there's no corresponding view) still end up with an error either way. So I think we can just lose it.

matthewd · 2015-07-27T21:13:41Z

Err, we seem to have lost a destroy... was that deliberate?

dclarizio · 2015-07-27T22:44:07Z

@bmclaughlin Travis failure, seems a spec still has the CamelCased route in it.

bmclaughlin · 2015-07-28T13:55:39Z

@matthewd, I agree on the nil check, and I lost the destroy on purpose as BinaryBlob is no longer being instantiated to a variable.

@dclarizio, fixed that pesky spec.

chessbyte · 2015-07-28T14:00:55Z

@bmclaughlin the destroy removes the row from the database. Not sure I am following your explanation of why it was removed. (cc @matthewd)

bmclaughlin · 2015-07-28T14:07:08Z

@chessbyte, apologies, thought that was destroying the object in memory. Need more coffee. Patch coming momentarily!

chessbyte · 2015-07-30T14:57:52Z

Patch coming momentarily!

bump @bmclaughlin

bmclaughlin · 2015-07-30T15:02:03Z

@chessbyte, 'Momentarily' became slightly longer due to a double rendering issue between rendering the view and rendering the xml for IE.

bmclaughlin · 2015-08-03T20:49:36Z

@chessbyte, @dclarizio this should be ready when travis finishes.

chessbyte · 2015-08-03T20:54:45Z

@bmclaughlin there still seems to be code duplication between the new multiple timeline_data methods. Will that be refactored at some point via a new PR?

chessbyte · 2015-08-03T21:12:37Z

@tenderlove @Fryguy @matthewd what is the best way to refactor the duplicate timeline_data methods? Is it via mixin, concern or some other technique/pattern?

chessbyte · 2015-08-03T21:26:38Z

app/controllers/application_controller/timelines.rb

-    if session[:tl_xml_blob_id] != nil
-      blob = BinaryBlob.find(session[:tl_xml_blob_id])
-      render :xml=>blob.binary
+  def timeline_data


If this timeline_data method is in ApplicationController, why do we need the duplicate methods in subclassed controllers (DashboardController and MiqCapacityController)?

Indeed. Though, as it's apparently only used by three routes, I think I'd rather reduce the massive ApplicationController surface area by one method, and move it to whichever third controller is currently actually capable of getting here.

matthewd · 2015-08-03T22:35:01Z

... but is it even useful? AFAICS, if it's not true, we'll fail to render, and (given there's no corresponding view) still end up with an error either way. So I think we can just lose it.

matthewd · 2015-08-03T22:37:42Z

@chessbyte I don't think there's enough code there to duplicate, to warrant extraction. Especially if my above allegation is true.

- DRY'd and refactored getTLdata method - added additional routing specs - Fixed double render issue caused by partial _bottlenecks_tl_detail.html.haml being rendered directly as well as indirectly via _bottlenecks_tabs.html.haml https://bugzilla.redhat.com/show_bug.cgi?id=1234588

bmclaughlin · 2015-08-04T17:37:11Z

@chessbyte, @matthewd Originally the miq_capacity_controller was throwing errors when trying to access the copy of timeline_data in application controller, however that seems to have been due to an intervening bug and the duplicate code is now properly DRY'd out as well as the suggested optimizations implemented. And I squashed the commits.

miq-bot · 2015-08-04T17:38:57Z

Checked commit bmclaughlin@2941dc7 with rubocop 0.32.1 and haml-lint 0.13.0
6 files checked, 0 offenses detected
Everything looks good. 🍰

chessbyte · 2015-08-04T17:39:22Z

@bmclaughlin Nice and DRY! Waiting for Travis to go green before merging.

…ne-error Fix error when viewing timelines in Optimize-Bottlenecks in IE

matthewd · 2015-08-04T19:03:13Z

@bmclaughlin I'm still missing an answer to #3529 (comment)... I don't understand what the two return lines are gaining us: won't we still just get an error?

bmclaughlin · 2015-08-04T19:40:15Z

@matthewd I agree, we will still just get an error. Would you like me to remove those lines in a quick follow-up PR?

matthewd · 2015-08-04T20:24:48Z

Yeah, let's kill those two, and switch to find instead of find_by_id. 3 lines is better than 5 😄

(and while I'd generally favour find over find_by_id anyway, in any situation where we're not explicitly planning to handle a nil return... I think using find should convert both those error conditions to 404s, which is a nice bonus)

dclarizio added bug ui labels Jul 20, 2015

bmclaughlin force-pushed the optimize-bottlenecks-timeline-error branch from 3af69c2 to bbfbe7d Compare July 24, 2015 20:24

chessbyte reviewed Jul 24, 2015
View reviewed changes

bmclaughlin force-pushed the optimize-bottlenecks-timeline-error branch from bbfbe7d to 66780cc Compare July 27, 2015 13:17

bmclaughlin force-pushed the optimize-bottlenecks-timeline-error branch from 66780cc to df9b85d Compare July 27, 2015 20:55

bmclaughlin force-pushed the optimize-bottlenecks-timeline-error branch 2 times, most recently from 8ab1097 to ee9c710 Compare August 3, 2015 20:35

chessbyte reviewed Aug 3, 2015
View reviewed changes

bmclaughlin force-pushed the optimize-bottlenecks-timeline-error branch from ee9c710 to 2941dc7 Compare August 4, 2015 17:27

chessbyte added a commit that referenced this pull request Aug 4, 2015

Merge pull request #3529 from bmclaughlin/optimize-bottlenecks-timeli…

4310a54

…ne-error Fix error when viewing timelines in Optimize-Bottlenecks in IE

chessbyte merged commit 4310a54 into ManageIQ:master Aug 4, 2015

chessbyte added this to the Sprint 28 Ending August 24, 2015 milestone Aug 4, 2015

bmclaughlin deleted the optimize-bottlenecks-timeline-error branch August 4, 2015 18:11

bmclaughlin mentioned this pull request Aug 5, 2015

Refactor timeline_data method #3725

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error when viewing timelines in Optimize-Bottlenecks in IE #3529

Fix error when viewing timelines in Optimize-Bottlenecks in IE #3529

bmclaughlin commented Jul 20, 2015

epwinchell commented Jul 21, 2015

dclarizio commented Jul 22, 2015

bmclaughlin commented Jul 22, 2015

tenderlove commented Jul 22, 2015

bmclaughlin commented Jul 22, 2015

tenderlove commented Jul 22, 2015

chessbyte Jul 24, 2015

chessbyte Jul 27, 2015

bmclaughlin Jul 27, 2015

matthewd Jul 27, 2015

dclarizio commented Jul 27, 2015

bmclaughlin commented Jul 27, 2015

matthewd commented Jul 27, 2015

matthewd commented Jul 27, 2015

dclarizio commented Jul 27, 2015

bmclaughlin commented Jul 28, 2015

chessbyte commented Jul 28, 2015

bmclaughlin commented Jul 28, 2015

chessbyte commented Jul 30, 2015

bmclaughlin commented Jul 30, 2015

bmclaughlin commented Aug 3, 2015

chessbyte commented Aug 3, 2015

chessbyte commented Aug 3, 2015

chessbyte Aug 3, 2015

matthewd Aug 3, 2015

matthewd commented Aug 3, 2015

matthewd commented Aug 3, 2015

bmclaughlin commented Aug 4, 2015

miq-bot commented Aug 4, 2015

chessbyte commented Aug 4, 2015

matthewd commented Aug 4, 2015

bmclaughlin commented Aug 4, 2015

matthewd commented Aug 4, 2015

Fix error when viewing timelines in Optimize-Bottlenecks in IE #3529

Fix error when viewing timelines in Optimize-Bottlenecks in IE #3529

Conversation

bmclaughlin commented Jul 20, 2015

epwinchell commented Jul 21, 2015

dclarizio commented Jul 22, 2015

bmclaughlin commented Jul 22, 2015

tenderlove commented Jul 22, 2015

bmclaughlin commented Jul 22, 2015

tenderlove commented Jul 22, 2015

chessbyte Jul 24, 2015

Choose a reason for hiding this comment

chessbyte Jul 27, 2015

Choose a reason for hiding this comment

bmclaughlin Jul 27, 2015

Choose a reason for hiding this comment

matthewd Jul 27, 2015

Choose a reason for hiding this comment

dclarizio commented Jul 27, 2015

bmclaughlin commented Jul 27, 2015

matthewd commented Jul 27, 2015

matthewd commented Jul 27, 2015

dclarizio commented Jul 27, 2015

bmclaughlin commented Jul 28, 2015

chessbyte commented Jul 28, 2015

bmclaughlin commented Jul 28, 2015

chessbyte commented Jul 30, 2015

bmclaughlin commented Jul 30, 2015

bmclaughlin commented Aug 3, 2015

chessbyte commented Aug 3, 2015

chessbyte commented Aug 3, 2015

chessbyte Aug 3, 2015

Choose a reason for hiding this comment

matthewd Aug 3, 2015

Choose a reason for hiding this comment

matthewd commented Aug 3, 2015

matthewd commented Aug 3, 2015

bmclaughlin commented Aug 4, 2015

miq-bot commented Aug 4, 2015

chessbyte commented Aug 4, 2015

matthewd commented Aug 4, 2015

bmclaughlin commented Aug 4, 2015

matthewd commented Aug 4, 2015