Fixed links to point to correct provider controller based upon type. #4760

h-kataria · 2018-10-11T19:33:04Z

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

martinpovolny · 2018-10-12T07:04:20Z

@h-kataria , I am concerned that similar functionality already exists in app/controllers/restful_redirect_controller.rb.

Furthermore the code in the RestfulRedirectController shows, that there are situations where the information used in this PR does not suffice for the decision.

Therefor I suggest investigating if a fix for the BZ can be implemented using the existing code rather that adding similar logic on the Javascript side.

skateman · 2018-10-12T07:13:03Z

app/assets/javascripts/services/alerts_center_service.js

+    var action = '/';
+    var descriptors = providerType.split('::');
+
+    switch (descriptors.pop()) {


I don't like this client-side specification because it has to be in sync with the controllers all the time. What do you think about using the restful_redirect_controller instead? This way the switch could be completely removed and the redirect decision logic could live in that controller.

skateman · 2018-10-12T07:18:04Z

Oh, I see @martinpovolny had the exact same idea 🤣

skateman

I think if you try to use singular_route_key, you could probably make the code just a few lines and move it under the ˙index˙ route.

skateman · 2018-10-15T15:12:41Z

app/controllers/restful_redirect_controller.rb

+    action = '/'
+    descriptors = params['provider_type'].split('::')
+
+    case descriptors.last


What about

provider_type.constantize.model_name.singular_route_key

?

skateman · 2018-10-15T15:13:14Z

config/routes.rb

@@ -2830,6 +2830,7 @@
    :restful_redirect => {
      :get => %w(
        index
+        provider_url


I don't really like the new route thing, any reason why this isn't under index?

@skateman i kept it separate from index method because that method expects different params and is being used to redirect to a page based upon model that's passed in. provider_url sets a link that is being used by list of Alerts, not sure if it's a good idea to mix the both methods.

And what about passing them the same params? Like ˙ExtManagementSystemasmodeland theid`. And use the method I wrote above to determine the URL.

h-kataria · 2018-10-16T20:17:34Z

@skateman please review/test. Thanks for the help!

skateman · 2018-10-17T07:18:03Z

app/assets/javascripts/services/alerts_center_service.js

@@ -488,6 +488,15 @@ function alertsCenterService(API, $q, $timeout, $document, $uibModal, $http) {
    }
  }

+  var providerLink = function(providerType, providerId, newAlert) {


You no longer need the providerType argument

Suggested change

var providerLink = function(providerType, providerId, newAlert) {

var providerLink = function(providerId, newAlert) {

skateman · 2018-10-17T07:18:52Z

app/assets/javascripts/services/alerts_center_service.js

@@ -532,7 +540,7 @@ function alertsCenterService(API, $q, $timeout, $document, $uibModal, $http) {
    }

    updateAlertStatus(newAlert);
-
+    providerLink(objectClassifiedType, alertData.ems_id, newAlert);


Suggested change

providerLink(objectClassifiedType, alertData.ems_id, newAlert);

providerLink(alertData.ems_id, newAlert);

skateman · 2018-10-17T07:19:28Z

app/assets/javascripts/services/alerts_center_service.js

@@ -488,6 +488,15 @@ function alertsCenterService(API, $q, $timeout, $document, $uibModal, $http) {
    }
  }

+  var providerLink = function(providerType, providerId, newAlert) {
+    $http.get('/restful_redirect/index?model=ExtManagementSystem&id=' + providerId)
+      .then(updateNewAlert);


Suggested change

.then(updateNewAlert);

.then(function(response) {

newAlert.objectLink = response.data.data;

});

skateman · 2018-10-17T20:17:57Z

app/assets/javascripts/services/alerts_center_service.js

@@ -532,7 +538,7 @@ function alertsCenterService(API, $q, $timeout, $document, $uibModal, $http) {
    }

    updateAlertStatus(newAlert);
-
+    providerLink(alertData.ems_id, newAlert);


What about having the link straight to '/restful_redirect/index?model=ExtManagementSystem&id=' + providerId and just use redirect_to so it is consistent?

skateman · 2018-10-18T14:17:40Z

I know that Real Programmers aren't afraid to use GOTO's, but I am not a fan of that style and this looks almost like that. So what about creating a private handle_error (I'm sure you can come up with a better name) method that does the error handling, call it when something goes wrong and also in the else branch of the case?

case xy
when a
  handle_error unless record
  blah
when b
  ...
else
  handle_error
end

skateman · 2018-10-18T20:02:01Z

app/controllers/restful_redirect_controller.rb

+      unless record
+        handle_error
+        return
+      end


Suggested change

end

return handle_error unless

Or maybe

Suggested change

end

record ? redirect_to() : handle_error

skateman · 2018-10-18T20:02:30Z

app/controllers/restful_redirect_controller.rb

+        handle_error
+        return
+      end
+      redirect_to(polymorphic_path(record))
    when 'ServiceTemplateTransformationPlanRequest'
      req = ServiceTemplateTransformationPlanRequest.select(:source_id).find(params[:id])
      redirect_to(:controller => 'migration', :action => 'index', :anchor => "plan/#{req.source_id}")


I think you might have to add the error handling here as well...

Up to you if in a separate commit or here

skateman · 2018-10-18T20:03:31Z

app/controllers/restful_redirect_controller.rb

    end
  end
+
+  def handle_error


I seriously thought you can come up with a better name than I, but it's okay 🤣

Added a method in restful_redirect_controller that can be called from JS service to determine provider link. Moved fallback code to redirect to dashboard show with flash message out of switch. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1637609

skateman · 2018-10-19T03:55:00Z

app/controllers/restful_redirect_controller.rb

    end
  end
+


Suggested change

private

Added controller spec tests Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1637609

miq-bot · 2018-10-19T18:21:32Z

Checked commits h-kataria/manageiq-ui-classic@efa4d7a~...c186097 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

skateman

…s_list Fixed links to point to correct provider controller based upon type. (cherry picked from commit 5a0d67d) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1637609

simaishi · 2018-10-22T17:44:43Z

Hammer backport details:

$ git log -1
commit 74bee4611f3f701727228a28df158d69e5a69cc4
Author: Milan Zázrivec <mzazrivec@redhat.com>
Date:   Mon Oct 22 17:14:47 2018 +0200

    Merge pull request #4760 from h-kataria/fixed_provider_links_on_alerts_list
    
    Fixed links to point to correct provider controller based upon type.
    
    (cherry picked from commit 5a0d67dfaa4c729ceb5defa30147767b18e6a9ea)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1637609

h-kataria added bug blocker monitor labels Oct 11, 2018

h-kataria assigned mzazrivec Oct 11, 2018

h-kataria requested a review from skateman October 11, 2018 19:36

h-kataria added the hammer/yes label Oct 11, 2018

h-kataria force-pushed the fixed_provider_links_on_alerts_list branch 2 times, most recently from d68b450 to d18ec8e Compare October 11, 2018 19:53

skateman suggested changes Oct 12, 2018

View reviewed changes

h-kataria force-pushed the fixed_provider_links_on_alerts_list branch 3 times, most recently from eddc392 to 932e0b1 Compare October 15, 2018 13:47

skateman suggested changes Oct 15, 2018

View reviewed changes

h-kataria force-pushed the fixed_provider_links_on_alerts_list branch from 932e0b1 to e83b2c1 Compare October 16, 2018 20:16

skateman suggested changes Oct 17, 2018

View reviewed changes

h-kataria force-pushed the fixed_provider_links_on_alerts_list branch 2 times, most recently from 020b8b9 to 0bae925 Compare October 17, 2018 13:56

skateman reviewed Oct 17, 2018

View reviewed changes

h-kataria force-pushed the fixed_provider_links_on_alerts_list branch 2 times, most recently from c1fea76 to e571eb4 Compare October 18, 2018 13:51

h-kataria force-pushed the fixed_provider_links_on_alerts_list branch from e571eb4 to 6e5928b Compare October 18, 2018 16:11

skateman suggested changes Oct 18, 2018

View reviewed changes

h-kataria force-pushed the fixed_provider_links_on_alerts_list branch from 6e5928b to efa4d7a Compare October 18, 2018 20:48

skateman reviewed Oct 19, 2018

View reviewed changes

app/controllers/restful_redirect_controller.rb

end

end

Copy link

Member

skateman Oct 19, 2018

Choose a reason for hiding this comment

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

Suggested change

private

redirect and show flash when VM record is missing.

c186097

Added controller spec tests Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1637609

h-kataria force-pushed the fixed_provider_links_on_alerts_list branch from 293f8f0 to c186097 Compare October 19, 2018 18:17

skateman approved these changes Oct 22, 2018

View reviewed changes

mzazrivec added this to the Sprint 97 Ending Oct 22, 2018 milestone Oct 22, 2018

mzazrivec merged commit 5a0d67d into ManageIQ:master Oct 22, 2018

simaishi added hammer/backported and removed hammer/yes labels Oct 22, 2018

h-kataria deleted the fixed_provider_links_on_alerts_list branch October 24, 2018 22:04

h-kataria added the gaprindashvili/yes label Nov 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed links to point to correct provider controller based upon type. #4760

Fixed links to point to correct provider controller based upon type. #4760

h-kataria commented Oct 11, 2018

martinpovolny commented Oct 12, 2018 •

edited

Loading

skateman Oct 12, 2018

skateman commented Oct 12, 2018

skateman left a comment

skateman Oct 15, 2018

skateman Oct 15, 2018

h-kataria Oct 15, 2018

skateman Oct 15, 2018

h-kataria commented Oct 16, 2018

skateman Oct 17, 2018

skateman Oct 17, 2018

skateman Oct 17, 2018

skateman Oct 17, 2018

skateman commented Oct 18, 2018 •

edited

Loading

skateman Oct 18, 2018

skateman Oct 18, 2018

skateman Oct 18, 2018

skateman Oct 18, 2018

skateman Oct 18, 2018

skateman Oct 19, 2018

miq-bot commented Oct 19, 2018

skateman left a comment

simaishi commented Oct 22, 2018

	var providerLink = function(providerType, providerId, newAlert) {
	var providerLink = function(providerId, newAlert) {

	providerLink(objectClassifiedType, alertData.ems_id, newAlert);
	providerLink(alertData.ems_id, newAlert);

-      .then(updateNewAlert);
+      .then(function(response) {
+        newAlert.objectLink = response.data.data;
+      });

Fixed links to point to correct provider controller based upon type. #4760

Fixed links to point to correct provider controller based upon type. #4760

Conversation

h-kataria commented Oct 11, 2018

martinpovolny commented Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

skateman commented Oct 12, 2018

skateman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-kataria commented Oct 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skateman commented Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Oct 19, 2018

skateman left a comment

Choose a reason for hiding this comment

simaishi commented Oct 22, 2018

martinpovolny commented Oct 12, 2018 •

edited

Loading

skateman commented Oct 18, 2018 •

edited

Loading