Refactor - Replace .success calls with .then/.catch #13

AparnaKarve · 2016-12-22T21:51:47Z

With Angular 1.6+, the $http .success and $http .error would not be supported anymore.

The PR currently addresses the .success calls in one controller.
The other controllers will follow suit.

Tackling the following controllers in this PR:

auth_key_pair_cloud_controller.js
ems_common_form_controller.js
host_form_controller.js
schedule_form_controller.js

AparnaKarve · 2016-12-22T21:52:22Z

@miq-bot add_label wip,refactor

miq-bot · 2016-12-22T21:53:24Z

@AparnaKarve Cannot apply the following label because they are not recognized: refactor

AparnaKarve · 2017-01-04T22:50:24Z

@himdel Let me know what you think of these changes. If this looks good, I would like to replicate this approach across all angular controllers.

I've also tried to address some of the CodeClimate issues, specifically the ones that were easily addressable and pertaining to the changes made in this PR.

himdel · 2017-01-05T12:39:20Z

app/assets/javascripts/controllers/ems_common/ems_common_form_controller.js

    }
    $scope.actionUrl = $scope.newRecord ? $scope.createUrl : $scope.updateUrl;
    $scope.currentTab = "default";
+
+    function getEmsFormIdDataComplete(response, status, headers, config) {


(response, status, headers, config)

The docs say that the handler takes only one parameter, which is an object containing data, status, headers, config and statusText - not 4. Or am I reading it wrong?

You're right. I'll definitely go with what the Angular docs say.

My source was this, which could be outdated.

himdel · 2017-01-05T12:52:49Z

app/assets/javascripts/controllers/ems_common/ems_common_form_controller.js

+
+    function getEmsFormDataFailed(e) {
+      miqService.sparkleOff();
+      console.log(e.message);


I'd do (e && e.message) || e here (or something like that): there's no guarantee the error will have a message field. (There is if it was a failure in http, but if the then function throws an exception, that one doesn't have to have a message.)

We would always have e, although e.message could be questionable.

So I'll change it to --

if (e.message) { console.log(e.message); }

himdel · 2017-01-05T12:53:52Z

@AparnaKarve the approach looks solid, except for that args oversight I have nothing to add 👍 :)

AparnaKarve · 2017-01-05T23:59:22Z

@AparnaKarve the approach looks solid, except for that args oversight I have nothing to add 👍 :)

Thanks! I've implemented your suggestions.
I will now work on the other controllers.

AparnaKarve · 2017-01-09T23:41:56Z

@himdel I'm thinking of dividing this task into smaller chunks. Here's why --

$ git grep -l "\.success(" *.js
app/assets/javascripts/controllers/auth_key_pair_cloud/auth_key_pair_cloud_controller.js
app/assets/javascripts/controllers/cloud_network/cloud_network_form_controller.js
app/assets/javascripts/controllers/cloud_subnet/cloud_subnet_form_controller.js
app/assets/javascripts/controllers/cloud_tenant/cloud_tenant_form_controller.js
app/assets/javascripts/controllers/cloud_topology/cloud_topology_controller.js
app/assets/javascripts/controllers/cloud_volume/cloud_volume_form_controller.js
app/assets/javascripts/controllers/configuration/time_profile_form_controller.js
app/assets/javascripts/controllers/container_dashboard/container_dashboard_controller.js
app/assets/javascripts/controllers/container_live_dashboard/container_live_dashboard_controller.js
app/assets/javascripts/controllers/container_topology/container_topology_controller.js
app/assets/javascripts/controllers/ems_common/ems_common_form_controller.js
app/assets/javascripts/controllers/ems_infra_dashboard/ems_infra_dashboard_controller.js
app/assets/javascripts/controllers/floating_ip/floating_ip_form_controller.js
app/assets/javascripts/controllers/host/host_form_controller.js
app/assets/javascripts/controllers/host_aggregate/host_aggregate_form_controller.js
app/assets/javascripts/controllers/infra_topology/infra_topology_controller.js
app/assets/javascripts/controllers/middleware_topology/middleware_topology_controller.js
app/assets/javascripts/controllers/network_router/network_router_form_controller.js
app/assets/javascripts/controllers/network_topology/network_topology_controller.js
app/assets/javascripts/controllers/ops/diagnostics_database_form_controller.js
app/assets/javascripts/controllers/ops/log_collection_form_controller.js
app/assets/javascripts/controllers/ops/pglogical_replication_form_controller.js
app/assets/javascripts/controllers/ops/tenant_form_controller.js
app/assets/javascripts/controllers/ops/tenant_quota_form_controller.js
app/assets/javascripts/controllers/orchestration_template/orchestration_template_copy_controller.js
app/assets/javascripts/controllers/ownership/ownership_form_controller.js
app/assets/javascripts/controllers/provider_foreman/provider_foreman_form_controller.js
app/assets/javascripts/controllers/reconfigure/reconfigure_form_controller.js
app/assets/javascripts/controllers/retirement/retirement_form_controller.js
app/assets/javascripts/controllers/schedule/schedule_form_controller.js
app/assets/javascripts/controllers/security_group/security_group_form_controller.js
app/assets/javascripts/controllers/service/service_form_controller.js
app/assets/javascripts/controllers/vm_cloud/vm_cloud_associate_floating_ip_form_controller.js
app/assets/javascripts/controllers/vm_cloud/vm_cloud_disassociate_floating_ip_form_controller.js
app/assets/javascripts/controllers/vm_cloud/vm_cloud_evacuate_form_controller.js
app/assets/javascripts/controllers/vm_cloud/vm_cloud_live_migrate_form_controller.js
app/assets/javascripts/miq_tree.js

$ git grep -l "\.success(" *.js|wc -l
      37

So close to 37 controllers need to be addressed.
It will be one ginormous PR if I tackle all of them in one go (and I for one know how you feel about such PRs 😄 )
Hence let's do this in baby steps (about 4 to 5 controllers in each PR) for an easier review. That said, I've covered 4 controllers in this PR.

himdel · 2017-01-10T12:12:22Z

@AparnaKarve agreed there, split any way you like ;)

