Add Picture support in Generic Object Definition editor #2339

AparnaKarve · 2017-10-10T17:28:00Z

Requires - ManageIQ/manageiq-api#116

https://www.pivotaltracker.com/n/projects/1613907/stories/151742686

Automation > Automate > Generic Objects

Create Generic Object Screen

Generic Object Definition Summary Screen

Edit Generic Object Screen

to enable Save button

himdel · 2017-10-10T18:03:15Z

app/views/static/generic_object/custom-image-component.html.haml

+                        "ng-model"  => "vm.changeImage"}
+
+:javascript
+  $(":file").filestyle({placeholder: _("No file chosen"), icon: false});


__ ? This is js..

But it's a JS inside a haml. So do we still need the __?

Well, it can be either placeholder: __("No file chosen"), or placeholder: "#{_("No file chosen")}",

it is haml, but not ruby unless interpolating

Addressed in fb22a12

himdel · 2017-10-10T18:08:57Z

app/assets/javascripts/components/generic_object/custom-image-component.js

+      vm.picture.extension = 'jpeg';
+    } else {
+      vm.angularForm.generic_object_definition_image_file_status.$setValidity("incompatibleFileType", false);
+      vm.imageUploadStatus = __("Incompatible image type");


Won't this show the message while the file is still loading? (or is the time really close to 0 even for huge images?)

At this stage the file upload (or loading) hasn't even begun - it's just being vetted for the right extension.

Yes.. but the file is being processed by the browser, in the background.

So, between you setting this and that onload event firing, the message will be visible.

Will test.. :)

Oh.. sorry, I read the code wrong, no such problem here :). (I thought this happened always, not only on bad type.)

It does feel a bit weird that you have to punch the Upload button after chosing the file, but that's not a blocker, more of a detail for UX :).

We are using the exact same UX in Catalog Explorer as well where we set custom images for Catalog Items. And that UX comes from Bootstrap Filestyle - http://markusslima.github.io/bootstrap-filestyle/

I did give a shot to "angularize" Bootstrap Filestyle, as in, keep the Upload button disabled until a file is available to be uploaded, but once you run the JS script $(":file").filestyle, it pretty much disregards all angular directives etc

We are using the exact same UX in Catalog Explorer ..

Ah, ok, then definitely not a problem here :)

(Though just to be clear, my objection was not to what I see in bootstrap-filestyle, but to the extra button they have to press after they chose the file.)

himdel · 2017-10-10T18:12:39Z

app/assets/javascripts/components/generic_object/custom-image-component.js

