Adding view full log in alerts button #2862

nimrodshn · 2017-11-29T13:03:24Z

Currently we truncate (after 200 characters) the log error / warning that was received when connecting to a provider - In this patch I purpose a way of dealing with the full log error by adding a button to the alert when the alert is too long (above 200 characters).
Summary of scenarios tested:
Short alert:

Long alert:

Very Long alert

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1506718
cc: @cben , @serenamarie125 , @himdel
Please share thoughts on this 😅 - this is just a proposition 😇

nimrodshn · 2017-11-29T13:04:46Z

@miq-bot add_label bug, gaprindashvili/yes, wip

himdel · 2017-11-29T14:16:50Z

This needs to be different.

You are changing the styling of every single flash message generated on JS side, in a way that is inconsistent with flash messages coming from ruby.

You're also randomly hardcoding a 200 character limit - this is wrong because this will affect any existing messages long enough. And it is also wrong because the reasonable size will depend on the user's resolution, and chosen language (this is why Twitter still limits Chinese to 140 chars).

Also, the CSS looks like it can't work on mobile, limiting height is seldom a good idea.

So.. proposing:

keep the both implementations consistent, and update ruby-side add_flash and corresponding templates to keep generating the same layout
do a proper css class for the style changes, and ... if you can prevent using overflow:hidden and setting height, even better. (Test this on a phone.)
don't determine the Details logic by message length, use an explicit option (feel free to add a 3rd options={} arg to add_flash if needed), apply only to messages coming from authentication mixin

(or we could add a separate message body concept to flash messages - so that we keep the flash the same, but can optionally add a longer body, which will trigger a button to display it, and needs no extra logic)

(EDIT: sorry, closed by mistake)

serenamarie125 · 2017-11-29T15:04:09Z

@nimrodshn Although we don't have PatternFly support for this, I'd suggest doing something like OpenShift has implement ... where there is a See All link which allows for expansion and a vertical scrolllbar

Tagging @jeff-phillips-18 who may be able to point you to some code which can does this

nimrodshn · 2017-11-29T15:57:10Z

@himdel is @serenamarie125 's approach OK with you? (before I continue with implementation I just wanna make sure we're all on the same page).
Edit: Although without proper Patternfly support I'm not really sure how to implement this..

himdel · 2017-11-29T16:02:33Z

Depends on the details - @serenamarie125 's screenshot is a toast notification, not a flash message.

So... if you do manage to make the error go via our asynchronous notifications so that it ends up in the notification area... even better, I do believe there is some support for expandable messages already.

But it means going through a completely different code path than a flash message, and the message pretty much needs to come from the server, we have no way of creating these toasts in JS now.