Just one thing I'm noticing now.. can you move that error handler function to miqService please? (Or somewhere so that we don't define the same thing in all of the controllers.)

AparnaKarve · 2017-01-10T17:17:37Z

can you move that error handler function to miqService please? (Or somewhere so that we don't define the same thing in all of the controllers.)

Yes, I thought about that too...moving the error handler to miqService, but somehow it didn't seem like an angular service material. (it seems like a very controller thing)
Wish there was a way to define a base controller where we could have all this common controller stuff.
Anyway, if I don't come up with a better approach, I guess I will include it in miqService for now.
Thanks.

himdel · 2017-01-10T17:21:58Z

@AparnaKarve yeah, kinda agreed but better there than in every controller :).

Or, if you're really unconfortable with putting this in a service, an alternative would be a global function, defined in miq_angular_application - just not sure that's better :).

As for a common controller .. yeah, agreed completely, in fact I was planning to do a BaseFormController (or something) ..but I guess it really makes sense only after we move to es6 and can use es6 classes. (We can do it before, but .. possibly confusing syntax..)

AparnaKarve · 2017-01-10T17:26:06Z

in fact I was planning to do a BaseFormController (or something) ..but I guess it really makes sense only after we move to es6 and can use es6 classes.

Interesting!
Yeah, let's wait until we move to es6.

to Move function declaration to function body root.

miq-bot · 2017-01-10T18:27:52Z

Checked commits AparnaKarve/manageiq-ui-classic@eadd9a1~...25eaa96 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
0 files checked, 0 offenses detected
Everything looks good. 🍰

AparnaKarve · 2017-01-10T18:45:46Z

@himdel I guess now would be a good time to un-WIP this.
Check out my last commit -- 25eaa96

@miq-bot remove_label wip

AparnaKarve · 2017-01-11T15:17:21Z

@himdel Ideally I would like this PR merged before I move on to the next set of controllers.
Can you please merge this if all the concerns have been addressed?
Thanks.

himdel · 2017-01-13T14:01:06Z

Sorry @AparnaKarve it took so long.. merged :)

LGTM 👍, tested in the UI.. (ems_common at least)

AparnaKarve · 2017-01-13T15:07:11Z

@himdel Thanks for merging and thank you 🙏 especially for testing in the UI!

Xcs 20

miq-bot added the wip label Dec 22, 2016

AparnaKarve force-pushed the angular16_prep_refactor branch 2 times, most recently from 4f660e0 to b17c135 Compare January 4, 2017 01:38

AparnaKarve changed the title ~~[WIP] Refactor - Preparing for a future angular version~~ [WIP] Refactor - Replace .success calls with .then/.catch Jan 4, 2017

himdel reviewed Jan 5, 2017

View reviewed changes

AparnaKarve force-pushed the angular16_prep_refactor branch from f151f02 to 1da247f Compare January 5, 2017 23:57

This was referenced Jan 9, 2017

Angular: Remove all references to $http success/error in favor of $http.then() ManageIQ/manageiq#13127

Closed

Angular: Remove all references to $http success/error in favor of $http.then() #102

Closed

AparnaKarve force-pushed the angular16_prep_refactor branch 4 times, most recently from d204071 to 9264705 Compare January 9, 2017 23:26

