Lazy load StorageManagers upon volume creation #4549

miha-plesko · 2018-08-28T08:18:44Z

When creating a new volume user has to pick what StorageManager is volume supposed to be created with, hence we perform API request to fetch those. Problem is we share angular controller with other volume operations (attach, detach) that don't need this list of all managers and fetching those anyway results in permission denied error due to fine-grained permission setting.

For example if we give user only permission to attach/detach volume, but not to view StorageManagers:

then we must not perform the API call as it results in 403 with red popup:

With this commit we refactor controller so that it now only performs the API call "fetch storage managers" when we're creating a new volume, and not just always by default. Attaching/detaching volume therefore doesn't trigger the problematic API call anymore:

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

@miq-bot add_label bug,fine/yes,gaprindashvili/yes
@miq-bot assign @h-kataria

BZ is reported for Fine, therefore we have to reach that far.

/cc @AparnaKarve

JPrause · 2018-08-28T15:37:41Z

@miq-bot add_label blocker

JPrause · 2018-08-30T15:51:28Z

@miha-plesko can you add a reviewer. We have an upcoming final dev complete on Sep 10 and this fix is needed.

miha-plesko · 2018-08-30T18:45:06Z

@JPrause I dont have enough permissions..

JPrause · 2018-08-30T18:47:01Z

Thanks @miha-plesko it's enough if you ping them to review.

miha-plesko · 2018-08-30T19:45:16Z

@h-kataria @AparnaKarve @himdel @mzazrivec @skateman not sure who, but can someone please take a look at this one? It's kinda urgent. Thanks!

AparnaKarve · 2018-08-31T04:14:30Z

@miha-plesko I verified that the reported issue has been fixed by this PR.

I noticed something else while testing -

With this PR -
In the Edit Cloud Volume screen, the Storage Manager and Availability Zone are not getting populated - This is for an admin user.

With master branch -
Availability Zone is getting populated

Both Storage Manager and Availability Zone are read-only fields, so this is not a big issue.

Anyway, this was something that I happened to notice, that is partially related to the subject matter of this PR, and exists in master as well.

AparnaKarve

Verified that the reported issue has been fixed.

miha-plesko · 2018-08-31T07:23:06Z

Nice find, thanks @AparnaKarve. I'm refactoring a little more to get the issue fixed. What I'm going to do is introduce a new variable cloudVolumeFormOperation so that we'll be able to fully address different operations (new, edit, attach, detach, ...) explicitly.

himdel · 2018-08-31T11:40:18Z

So... sounds like the bug is really that the controller is being used by multiple unrelated forms, isn't it?

This should become 7 different controllers + a service to share common code.

But.. perhaps more importantly, doesn't this break creating new cloud volumes? Since you can no longer pick a storage manager when cloudVolumeFormId === 'new'

EDIT: verified this is indeed the case, it's impossible to select a storage manager for new cloud volumes now. (And additionally, even though the field is required, it let's you save.)

miha-plesko · 2018-08-31T12:04:38Z

@himdel it was OK for adding new volumes because condition was in fact negated cloudVolumeFormId !== 'new'. However, I picked different approach now: there is a switch-case statement which picks what API requests are needed depending what view are we handling. I like your idea of multiple controllers with a common service, let me think about it a bit more.

miha-plesko · 2018-08-31T12:19:34Z

app/views/cloud_volume/attach.html.haml

@@ -56,5 +56,6 @@

 :javascript
  ManageIQ.angular.app.value('cloudVolumeFormId', '#{@volume.id}');
-  ManageIQ.angular.app.value('storageManagerId', #{@volume.ext_management_system.id});
+  ManageIQ.angular.app.value('storageManagerId', '#{@volume.ext_management_system.id.to_s}');


@AparnaKarve this is the reason why StorageManager wasn't getting selected upon volume edit (on master branch). It's because we're passing integer id from rails, but drop-down options somehow have strings. Well hidden bug, spent quite some time figuring it out :)

So the easiest solution is to convert it to string immediately here. Problem is if we would want it be integer, then we could hit overflow as MIQ ID's are huge numbers.

Agreed, strings are necessary for our long ids in js :). (The to_s is not necessary, but.. not a problem either.)