(Or rather, no clean way - if this is something that doesn't have to be visible in the notification area after the user reloads the page, then maybe we do. If it needs to be there after reload, it needs to come from the server.)

(I don't really object to the flash message approach either, just that the style needs to be consistent, and work for all the existing ones too.)

serenamarie125 · 2017-11-29T16:18:50Z

what's a flash message @himdel - inline notification?

himdel · 2017-11-29T16:23:51Z

@serenamarie125 Yes, it's the inline one - the one you can see in the gif in the description (except the button is new there).

serenamarie125 · 2017-11-30T03:41:19Z

@nimrodshn I'd suggest using similar behavior, regardless if it's a flash/inline or toast notification.

@himdel I think we need to discuss the flash/inline versus toast approach in general and see how we can transition away from flash/inline in most cases. I wouldn't expect that to be part of this PR.

nimrodshn · 2017-11-30T08:44:53Z

So @jrafanie defers for a different solution in the backend:

I'd be ok with a few different solutions:

extending the truncate length to fit a reasonable error message to fix your bug (while still fitting into the status_details column)

Or move the truncate to the log line and where we save the status_details and all callers of authentication_check

Thoughts @himdel @serenamarie125 ?

himdel · 2017-11-30T13:51:00Z

Re-reading the BZ, I'm wondering if we're looking at the problem the right way.. does the user really need to get that long HTML message in its entirety? Do we assume the user will be able to understand it?

Because, if this is not about getting randomly long descriptive messages from backend, but about sometimes getting unexpected HTML back, maybe we could fix this by actually dealing with the HTML on the backend - parsing it, or replacing it with a simpler message.

(I don't know the details of what openshift actually returns when, but seems to me that if we just stripped all the html tags and left just the text nodes, if would probably fit in the limit while still letting the user know more.)

(Incidentally, that's what we do in JS when the rails backend returns an unexpected response - try to parse out the exception from the message.)

((But if parsing it server-side doesn't work .. I don't know, my complaints were more about the style changes to the flash messages than anything else - if we drop the length limit without changing the format, I don't think there's any issue, as long as the backend is happy. :) ))

cben · 2017-11-30T14:10:55Z

There are a couple common mistakes (such as "no such app" page from openshift router). But it's impossible to say what we'll get generally, we can't even know whether it's HTML! (for working openshift it's actually JSON) The main scenario I had in mind when I opened that BZ is *talking to something unexpected* — because user entered wrong hostname/port, or entered correct ones but reached something else. (In a customer case that prompted this, a firewall blocked the connection.) The user might or might not understand it... In the firewall case, first 200 chars was unhelpful but once I asked them to run `curl` and they saw "blocked" and the firewall vendor name they went "aha!" and fixed it quickly. The way I see it, when you're trying to connect a provider and it Just Doesn't Work (tm), it's a miserable experience. At that point, you got exactly zero value of the product, you can't make any progress until you get help, and you don't even have enough data for troubleshooting, like text to google — or to send to people who might help. (well in case of RedHat support you'd be asked to upload logs, but in any case triage is faster if you have error message) I like the idea of backend sending independent "message" and "details" texts, because in future we might want to add more diagnostic data there, e.g. details on the TLS certificate, maybe some advice / pointers to docs etc...

nimrodshn · 2017-12-06T16:12:51Z

@himdel PTAL

nimrodshn · 2017-12-07T13:54:41Z

@himdel code climate is really annoying 😭

himdel · 2017-12-07T14:01:38Z

@nimrodshn Feel free to ignore :)

We do need to fix CC at some point, yes, the 3 issues are nonsense.

The code looks good I think 👍

(have not tested in UI yet)

One issue I see - we can't use an id for alert_expand_button .. you can have more than one flash message on the screen, so it needs to be a class.

Other than that, can you please also add the corresponding change to the Ruby add_flash?
(Or I can do a separate PR if you have trouble there, just want to ensure both are in sync.)

nimrodshn · 2017-12-07T14:15:22Z

@himdel Sure will do that!

cben · 2017-12-07T18:20:25Z

app/controllers/mixins/ems_common_angular.rb

@@ -98,7 +98,7 @@ def render_validation_result(result, details)
        level = :error
      end

-      render_flash_json(msg, level)
+      render_flash_json(msg, level, :long_alert => true)


Nowsg is unlimited, perhaps ruby should still truncate to some large limit here, to avoid sending megabyte responses to UI.

@cben so truncate at 500 chars here?

Is this addressed by https://github.com/ManageIQ/manageiq/pull/16550/files#diff-f9cace1fe4fdcec66f962b23f2110fd6R337 now? (I think it is, making sure :).)

cben · 2017-12-07T22:30:32Z

more like 20000 if you ask me :) it's will be loooong but only in rare cases, and it's hidden by default, and in those cases long is better than lack of info... it's just to avoid completely blowing up... (well a gigabytes response could still exhaust ruby memory before we even get to UI. but that's out of scope, let's hope people don't connect MIQ directly to youtube :-)

miq-bot · 2017-12-08T10:14:04Z

This pull request is not mergeable. Please rebase and repush.

nimrodshn · 2017-12-18T15:04:14Z

@himdel reverted to the old code. just need to add the scrollbar.

nimrodshn · 2017-12-18T18:57:21Z

@himdel Added the scroll bar plus fixed the dangling link situation.

yaacov · 2017-12-18T20:05:02Z

app/assets/javascripts/miq_flash.js

+    params.clicked = true;
+    params.alert_elem.removeClass('text-overflow-pf');
+    params.alert_elem.addClass('text-vertical-overflow-pf');
+    params.link.find('a').text("View Less");


translate ?

nice catch!

yaacov · 2017-12-18T20:10:00Z

app/assets/javascripts/miq_flash.js

+
+  // remove dangling 'Show More' link when the alert msg is short.
+  if( options.long_alert && !checkElipsis(alertDiv) ) {
+    detailsLink.hide();


nimrodshn · 2017-12-18T20:14:23Z

cc @skateman @serenamarie125
GIF(with scrollbar):

himdel · 2017-12-20T12:04:50Z

app/assets/javascripts/miq_flash.js

+}
+
+function expandAlert(params) {
+  var viewMoreTxt = _("View More");


In js, you need __, not _ for i18n ;).

(_ seems to work because it's lodash("string") -> ["string"].toString() -> "string" :))

serenamarie125 · 2017-12-20T12:06:48Z

@miq-bot add_label ux/review

serenamarie125 · 2017-12-20T12:07:16Z

@miq-bot add_label ux/approved

serenamarie125 · 2017-12-20T12:07:36Z

@miq-bot remove_label ux/review

himdel · 2017-12-20T12:35:49Z

Tested in UI, looks like resizing the window now works both for expanded and for non-expanded alerts 👍

Two more things I notice:

if you have more than one alert, the code touches both whenever you click any of them
checkEllipsis works in chrome but not in firefox (or at least not firefox 51) - View More is displayed there every time (=> Adding view full log in alerts button #2862 (comment))

(+ the __ thing #2862 (comment))

EDIT: and fixed 👍

himdel · 2017-12-20T12:38:38Z

app/assets/javascripts/miq_flash.js

+  if (options.long_alert) {
+    var detailsLinkTxt = _("View More");
+    var detailsLink = $('<div class="alert_expand_link"><strong><a href="#">' + detailsLinkTxt + '</a></strong></div>');
+    var params = {clicked: false, alert_elem: alertDiv, link: detailsLink};


You may want to save the state in that particular element's data, or somewhere per-element, not global.

himdel · 2017-12-20T12:45:11Z

app/assets/javascripts/miq_flash.js

+  var found = false;
+  var $c = element
+    .clone()
+    .css({display: 'inline', width: 'auto', visibility: 'hidden'})


display: 'inline-block' will make this work in firefox :)

minor refactoring to pr second iteration minor refactoring making code climate happy minor refactoring and tests minor refactoring adding dealing with small alerts minor refactoring extracting txt and trimming whitespaces fixed the specs moved to link refactoing class selector adding view more minor refactoring hardcoded msg length solution reverting to elipsis solution fix dangling link adding stylesheet for scrollbar fix whitespaces and tests fix dangling whitespace minor refactoring, adding compatability with firefox

miq-bot · 2017-12-20T13:39:17Z

Checked commit nimrodshn@7e07090 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

simaishi · 2018-01-03T18:22:29Z

Gaprindashvili backport details:

$ git log -1
commit 56283a047f9e3571cce1538abadd723a44b94f97
Author: Martin Hradil <himdel@seznam.cz>
Date:   Wed Dec 20 16:41:15 2017 +0100

    Merge pull request #2862 from nimrodshn/fix_log_bug
    
    Adding view full log in alerts button
    (cherry picked from commit dd0eadbacc34eab74dd1524e7492d8825bdaedbb)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1530645

Adding view full log in alerts button (cherry picked from commit dd0eadb) https://bugzilla.redhat.com/show_bug.cgi?id=1530645

miq-bot added the wip label Nov 29, 2017

miq-bot added bug gaprindashvili/yes labels Nov 29, 2017

nimrodshn force-pushed the fix_log_bug branch from 02e6450 to a05086c Compare November 29, 2017 13:16

himdel added the pending core label Nov 29, 2017

himdel closed this Nov 29, 2017

himdel reopened this Nov 29, 2017

cben mentioned this pull request Nov 30, 2017

Allow full error log ManageIQ/manageiq#16550

Merged

nimrodshn force-pushed the fix_log_bug branch from a05086c to b887fcc Compare December 6, 2017 15:14

nimrodshn requested a review from martinpovolny as a code owner December 6, 2017 15:14

nimrodshn force-pushed the fix_log_bug branch 2 times, most recently from c9c1295 to 8d9182b Compare December 6, 2017 15:47

cben reviewed Dec 7, 2017

View reviewed changes

nimrodshn force-pushed the fix_log_bug branch from 1e86d1f to 949434d Compare December 18, 2017 14:59

nimrodshn force-pushed the fix_log_bug branch 2 times, most recently from 8f73083 to 638800f Compare December 18, 2017 18:52

yaacov reviewed Dec 18, 2017

View reviewed changes

nimrodshn force-pushed the fix_log_bug branch 2 times, most recently from 49f3919 to 02b6ca3 Compare December 19, 2017 11:07

himdel reviewed Dec 20, 2017

View reviewed changes

miq-bot added the ux/review label Dec 20, 2017

miq-bot added ux/approved and removed ux/review labels Dec 20, 2017

himdel self-assigned this Dec 20, 2017

himdel reviewed Dec 20, 2017

View reviewed changes

nimrodshn force-pushed the fix_log_bug branch from 02b6ca3 to 7e07090 Compare December 20, 2017 13:31

himdel added this to the Sprint 76 Ending Jan 1, 2018 milestone Dec 20, 2017

himdel merged commit dd0eadb into ManageIQ:master Dec 20, 2017

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jan 3, 2018

simaishi pushed a commit that referenced this pull request Jan 3, 2018

Merge pull request #2862 from nimrodshn/fix_log_bug

56283a0

Adding view full log in alerts button (cherry picked from commit dd0eadb) https://bugzilla.redhat.com/show_bug.cgi?id=1530645

Adding view full log in alerts button #2862

Adding view full log in alerts button #2862

Conversation

nimrodshn commented Nov 29, 2017 • edited Loading

nimrodshn commented Nov 29, 2017

himdel commented Nov 29, 2017 • edited Loading

serenamarie125 commented Nov 29, 2017

nimrodshn commented Nov 29, 2017 • edited Loading

himdel commented Nov 29, 2017

serenamarie125 commented Nov 29, 2017 • edited Loading

himdel commented Nov 29, 2017

serenamarie125 commented Nov 30, 2017

nimrodshn commented Nov 30, 2017 • edited Loading

himdel commented Nov 30, 2017 • edited Loading

cben commented Nov 30, 2017 via email • edited Loading

nimrodshn commented Dec 6, 2017

nimrodshn commented Dec 7, 2017

himdel commented Dec 7, 2017 • edited Loading

nimrodshn commented Dec 7, 2017

cben Dec 7, 2017

Choose a reason for hiding this comment

nimrodshn Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

himdel Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

cben commented Dec 7, 2017 via email

miq-bot commented Dec 8, 2017

nimrodshn commented Dec 18, 2017 • edited Loading

nimrodshn commented Dec 18, 2017 • edited Loading

yaacov Dec 18, 2017

Choose a reason for hiding this comment

nimrodshn Dec 18, 2017

Choose a reason for hiding this comment

yaacov Dec 18, 2017

Choose a reason for hiding this comment

nimrodshn commented Dec 18, 2017

himdel Dec 20, 2017

Choose a reason for hiding this comment

serenamarie125 commented Dec 20, 2017

serenamarie125 commented Dec 20, 2017

serenamarie125 commented Dec 20, 2017

himdel commented Dec 20, 2017 • edited Loading

himdel Dec 20, 2017

Choose a reason for hiding this comment

himdel Dec 20, 2017

Choose a reason for hiding this comment

miq-bot commented Dec 20, 2017

simaishi commented Jan 3, 2018

nimrodshn commented Nov 29, 2017 •

edited

Loading

himdel commented Nov 29, 2017 •

edited

Loading

nimrodshn commented Nov 29, 2017 •

edited

Loading

serenamarie125 commented Nov 29, 2017 •

edited

Loading

nimrodshn commented Nov 30, 2017 •

edited

Loading

himdel commented Nov 30, 2017 •

edited

Loading

cben commented Nov 30, 2017 via email •

edited

Loading

himdel commented Dec 7, 2017 •

edited

Loading

nimrodshn Dec 7, 2017 •

edited

Loading

himdel Dec 20, 2017 •

edited

Loading

nimrodshn commented Dec 18, 2017 •

edited

Loading

nimrodshn commented Dec 18, 2017 •

edited

Loading

himdel commented Dec 20, 2017 •

edited

Loading