[migrateVolume API method] Filter disk offerings based on target storage pool selected #2612
Conversation
rafaelweingartner
force-pushed
the
filterDiskOfferingMigrateVolumeApi
branch
from
6b660ee to
3e7bf4f
Compare
…l selected After using the feature introduced by apache#2486 in production, we felt the need for an improvement in the UI. It is interesting to filter the displayed disk offerings according to the type of storage selected (local/shared) to migrate the volume to.
rafaelweingartner
force-pushed
the
filterDiskOfferingMigrateVolumeApi
branch
from
3e7bf4f to
094a9e3
Compare
| return array; | ||
| } | ||
|
|
||
| cloudStack.listDiskOfferings = function(options){ |
There was a problem hiding this comment.
Hi @rafaelweingartner, This is a great improvement. I came across several places where the same AJAX call(Code) is repeated multiple places. We should put more of this to reduce redundancy. Thanks.
| var diskOfferings = undefined; | ||
| $.ajax({ | ||
| url: createURL(listDiskOfferingsUrl), | ||
| data: mergedOptions.data, |
There was a problem hiding this comment.
For some calls, we don't pass 'data' in the parameter list. For e.g. [var diskOfferings = cloudStack.listDiskOfferings({listAll: true});]
I think, it would be better if we check the existance of the parameter before assigning.
There was a problem hiding this comment.
I am not checking the existence of mergedOptions.data before ahe ssignment because the result would be the same. I mean, if mergedOptions.data is undefined, than the object data was not provided. Therefore, the object data in the ajax function will have the value undefined. If I add an IF condition and decide to assign data based on the existence of mergedOptions.data; then, when mergedOptions.data == undefined (I would not assign it), the data object of ajax would also be undefined. The result is the same, and thus there is no reason to add extra code.
Was my explanation clear?
borisstoyanov
left a comment
borisstoyanov
left a comment
There was a problem hiding this comment.
LGTM, no need to run integration tests since the changes are in ui/
3 participants
Description
After using the feature introduced by #2486 in production, we felt the need for an improvement in the UI. It is interesting to filter the displayed disk offerings according to the type of storage selected (local/shared) to migrate the volume to.
Types of changes
How Has This Been Tested?
Locally
Checklist:
Testing