Arvados

Branch looked good overall. Some of my observations:

• Ctrl-click on pipeline instance “Re-run options” is working in staging already and I see the dialog. It is of course working the same way in the new branch as well

• Right click -> "Open in new tab" working both in staging and new branch on “Re-run options”

• Right click -> "Open in new tab" on Pipeline instance -> “Re-run with latest” button shows fiddlesticks in staging. In the branch, the right click does not show the “Open in new tab” link though. It shows drop down menu with Back, Forward, Reload and other options only. All in all, this is the one improvement I observed in this branch since Ctrl-click is already working.

• Project -> "Run a pipeline…" button: Ctrl+click does not do anything (Ctrl-click on Move project works as before)

• The comments in helpers/application_helper.rb:
  • I see this comment “ When using {remote:true} or {method:...}, move the target URI from …” . Would you update this comment to say non-GET methods since we are changing only when method != GET?
  • This comment: “but there's no ‘copy link address’ option in the right-click menu” … I saw this in right-click menu (in the branch) and it also worked as expected.

• I did some manual testing around pipeline running and re-running but did not do extensive manual testing

• I ran only the project and pipeline integration tests, unit and controller tests (which all passed) and did not run all the other workbench integration tests …

• The diagnostics test failed for me but I did not spend much time debugging this. As long as you tested it and it is working for you, I am ok with it.

Radhika Chippada wrote:

Branch looked good overall. Some of my observations:

• Ctrl-click on pipeline instance “Re-run options” is working in staging already and I see the dialog. It is of course working the same way in the new branch as well

• Right click -> "Open in new tab" working both in staging and new branch on “Re-run options”

True, that button didn't exhibit this bug. It uses href="#" and Bootstrap data-toggle: if you ctrl-click it, you get a dialog, and if you "copy link address" and paste it somewhere, you just get the current page again. (This seems less serious than giving a Fiddlesticks, but I'd characterize it as "still weird" rather than "still working"...)

• Right click -> "Open in new tab" on Pipeline instance -> “Re-run with latest” button shows fiddlesticks in staging. In the branch, the right click does not show the “Open in new tab” link though. It shows drop down menu with Back, Forward, Reload and other options only. All in all, this is the one improvement I observed in this branch since Ctrl-click is already working.

Right. The fix here is: when the button makes an HTTP request that isn't GET or doesn't return HTML, it doesn't make sense to offer "open in new tab" or "copy link address" -- you'd just get an error, or some unexpected behavior -- so don't offer those things. For example, there is no link address that you can copy and paste to get the "re-run with latest" action, so that button shouldn't offer to "copy link address" or "open in a new tab".

• Project -> "Run a pipeline…" button: Ctrl+click does not do anything (Ctrl-click on Move project works as before)

Hm, the difference here seems to be that "Run a pipeline..." doesn't specify data-method. Haven't dug deeper than that.

• The comments in helpers/application_helper.rb:
  • I see this comment “ When using {remote:true} or {method:...}, move the target URI from …” . Would you update this comment to say non-GET methods since we are changing only when method != GET?

Updated, 17efed0

• This comment: “but there's no ‘copy link address’ option in the right-click menu” … I saw this in right-click menu (in the branch) and it also worked as expected.

Hm. Which button is still doing this? Is it a button that was affected by this bug/helper? If you copy that link address into the location bar, does it show something reasonable?

• The diagnostics test failed for me but I did not spend much time debugging this. As long as you tested it and it is working for you, I am ok with it.

Diagnostics (and other) tests pass for me.

Tom said:

This comment: “but there's no ‘copy link address’ option in the right-click menu” … I saw this in right-click menu (in the branch) and it also worked as expected.

Hm. Which button is still doing this? Is it a button that was affected by this bug/helper? If you copy that link address into the location bar, does it show something reasonable?

You are right. This is the other "Rerun with options" button. So, please ignore my comment.

Diagnostics (and other) tests pass for me

My diagnostics testing config was out of date. I updated it and it is working for me as well. It is not impacted by work in this branch.

LGTM