AparnaKarve added 7 commits January 10, 2017 10:10

Refactor - Replace .success() with .then()

eadd9a1

Address failure scenario during get

abb195e

Address CC warning --

9e9b94e

to Move function declaration to function body root.

Address CC warning -- Expected '!==' and instead saw '!='

7cd7aba

PR feedback

41eef62

Address CC error - Unexpected console statement.

e196601

Refactor - Replace .success() with .then() and handle failures

4e2f85a

AparnaKarve added 3 commits January 10, 2017 10:10

Resolve couple of CC errors

138f6df

Refactor - Replace .success() with .then() and handle failures

1192551

Refactor - Replace .success() with .then() and handle failures

f473a0e

AparnaKarve force-pushed the angular16_prep_refactor branch from 9264705 to 8b27691 Compare January 10, 2017 18:11

Address PR feedback -- move error handler to miqService

25eaa96

AparnaKarve force-pushed the angular16_prep_refactor branch from 8b27691 to 25eaa96 Compare January 10, 2017 18:24

AparnaKarve changed the title ~~[WIP] Refactor - Replace .success calls with .then/.catch~~ Refactor - Replace .success calls with .then/.catch Jan 10, 2017

miq-bot removed the wip label Jan 10, 2017

himdel self-assigned this Jan 12, 2017

himdel added euwe/no technical debt labels Jan 12, 2017

himdel merged commit efabac9 into ManageIQ:master Jan 13, 2017

himdel added this to the Sprint 52 Ending Jan 16, 2017 milestone Jan 13, 2017

AparnaKarve mentioned this pull request Jan 17, 2017

Refactor - Replace .success calls with .then/.catch (Round 2) #179

Merged

This was referenced Jan 24, 2017

Refactor - Replace .success calls with .then/.catch (Round 3) #232

Merged

Refactor - Replace .success calls with .then/.catch (Round 4) #281

Merged

This was referenced Feb 1, 2017

Move the Ansible Tower UI to the Automation tab #170

Merged

Refactor - Replace .success calls with .then/.catch (Round 5, final round) #284

Merged

rodneyhbrown7 added a commit to rodneyhbrown7/manageiq-ui-classic that referenced this pull request Feb 28, 2017

Merge pull request ManageIQ#13 from lenovo/XCS-20

aa63bd2

Xcs 20

AparnaKarve deleted the angular16_prep_refactor branch July 26, 2017 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor - Replace .success calls with .then/.catch #13

Refactor - Replace .success calls with .then/.catch #13

AparnaKarve commented Dec 22, 2016 •

edited

Loading

AparnaKarve commented Dec 22, 2016

miq-bot commented Dec 22, 2016

AparnaKarve commented Jan 4, 2017

himdel Jan 5, 2017

AparnaKarve Jan 5, 2017

himdel Jan 5, 2017

AparnaKarve Jan 5, 2017

himdel commented Jan 5, 2017

AparnaKarve commented Jan 5, 2017

AparnaKarve commented Jan 9, 2017

himdel commented Jan 10, 2017

AparnaKarve commented Jan 10, 2017

himdel commented Jan 10, 2017

AparnaKarve commented Jan 10, 2017

miq-bot commented Jan 10, 2017

AparnaKarve commented Jan 10, 2017

AparnaKarve commented Jan 11, 2017

himdel commented Jan 13, 2017

AparnaKarve commented Jan 13, 2017

Refactor - Replace .success calls with .then/.catch #13

Refactor - Replace .success calls with .then/.catch #13

Conversation

AparnaKarve commented Dec 22, 2016 • edited Loading

AparnaKarve commented Dec 22, 2016

miq-bot commented Dec 22, 2016

AparnaKarve commented Jan 4, 2017

himdel Jan 5, 2017

Choose a reason for hiding this comment

AparnaKarve Jan 5, 2017

Choose a reason for hiding this comment

himdel Jan 5, 2017

Choose a reason for hiding this comment

AparnaKarve Jan 5, 2017

Choose a reason for hiding this comment

himdel commented Jan 5, 2017

AparnaKarve commented Jan 5, 2017

AparnaKarve commented Jan 9, 2017

himdel commented Jan 10, 2017

AparnaKarve commented Jan 10, 2017

himdel commented Jan 10, 2017

AparnaKarve commented Jan 10, 2017

miq-bot commented Jan 10, 2017

AparnaKarve commented Jan 10, 2017

AparnaKarve commented Jan 11, 2017

himdel commented Jan 13, 2017

AparnaKarve commented Jan 13, 2017

AparnaKarve commented Dec 22, 2016 •

edited

Loading