Story #3112

[Workbench] "Report bug on this page" and "Show version detail" buttons on each workbench page (footer or top nav or [?]), and more prominently on error pages.

Added by Tom Clegg over 5 years ago. Updated about 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
Workbench
Target version:
Start date:
08/12/2014
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
2.0

Description

Finding it
  • Modal, always in DOM, menu items under "help" dropdown cause it to open
  • Menu items: "Show version / debugging info", "Report a problem"
    • First existing help links, then separator, then this new stuff
Content
  • Render as a bootstrap form with static form inputs (plus hidden ones, so they actually get submitted)
  • Show workbench source_version (get this from application.yml)
  • Show workbench arvados_v1_base config
  • Show apiserver source_version (get this from discovery doc, which gets it from apiserver's application.yml)
  • Show apiserver discovery document generatedAt timestamp
  • Current page URI (with overflow=ellipsis) -- populate when showing modal, using window.location
  • If current page is an error page:
    • Current error message
    • Current api_response[:error_token]
  • Your user_uuid
  • Support email address (from workbench config)
  • If "report a problem" is how we got here, also reveal:
    • "Found a problem? Tell us what happened:" + textarea + "Report bug" button
    • "Cancel" button
  • else reveal a "Close" button
Error page
  • (Make sure the modal still works if current_user is nil)
  • Add a "More info / report problem..." button to the error page that brings up the "report" modal
Submit
  • AJAX
    • Disable "Report bug" button and change its text to "Sending..." during AJAX call
    • When AJAX succeeds, replace form with an info panel "Thanks for reporting!" and reveal the "Close" (close dialog) button
    • If AJAX fails, say something that acknowledges the irony like "Well, this is pretty embarrassing. I couldn't even submit your bug report! We really want this to work, though -- please do try again."
    • (I think there's something out there like "data-disable-with" that can do all the button text changing for us.)
  • Send email message from workbench (no API call)
  • Also log the submitted info to stderr
Add configs
  • apiserver source_version (and include this in the discovery doc as sourceVersion)
  • workbench source_version

Default in application.default.yml can be something like this, but we also want to know whether there are local modifications (i.e., does `git status -s` report anything?). If there are local modifications, abbreviate the commit sha1 to 20 characters and append "-modified"

common:
  source_version: <%= `git show --format=%h` %>

itfeelssmallinhere.png (29.5 KB) itfeelssmallinhere.png Tom Clegg, 08/18/2014 07:43 PM
3112-button-margins.png (14.2 KB) 3112-button-margins.png No margin, extra padding on report alert Brett Smith, 08/25/2014 04:04 PM

Subtasks

Task #3268: version detailResolvedRadhika Chippada

Task #3431: [Workbench] We need a link / page to contact support in our topnav.ResolvedRadhika Chippada

Task #3574: Add source_version to api server configResolvedRadhika Chippada

Task #3596: Add source_version and report email to workbench configResolvedRadhika Chippada

Task #3597: Add Report Issue Mailer to workbenchResolvedRadhika Chippada

Task #3598: Add report issue popupResolvedRadhika Chippada

Task #3599: Update workbench filter chaining to allow report issue popup and action propagation for no user, inactive user cases.ResolvedRadhika Chippada

Task #3600: Add integration tests for the new report issue linksResolvedRadhika Chippada

Task #3601: Add "report issue" button to 404 error pageResolvedRadhika Chippada

Task #3602: Perform the "report issue" action using ajaxResolvedRadhika Chippada

Task #3606: Add "report issue" button to fiddlesticks error pageResolvedRadhika Chippada

Task #3608: Review branch: 3112-report-bugResolvedBrett Smith

Associated revisions

Revision 183bdcd6
Added by Radhika Chippada about 5 years ago

closes #3112
Merge branch '3112-report-bug'

Revision 02cbe072 (diff)
Added by Tom Clegg about 5 years ago

Ensure source_version is a string, even when it consists entirely of decimal digits. refs #3112

Revision ff742a82 (diff)
Added by Tom Clegg about 5 years ago

Fix label for API endpoint. Re-order fields. refs #3112

History

#1 Updated by Tom Clegg over 5 years ago

  • Subject changed from "Report bug on this page" button on each workbench page to "Report bug on this page" and "Show version detail" buttons in footer of each workbench page, and more prominently on error pages.

#2 Updated by Tom Clegg over 5 years ago

  • Target version set to 2014-08-06 Sprint

#3 Updated by Tom Clegg over 5 years ago

  • Subject changed from "Report bug on this page" and "Show version detail" buttons in footer of each workbench page, and more prominently on error pages. to "Report bug on this page" and "Show version detail" buttons on each workbench page (footer or top nav or [?]), and more prominently on error pages.

#4 Updated by Radhika Chippada over 5 years ago

  • Subject changed from "Report bug on this page" and "Show version detail" buttons on each workbench page (footer or top nav or [?]), and more prominently on error pages. to [Workbench] "Report bug on this page" and "Show version detail" buttons on each workbench page (footer or top nav or [?]), and more prominently on error pages.
  • Category set to Workbench

#5 Updated by Tom Clegg over 5 years ago

  • Target version changed from 2014-08-06 Sprint to Arvados Future Sprints

#6 Updated by Ward Vandewege about 5 years ago

  • Target version changed from Arvados Future Sprints to 2014-08-27 Sprint

#7 Updated by Tom Clegg about 5 years ago

  • Assigned To set to Radhika Chippada

#8 Updated by Tom Clegg about 5 years ago

  • Description updated (diff)

#9 Updated by Radhika Chippada about 5 years ago

  • Status changed from New to In Progress

#10 Updated by Radhika Chippada about 5 years ago

Notes for review:

- Modal is loaded when button is clicked rather than pre-loading. Since this page changes based on show version or report issue type, it was simpler to stick to this format.

- Discussed the form fields displayed with Tom and we consolidated information such as user's uuid, current location etc into an additional information text area. This helped reduce the popup size and easy to access the Submit button.

- Slightly modified the ajax success event handling. I did not like hiding the form, as this makes the popup size smaller. Also, this way the user can still see all the info after successfully sending the report. Instead, I added the status messages to the footer area.

#11 Updated by Tom Clegg about 5 years ago

Notes at a9170bf

Haven't combed the code yet, but I have some feedback on the presentation:

  • Workbench version is reported as a9170bfa9170bf. I suspect source_version should say local_modified here in apps/workbench/app/views/application/_report_issue_popup.html.erb:
wb_version += Rails.configuration.source_version if Rails.configuration.local_modified
  • I think it's more consistent to put the action button at the far right (and if there's room, I think a clearer verb phrase like "Send problem report" would be better).
  • "Tell us what happened" textarea and "Additional info" box are too small.

The textarea with the "Additional info" is also too small to be usable. However, we do need to conserve space somehow so the modal doesn't require a huge window. Could this sort of layout allow us to fit in a popup without having tiny scrolling areas?

  • support email {blah}
  • Current page {blah} (just path, no host part -- hide x overflow with ellipsis)
  • Found a problem? etc. {textarea rows="5"?}
  • Scrolling area with the rest of the details (but still formatted as bootstrap form with static inputs, instead of key=value). "height:5em"?
    • Workbench URI {blah}
    • Workbench version {blah}
    • API version {blah}
    • API startup time {blah}
  • Report/Cancel buttons

(I'm attaching a screen capture in case it doesn't match what you see on your system.)

#12 Updated by Tom Clegg about 5 years ago

(continued)

  • After a little more thought about the vertical space problem: perhaps all we need to do is put overflow-y:scroll on the modal-body div, and put the most important things at the top (i.e., support email, current path, problem report textarea if applicable, then the rest of the stuff).
  • div.modal-footer should be a sibling of div.modal-body, not a child element. (This will fix the grey separator line above the footer that doesn't quite reach the edges of the modal.)
  • Menu item in help dropdown should have a trailing ellipsis to indicate that it asks for more information before doing anything.
  • Could we change "report_notifier_email_from/to" configurations to "issue_reporter_email_from/to", so the code/config are easier to match up with each other?
  • Typo in application.default.yml: "nitification"
  • Make the support email address clickable in the popup:
    • <a href="mailto:blah@example.com?subject=Workbench problem report&body=Problem while viewing page https://workbench.example.com/foo/bar%0D%0A%0D%0ADetails%3A">blah@example.com</a>
  • In ActionsController.report_issue:
    • Isn't $stderr.puts + logger.warn redundant? (I would have thought logger.warn would be enough)
    • Message might be easier to grep if we use the literal string "report_issue" in there somewhere ("report_issue: params=#{params.inspect}")
  • Suggest alternate wording for "Would you like to report this problem" on error pages:
    • If you suspect this is a bug, you can help us fix it by sending us a problem report:
      • Send a problem report right here. [Report a problem...]
      • If that doesn't work out (or if you prefer email), send email to {hyperlink support email address}.
  • Better to use a partial with locals for the error page snippet, rather than have three copies of the link_to report_issue_popup_path code.
  • "Report sent" div.alert-success/danger should be full width, not pull-left. (Might need "text-align:left" to counteract the footer's inclination to right-justify, though.)
  • The modal should be opened from report_issue_popup.js.erb instead of using data-toggle and data-target on the menu items / PR buttons. Currently, if you open the modal, close it, and open it again, you see an old version of the modal while the new one is still loading. (This is especially confusing if the new one fails to load, or takes a long time.) This should make reset_form() unnecessary too.
  • Most of the code in report_issue_popup.js.erb (the event handling stuff) looks like it should move to app/assets/javascripts/report_issue.js.
  • Event handlers in report_issue_popup.js.erb should be confined to the #report-issue-modal-window. (I think all you need to do is change $(document).on to $('#report-issue-modal-window form').live ...?
  • I think the jQuery magic you're looking for is
-    var sendButton = document.getElementById('report-issue-submit');
-    if (sendButton && sendButton.disabled) {
-      var text = document.getElementById('report-issue-submit').firstChild;
-      text.data = "Report sent";
+    var $sendButton = $('#report-issue-submit');
+    if ($sendButton.prop('disabled')) {
+       $sendButton.html('Report sent');

#13 Updated by Radhika Chippada about 5 years ago

Tom, addressed all your comments. Please take another look. Thanks.

#14 Updated by Brett Smith about 5 years ago

Reviewing d0c85cb

  • General style
    • There are several references like Rails.configuration.varname ? Rails.configuration.varname : "Default value". Elsewhere in our code we use the pattern (Rails.configuration.varname || "Default value"), and I think it could save you some hassle here.
      • Not sure which instance you are referring to. Left this as is.
  • Popup
    • There are a couple of places where you have &body in a URL in the HTML. That should be written as &amp;body.
      • Fixed
  • Views can access the params hash directly, so you can get rid of the extra storage of popup_params.
    • Fixed
  • The story doesn't require displaying the user's e-mail address, and I'm not sure I see the value. It's already displayed on every page for them, and presumably they know it. I suggest omitting this, but it's not a showstopper.
    • Fixed by removing it
  • "If you prefer email, send to:" reads a little strangely because "send" is missing its object. I suggest "If you prefer, send email to:"
    • Fixed
  • Similarly, the text "Found a problem? Tell us what happened" seems a little redudant, since the user clicked the "Report a problem" link to get here. Suggest instead "Describe the problem"
    • Fixed
  • "URL" should be written in uppercase.
    • Fixed
  • JavaScript
    • Closing the modal reloads the original page. Is this necessary for some reason? It seems like surprising behavior, and it could potentially lose my place if I'm viewing a specific tab or something like that.
      • Without this, the popup does not work if accessed right after sending a report from within 404 error page. Hence, left it as is.
  • The "Send problem report" button should be right of "Cancel" to better match our other dialogs.
    • Changed it. Though I don't necessarily like it :)
  • The modal declares "overflow-y: scroll;" but I think the modal needs a height set for this to work as intended. In the current version, if the modal dialog is very long, the browser will simply grow the parent page to accommodate it. The scrollbar for the modal itself is never active. I can demonstrate this in Firefox by resizing the report textarea to be very tall.
    • Changed to auto as suggested below
  • I think "overflow-y: auto;" would be preferable, to avoid showing the scrollbar when it's not necessary.
    • Fixed
  • When I submit a report, there's no margin between the buttons, and extra padding below the text. Please see the attached screenshot.
    • Fixed
  • The fail alert has a </br> out of place, which makes the HTML invalid.
    • Fixed
  • The alerts use the old HTML align attribute. Please migrate this to CSS.
    • Fixed
  • The if block inside the fail block is not indented consistently.
    • Fixed
  • The var text line seems to be left over from a previous implementation and should be removed.
    • Removed. Thanks for catching this.
  • Please remove apps/workbench/app/views/application/report_issue.js.erb.
    • Need this by the report_issue method in actions_controller. Otherwise, I get "Missing template actions/report_issue, application/report_issue" error
  • E-mail
    • Please demarcate the user's e-mail address from their name.
      • Removed full name, since email is sufficient
  • The json_map variable seems to be redundant, since you call map again in the next line.
    • Removed it
  • Configuration
    • I feel like the proposed routes don't match our existing conventions very well. How do you feel about this:
        get "report_issue" => 'actions#report_issue_popup', :as => :report_issue_popup
        post "report_issue" => 'actions#report_issue', :as => :report_issue
      
      • Changed this as suggested
  • Would it be possible for us to set local_modified: <%= '-modified' if `git status -s` %> in the common configuration for both servers? I feel like that's more likely to accomplish the goals specced for this story.
    • I think they should be separate. It is possible that we run workbench locally pointing to staging, for example. In any case, I don't think it is worth the hassle.
  • Tests
    • I am consistently getting this test failure:
        1) Failure:
      LoginsTest#test_trying_to_use_expired_token_redirects_to_login_page [/home/brett/repos/arvados/apps/workbench/test/integration/logins_test.rb:15]:
      Failed to find one login button.
      Expected: 1
        Actual: 0
      
      • Unfortunately, this test always fails when the entire suite is run for me as well (not just in this branch). You can verify that it passes when you run only test/integration/logins_test.rb. In any case, not something caused by the updates in this branch.
  • New test classes are all named ApplicationLayoutTest. They should be renamed to avoid conflicts with the existing ApplicationLayoutTest.
    • Fixed this and thanks for noticing this
  • I am concerned about the extended series of tests that all check for whether or not particular pages have or don't have long sequences of strings. There's value in checking that important interface elements appear, but many of the strings tested are more on the aesthetic side, which makes the tests very brittle. That's especially true when many of the series are duplicated throughout the code, so minor presentation changes would require multiple changes to the tests. There is less emphasis on testing application behavior, too. For example, I would really like to see a test that demonstrates that the report form is filled in with correct details about the user, server, and original error.
    • Removed some of those unnecessary checks
  • In "API error page has Report problem button", the first visit page_with_token("active") is unnecessary.
    • Removed it

#15 Updated by Brett Smith about 5 years ago

#16 Updated by Radhika Chippada about 5 years ago

Brett, addressed pretty much all of your comments. Please see your feedback (#14) for my responses inline for each entry. Thanks.

#17 Updated by Brett Smith about 5 years ago

Radhika Chippada, quoting Brett Smith, wrote:

  • General style
    • There are several references like Rails.configuration.varname ? Rails.configuration.varname : "Default value". Elsewhere in our code we use the pattern (Rails.configuration.varname || "Default value"), and I think it could save you some hassle here.
      • Not sure which instance you are referring to. Left this as is.

Like I said, it's a general style point—it's applicable anywhere this pattern comes up. I don't think it blocks the merge, just a suggestion.

  • Popup
    • There are a couple of places where you have &body in a URL in the HTML. That should be written as &amp;body.
      • Fixed

Unfortunately, one of these became &&amp;body in the process, which still isn't quite right.

  • JavaScript
    • Closing the modal reloads the original page. Is this necessary for some reason? It seems like surprising behavior, and it could potentially lose my place if I'm viewing a specific tab or something like that.
      • Without this, the popup does not work if accessed right after sending a report from within 404 error page. Hence, left it as is.

I see the bug you're describing, but this seems like it's a very brute-force solution to the problem. Digging into it a little more, I see that the problem always affects the "Report issue" button, but not the help menu items. It looks like this results from a bad interaction between the fact that the button says data-toggle="modal", and then the AJAX it receives also explicitly opens the modal with .modal('show'). In my environment, I was able to fix the issue by removing data-toggle and data-target from the button, along with the reload event. Could you please test this approach and see if it works for you too (and doesn't add any new bugs)?

  • The "Send problem report" button should be right of "Cancel" to better match our other dialogs.
    • Changed it. Though I don't necessarily like it :)

Aren't you a Mac user?! Apple started this trend! :)

  • The modal declares "overflow-y: scroll;" but I think the modal needs a height set for this to work as intended. In the current version, if the modal dialog is very long, the browser will simply grow the parent page to accommodate it. The scrollbar for the modal itself is never active. I can demonstrate this in Firefox by resizing the report textarea to be very tall.
    • Changed to auto as suggested below

These are separate issues. "auto" just means the browser won't show a scrollbar if it's not necessary. But without an express height, the browser still prefers to grow the modal indefinitely, rather than add a scrollbar to it. The issue I described is still reproducible.

  • Please remove apps/workbench/app/views/application/report_issue.js.erb.
    • Need this by the report_issue method in actions_controller. Otherwise, I get "Missing template actions/report_issue, application/report_issue" error

Since you don't need to send any data in this case, you can say format.js { render nothing: true } in the action method. That will also avoid the error, and I think it expresses your intent more clearly.

  • Would it be possible for us to set local_modified: <%= '-modified' if `git status -s` %> in the common configuration for both servers? I feel like that's more likely to accomplish the goals specced for this story.
    • I think they should be separate. It is possible that we run workbench locally pointing to staging, for example. In any case, I don't think it is worth the hassle.

Sorry, but I'm not sure I follow your example. The main example I'm thinking of is off-site deployments. If they're running in Rails' production mode, and have local changes to the git checkout, that information would be very valuable to include in the bug report. We can't prevent them from overriding the version information in their own configuration, but moving the git detection to the common section would help the right thing happen by default. It doesn't seem like it's noticeable hassle compared to the current situation—I'm only suggesting moving the line that's currently in the development section and moving it to common.

  • Tests
    • I am consistently getting this test failure: [...]
      • Unfortunately, this test always fails when the entire suite is run for me as well (not just in this branch). You can verify that it passes when you run only test/integration/logins_test.rb. In any case, not something caused by the updates in this branch.

I see the difference you describe between running that test individually and running the whole suite, but I can't reproduce this issue on master—it passes for me there. When this has happened in the past, what it usually means is that the failing test is expecting to run with a specific Capybara driver, and something has changed in the test structure so that it's running with a different one. You might try figuring out what driver it's using on a clean checkout of master, and then expressly setting that at the start of the test.

Two extra small things I noticed this round:

  • Please skip filters for the reporting actions in ActionsController, rather than ApplicationController. This will prevent trouble in case another controller subclass grows a report_issue method later.
  • In _report_error.html.erb, there are several instances where br tags are closed invalidly.

Thanks.

#18 Updated by Radhika Chippada about 5 years ago

Brett: addressed all your comments. Especially, thanks for testing that reload issue, which I should have fixed after fixing the links in the drop-down. Thanks.

#19 Updated by Brett Smith about 5 years ago

Just one last thing: the 404 page template only sets req_item_plain_text if there's an API error. This makes the error report say "The was not found." on any other 404 page. Please provide a default for the non-API case, and then I think this'll be good to go. Thanks.

#20 Updated by Radhika Chippada about 5 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:183bdcd6c6b2c54f8f2558fbdb6bec974c1d16e4.

Also available in: Atom PDF