+
+      $timeout(function() {
+        vm.angularForm.generic_object_definition_image_file_status.$setValidity("incompatibleFileType", true);
+        vm.imageUploadStatus = __("Image upload complete");


Would a different message make more sense here given this does not actually upload the image yet?

I mean, if I see "Image upload complete" I will think the image is already on the server. (And will wonder why submitting the form takes so long.)

(I guess there's no support for uploading the image in the background yet, right?)

"Image upload complete" is just a helpful acknowledgement to the user that the chosen file is now ready to be saved.
LMK if you have other wording suggestions for that message.

I guess there's no support for uploading the image in the background yet, right?

No background support - it all happens in the main thread.
In my testing so far, I haven't come across a situation where the file loading is like a really lengthy, time-consuming operation -- it has been almost instantaneous in my tests.

Take a 40 MB png file, and connect to manageiq running on the network, not on localhost.

And suddenly it takes about 40 seconds to upload (and 2s to even show the spinner :)).

But I guess we don't have to optimize for such huge image users quite yet, but I wouldn't mislead them either .. what about "Image will be uploaded when you submit the form"?

Sounds good. Will change that text.

On second thoughts, saying "...submit the form" sounds a bit techy, let's try and keep it very generic.
How about "Image is ready to be uploaded"?

Addressed in fb22a12

Better, thanks! :)

himdel · 2017-10-10T18:14:02Z

app/assets/javascripts/components/generic_object/generic-object-definition-component.js

@@ -76,6 +81,8 @@ function genericObjectDefinitionFormController(API, miqService, $q) {

    assignAllObjectsToKeyValueArrays(true);

+    vm.pictureReset = ! vm.pictureReset;


clicking reset twice will remove the picture?

I guess this should be always vm.pictureReset = false;? (Unless I'm misunderstanding pictureReset)

vm.pictureReset is just a toggle to trigger $onChanges in the custom-image-component every time Reset is clicked.

$onChanges in the custom-image-component is in-charge of resetting the UI in the component.

Aaah, can you make this explicit please?

Because now it looks like you're just switching the var for no reason, and not actually using it.

(I mean, if you look for pictureReset in the code, you will just see that you set it to false, and that you negate it here. But never actually read that. .. So it's a bit hard to notice that you're using it for the side effect caused by changing it.)

I would make $onChanges do that only when changes.pictureReset, not on any change, unless you're relying on it happening for other changes.

Addressed in fb22a12

himdel · 2017-10-10T18:20:52Z

Overall LGTM, will test in the morning...

(I vaguelly remember IE having some difference in the filetypes, but not sure if relevant here, so I want to check.)

With this change there is no need for a virtual column for `picture_url_path` as once thought would be necessary. It is important to set `picture.image_href` to undefined in the model to avoid API failure during a Save operation

gmcculloug · 2017-10-10T20:12:45Z

@lfu Please test.

himdel · 2017-10-11T11:57:35Z

app/assets/javascripts/components/generic_object/custom-image-component.js

+    } else if (imageFile.type === 'image/jpg') {
+      vm.picture.extension = 'jpg';
+    } else if (imageFile.type === 'image/jpeg') {
+      vm.picture.extension = 'jpeg';


{"error":{"kind":"bad_request","message":"Failed to update generic object definition - Validation failed: Extension must be a png, jpg, or svg","klass":"Api::BadRequestError"}}

This has to be jpg as well, not jpeg.

And it seems like you may want to add a condition for SVG.

Addressed in fb22a12

himdel · 2017-10-11T13:13:40Z

Tested in UI .. except for being unable to upload jpg images, everything seemed to work :).

Have not tested in IE as I'm unable to get it working.. so fingers crossed :). (That said, MDN claims IE11 supports File fully, so, what I remember may have been for previous versions.)

miq-bot · 2017-10-11T15:33:58Z

Checked commits AparnaKarve/manageiq-ui-classic@5486e6b~...fb22a12 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 1 offense detected

app/views/generic_object_definition/show.html.haml

⚠️ - Line 14 - Prefer to_s over string interpolation.

lfu · 2017-10-11T16:10:56Z

Testing looks good 👍

AparnaKarve · 2017-10-11T16:23:41Z

@himdel @lfu Thank you for testing.

@himdel I have addressed the feedback in fb22a12

himdel · 2017-10-12T12:15:33Z

Oh, @AparnaKarve I found one more bug, sorry :)..

edit an existing generic object which already has an image
toggle the Update current image switch to yes
add a different image
toggle the Update current image switch back to no
save

Expected:
toggling update current image back to no should mean the image does not get replaced

Actual:
it uploads the new image

(Otherwise LGTM)

AparnaKarve · 2017-10-12T13:50:09Z

@himdel Yes, I found that one too, and I'm thinking the fix should be to just hide the Update current image checkbox once the new image is actually uploaded.
(Reset is the way to get back things in the original status)

How about I do this in a follow-up PR, and close the chapter on this one since the core functionality is in?

dclarizio · 2017-10-12T16:21:41Z

@AparnaKarve follow up sounds good, merging

AparnaKarve added 7 commits October 10, 2017 10:13

Add picture support for new GOD records

5486e6b

Display the associated custom image in the Summary screen

d867a95

Display/Update custom image for an existing GOD record

84e7037

Make dirty-checking possible when a picture is updated

b38b520

to enable Save button

Handle Picture Reset

bdfec08

Add support for deleting an associated picture with a GOD record

86040ac

Address codeclimate issues

f403559

dclarizio requested review from gmcculloug and himdel October 10, 2017 17:52

dclarizio added enhancement fine/no generic objects labels Oct 10, 2017

dclarizio self-assigned this Oct 10, 2017

himdel reviewed Oct 10, 2017

View reviewed changes

himdel added the pending core label Oct 10, 2017

himdel removed the pending core label Oct 11, 2017

himdel reviewed Oct 11, 2017

View reviewed changes

Address PR feedback

fb22a12

AparnaKarve force-pushed the god_picture branch from f421ef0 to fb22a12 Compare October 11, 2017 15:32

dclarizio merged commit e9762a2 into ManageIQ:master Oct 12, 2017

dclarizio added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Picture support in Generic Object Definition editor #2339

Add Picture support in Generic Object Definition editor #2339

AparnaKarve commented Oct 10, 2017 •

edited by himdel

Loading

himdel Oct 10, 2017

AparnaKarve Oct 10, 2017

himdel Oct 11, 2017 •

edited

Loading

AparnaKarve Oct 11, 2017

himdel Oct 10, 2017

AparnaKarve Oct 10, 2017

himdel Oct 11, 2017

himdel Oct 11, 2017

AparnaKarve Oct 11, 2017

himdel Oct 12, 2017

himdel Oct 10, 2017

AparnaKarve Oct 10, 2017 •

edited

Loading

himdel Oct 11, 2017 •

edited

Loading

AparnaKarve Oct 11, 2017

AparnaKarve Oct 11, 2017

AparnaKarve Oct 11, 2017

himdel Oct 12, 2017

himdel Oct 10, 2017 •

edited

Loading

AparnaKarve Oct 10, 2017

himdel Oct 11, 2017

AparnaKarve Oct 11, 2017

himdel commented Oct 10, 2017

gmcculloug commented Oct 10, 2017

himdel Oct 11, 2017 •

edited

Loading

AparnaKarve Oct 11, 2017

himdel commented Oct 11, 2017 •

edited

Loading

miq-bot commented Oct 11, 2017

lfu commented Oct 11, 2017

AparnaKarve commented Oct 11, 2017

himdel commented Oct 12, 2017 •

edited

Loading

AparnaKarve commented Oct 12, 2017

dclarizio commented Oct 12, 2017

		@@ -76,6 +81,8 @@ function genericObjectDefinitionFormController(API, miqService, $q) {

		assignAllObjectsToKeyValueArrays(true);

		vm.pictureReset = ! vm.pictureReset;

Add Picture support in Generic Object Definition editor #2339

Add Picture support in Generic Object Definition editor #2339

Conversation

AparnaKarve commented Oct 10, 2017 • edited by himdel Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Oct 11, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

himdel Oct 11, 2017 • 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

himdel Oct 10, 2017 • 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

himdel commented Oct 10, 2017

gmcculloug commented Oct 10, 2017

himdel Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Oct 11, 2017 • edited Loading

miq-bot commented Oct 11, 2017

lfu commented Oct 11, 2017

AparnaKarve commented Oct 11, 2017

himdel commented Oct 12, 2017 • edited Loading

AparnaKarve commented Oct 12, 2017

dclarizio commented Oct 12, 2017

AparnaKarve commented Oct 10, 2017 •

edited by himdel

Loading

himdel Oct 11, 2017 •

edited

Loading

AparnaKarve Oct 10, 2017 •

edited

Loading

himdel Oct 11, 2017 •

edited

Loading

himdel Oct 10, 2017 •

edited

Loading

himdel Oct 11, 2017 •

edited

Loading

himdel commented Oct 11, 2017 •

edited

Loading

himdel commented Oct 12, 2017 •

edited

Loading