himdel · 2018-08-31T14:29:33Z

app/assets/javascripts/controllers/cloud_volume/cloud_volume_form_controller.js

+    switch (cloudVolumeFormOperation) {
+      case 'EDIT':
+        // Fetch relevant StorageManager, just enough to populate the disabled drop-down.
+        apiPromises.push(API.get(`/api/providers/${storageManagerId}?attributes=id,name`)


Uhh, can't do this here, sorry ;)

Anything under app/assets needs to be ES5-only.

(so, I guess apiPromises.push(API.get('/api/providers/' + storageManagerId + '?attributes=id,name') or sprintf)

Thanks for noticing! I've changed as you suggest on both places, no other change, and pushed. Now I'm gone for the weekend :)

himdel · 2018-08-31T14:33:37Z

Verified I can now see the right manager in the new cloud volume screen. 👍

When creating a new volume user has to pick what StorageManager is volume supposed to be created with, hence we perform API request to fetch those. Problem is we share angular controller with other volume operations (attach, detach) that don't need this list of all managers and fetching those anyway results in permission denied error due to fine-grained permission setting. For example if we give user only permission to attach/detach volume, but not to view StorageManagers, then we must not perform the API call as it results in 403 with red popup. With this commit we refactor controller so that it now only performs the API calls that are really needed. BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1571224 Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

miq-bot · 2018-08-31T14:42:27Z

Checked commit miha-plesko@771e97f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

himdel · 2018-09-03T09:40:16Z

Going through the other templates, looks like only attach depends on extra data, covered by the default switch branch, LGTM :)

himdel · 2018-09-03T09:48:23Z

Not seeing any breakage in the UI, merging 👍

miha-plesko · 2018-09-03T09:53:58Z

Yeah @himdel , I was thinking about defaulting to zero API requests but then didn't dare to do it because I wasn't sure... Having this one merged I think it will be simple enough to do it should BZ arise :)

himdel · 2018-09-03T09:57:22Z

@miha-plesko Yeah, agreed, for backporting, we better keep the existing behaviour.

But if you're planning to do the split-the-controller PR, we may as well remove the extra calls there.

simaishi · 2018-09-04T17:45:07Z

