Arvados

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.

(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');

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

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 &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

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

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 &body.
    • Fixed

Unfortunately, one of these became &&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.

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.

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.