@miha-plesko There are some conflicts (in app/assets/javascripts/controllers/cloud_volume/cloud_volume_form_controller.js backporting this to Fine branch. Can you please create a separate PR for Fine?

miha-plesko · 2018-09-05T08:06:49Z

@simaishi there you go #4600

simaishi · 2018-09-06T16:10:30Z

Backported to Fine via #4600

Lazy load StorageManagers upon volume creation (cherry picked from commit 47c98ac) https://bugzilla.redhat.com/show_bug.cgi?id=1625249

simaishi · 2018-09-17T17:17:14Z

Gaprindashvili backport details:

$ git log -1
commit 7dbad13ee87c8ad2e60d89f685bb6474c85ae328
Author: Martin Hradil <himdel@seznam.cz>
Date:   Mon Sep 3 09:48:25 2018 +0000

    Merge pull request #4549 from miha-plesko/lazy-fetch-storage-managers
    
    Lazy load StorageManagers upon volume creation
    (cherry picked from commit 47c98ac4cfeddc59ef2010bdec07b796e1d9bbbf)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1625249

miq-bot added bug fine/yes labels Aug 28, 2018

miq-bot assigned h-kataria Aug 28, 2018

miq-bot added the blocker label Aug 28, 2018

AparnaKarve approved these changes Aug 31, 2018

View reviewed changes

mzazrivec added the storage label Aug 31, 2018

miha-plesko force-pushed the lazy-fetch-storage-managers branch from 4ae7295 to 087b387 Compare August 31, 2018 12:01

miha-plesko commented Aug 31, 2018

View reviewed changes

himdel reviewed Aug 31, 2018

View reviewed changes

miha-plesko force-pushed the lazy-fetch-storage-managers branch from 087b387 to 771e97f Compare August 31, 2018 14:40

himdel merged commit 47c98ac into ManageIQ:master Sep 3, 2018

himdel added this to the Sprint 94 Ending Sept 10, 2018 milestone Sep 3, 2018

himdel mentioned this pull request Sep 3, 2018

[WIP] Cloud volume attach controller to component #3653

Closed

simaishi added the fine/conflict label Sep 4, 2018

miha-plesko mentioned this pull request Sep 5, 2018

[FINE] Lazy load StorageManagers upon volume creation #4600

Merged

simaishi added fine/backported and removed fine/conflict labels Sep 6, 2018

simaishi pushed a commit that referenced this pull request Sep 17, 2018

Merge pull request #4549 from miha-plesko/lazy-fetch-storage-managers

7dbad13

Lazy load StorageManagers upon volume creation (cherry picked from commit 47c98ac) https://bugzilla.redhat.com/show_bug.cgi?id=1625249

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Sep 17, 2018

miha-plesko mentioned this pull request Nov 19, 2018

Cloud Volume creation fixes #4908

Merged

miha-plesko deleted the lazy-fetch-storage-managers branch January 7, 2019 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy load StorageManagers upon volume creation #4549

Lazy load StorageManagers upon volume creation #4549

miha-plesko commented Aug 28, 2018

JPrause commented Aug 28, 2018

JPrause commented Aug 30, 2018

miha-plesko commented Aug 30, 2018

JPrause commented Aug 30, 2018

miha-plesko commented Aug 30, 2018

AparnaKarve commented Aug 31, 2018 •

edited

AparnaKarve left a comment

miha-plesko commented Aug 31, 2018

himdel commented Aug 31, 2018 •

edited

miha-plesko commented Aug 31, 2018

miha-plesko Aug 31, 2018

himdel Aug 31, 2018 •

edited

himdel Aug 31, 2018 •

edited

miha-plesko Aug 31, 2018

himdel commented Aug 31, 2018

miq-bot commented Aug 31, 2018

himdel commented Sep 3, 2018 •

edited

himdel commented Sep 3, 2018

miha-plesko commented Sep 3, 2018

himdel commented Sep 3, 2018

simaishi commented Sep 4, 2018

miha-plesko commented Sep 5, 2018

simaishi commented Sep 6, 2018

simaishi commented Sep 17, 2018

Lazy load StorageManagers upon volume creation #4549

Lazy load StorageManagers upon volume creation #4549

Conversation

miha-plesko commented Aug 28, 2018

JPrause commented Aug 28, 2018

JPrause commented Aug 30, 2018

miha-plesko commented Aug 30, 2018

JPrause commented Aug 30, 2018

miha-plesko commented Aug 30, 2018

AparnaKarve commented Aug 31, 2018 • edited

AparnaKarve left a comment

Choose a reason for hiding this comment

miha-plesko commented Aug 31, 2018

himdel commented Aug 31, 2018 • edited

miha-plesko commented Aug 31, 2018

miha-plesko Aug 31, 2018

Choose a reason for hiding this comment

himdel Aug 31, 2018 • edited

Choose a reason for hiding this comment

himdel Aug 31, 2018 • edited

Choose a reason for hiding this comment

miha-plesko Aug 31, 2018

Choose a reason for hiding this comment

himdel commented Aug 31, 2018

miq-bot commented Aug 31, 2018

himdel commented Sep 3, 2018 • edited

himdel commented Sep 3, 2018

miha-plesko commented Sep 3, 2018

himdel commented Sep 3, 2018

simaishi commented Sep 4, 2018

miha-plesko commented Sep 5, 2018

simaishi commented Sep 6, 2018

simaishi commented Sep 17, 2018

AparnaKarve commented Aug 31, 2018 •

edited

himdel commented Aug 31, 2018 •

edited

himdel Aug 31, 2018 •

edited

himdel Aug 31, 2018 •

edited

himdel commented Sep 3, 2018 •

edited