Add UI controls for developer session #1691

Martchus · 2018-06-14T16:19:25Z

codecov · 2018-06-15T12:01:14Z

Codecov Report

Merging #1691 into master will increase coverage by 0.32%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1691      +/-   ##
==========================================
+ Coverage   89.54%   89.87%   +0.32%     
==========================================
  Files         137      137              
  Lines        9537     9616      +79     
==========================================
+ Hits         8540     8642     +102     
+ Misses        997      974      -23

Impacted Files	Coverage Δ
lib/OpenQA/WebAPI/Controller/LiveViewHandler.pm	`97.66% <100%> (+1.33%)`	⬆️
lib/OpenQA/Utils.pm	`93.69% <100%> (+0.05%)`	⬆️
lib/OpenQA/WebAPI/Controller/Test.pm	`96.66% <100%> (+0.12%)`	⬆️
lib/OpenQA/WebAPI/Controller/Developer.pm	`100% <100%> (ø)`	⬆️
lib/OpenQA/WebAPI.pm	`93.77% <100%> (+0.02%)`	⬆️
lib/OpenQA/Schema/ResultSet/DeveloperSessions.pm	`100% <100%> (ø)`	⬆️
lib/OpenQA/LiveHandler.pm	`94.28% <100%> (+0.73%)`	⬆️
lib/OpenQA/Schema/Result/Jobs.pm	`91.63% <0%> (+0.13%)`	⬆️
lib/OpenQA/Worker/Jobs.pm	`81.73% <0%> (+0.68%)`	⬆️
lib/OpenQA/Scheduler/Scheduler.pm	`90.43% <0%> (+1.59%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74e1224...1066d02. Read the comment docs.

Martchus · 2018-06-19T15:40:22Z

I adapted the existing tests for the new UI. ~~Let's see whether everything~~It actually passes now.

@foursixnine When cherry-picking your commit, I made the host in the assertion variable instead of expecting localhost.

So far, this PR will allow any admin to start the development session just by clicking on the panel header in the live view tab. As discussed, I'll try to make it 'more difficult' to create the development session. Maybe I'll actually add a 'Create developer session to apply selection' at the bottom of the form?

When that is done, I'll continue with extending the tests.

foursixnine · 2018-06-20T07:02:56Z

@Martchus,What does Create developer session to apply selection :P

Martchus · 2018-06-20T07:45:31Z

@foursixnine This button would be under the controls where you select what you actually want to do (e.g. pause at a specific module). So 'selection' refers to the form elements above the button. And it would lock those controls for other users by creating the developer session.

foursixnine

There will be some technical debt after this but looks good so far.

foursixnine · 2018-06-20T07:08:59Z

assets/javascripts/openqa.js

@@ -103,3 +103,32 @@ function alignBuildLabels() {
  var max = Math.max.apply(null, values);
  $('.build-label').css('min-width', max + 'px');
 }
+
+// returns an absolute "ws://" URL for the specified URL which might be relative
+function makeWsUrlAbsolute(url, servicePortDelta) {


Why not call it getabsolutewsurl? (It's just cosmetic I know ;))

Because it doesn't just return a value which is already stored somewhere. It actually computes the value (though this is not very expensive).

foursixnine · 2018-06-20T07:13:21Z

assets/javascripts/running.js

+    if (toPauseAtIndex < 0) {
+        toPauseAtIndex = 0;
+    }
+    var moduleToPauseAtStillAhead = developerMode.moduleToPauseAt


I would enclose this inside ( )

Ok, I'll change that to make it less ambiguous.

foursixnine · 2018-06-20T08:55:14Z

templates/test/module_select.html.ep

@@ -0,0 +1,18 @@
+<select class="custom-select" name="pause-at-module" id="developer-pause-at-module">
+    <option selected style="font-style: italic;">Don't pause at a module</option>


I would change it to 'Do not pause'

Ok - so far there are no other ways to pause anyways.

foursixnine · 2018-06-20T09:21:32Z

lib/OpenQA/WebAPI/Controller/LiveViewHandler.pm

-# define global variable to store ws connections (tx) to JavaScript clients for each job
-my %java_script_transactions_by_job;
+# define global variable to store development session ws connections (tx) to JavaScript clients for each job
+my %devel_java_script_transactions_by_job;


This can be easily converted to a property of the controller via has: has blah => sub { {} };

No, it can't. Properties via has are non-static members. This has to be a static member variable. (To use sane C++ terms here.)

The reason is that Mojo will instantiate a separate controller object for each request it serves. At least in my tests it creates multiple controller objects. However, the transactions are shared by all requests to be able to broadcast.

So if you want to refactor this, the property must be on the application. But for now I would say there are more important things to adjust.

foursixnine · 2018-06-20T09:32:02Z

So 'selection' refers to the form elements above the button. And it would lock those controls for other users by creating the developer session.

Ah cool

Martchus · 2018-06-20T15:43:35Z

@coolo I've just mocked a possible workflow (imagine that the form will contain more elements later):

No developer session opened in the first place:
After clicking the panel header, the user can select but the session is not immediately started:
After clicking confirm:

Ok for you?

coolo · 2018-06-20T16:21:22Z

There seems to be a confusion here, because 'Developer mode' to me means the job is bound to a developer. Jobs not bound to a specific developer shouldn't even pause. If that developer has currently a session open or not is for sure interesting information because if not, another operator should at least have the option to cancel the job.

As such a "Developer session" to me is having a tab open while you're the developer. You don't start the session as such - you become the developer and happen to have a tab open.

foursixnine · 2018-06-26T12:36:19Z

templates/test/live.html.ep

+    <div class="card-body">
+        <form action="#" method="get" id="developer-form" class="developer-mode-element" data-hidden-on="lockedByOtherDeveloper">
+            <p class="developer-mode-element" data-hidden-on="ownSession">
+                Select what you want to do and confirm using the button below. This will start a developer


@cyntss Can you give your blessing here on the UI/UX side of things? :). There are some screenshots attached, but you can always do your own run. /me wonders if mail would work better :P

Hey! I think it looks good. I would only make the upper corners rounded as well in the blue header here (although im judging based on the screenshots you put, so maybe im missing something else below):

Also, what would "unknown status" mean to the user? can we give some information under a (?) icon that once clicked opens a popover or modal with more information?

it makes sense now that I see it in openQA.

foursixnine · 2018-06-26T12:43:36Z

templates/test/live.html.ep

+        <div class="card-header-appendix" id="developer-session-info">
+            % if (my $developer_session = $job->developer_session) {
+                % my $tab_count = $developer_session->ws_connection_count;
+                opened by <%= $developer_session->user->name %>


foursixnine · 2018-06-26T12:47:02Z

templates/test/live.html.ep

+                </a>
+                <a href="#" onclick="quitDeveloperSession(); return false;"
+                    class="btn btn-danger developer-mode-element" data-visible-on="ownSession">
+                    <i class="far fa-stop-circle"></i> Cancel job


Clicking on Cancel Job, will simply cancel the developer session. Better rename it to cancel developer session, and cancel the job. unless @coolo has a different opinion. But for me a test that was paused and likely touched, is tainted.

I still find the phrase 'developer session' pretty rather vague. To me a 'developer session' is the time between cigarettes - but we are looking for something more long term :)

foursixnine · 2018-06-26T12:49:20Z

templates/test/live.html.ep

+                </a>
+                <a href="<%= url_for('developer_ws_console')->query({proxy => 1}) %>" target="blank"
+                    class="btn btn-secondary developer-mode-element" data-visible-on="ownSession">
+                    <i class="fas fa-terminal"></i> Open console


Hmmm I think it's safe to hide the console.

Yes, I'll get rid of the console button. Or maybe I show it only when launching the Mojo app in development mode? Because when developing (the developer mode) it can still be useful to open that console.

+1 with the development mode of the Mojo app

implemented

foursixnine · 2018-06-26T12:50:05Z

templates/test/module_select.html.ep

@@ -0,0 +1,18 @@
+<select class="custom-select" name="pause-at-module" id="developer-pause-at-module">


As a follow up, we can set to disabled the options that are no longer available.

Ok, would be quite easy I guess.

foursixnine · 2018-06-26T12:54:03Z

assets/javascripts/running.js

+    if (developerMode.currentModule && !moduleToPauseAtStillAhead) {
+         statusInfo += ', at ' + developerMode.currentModule;
+    }
+    $('#developer-status-info').text(statusInfo);


I would actually put the name of the module in bold when the test execution is set to be paused.

Martchus · 2018-06-26T14:01:50Z

@cyntss Here's a more recent screenshot:

Taken from http://e212.suse.de/tests/12263 where you can also play around.

coolo · 2018-06-27T16:08:09Z

I think 'cancelled' or 'incomplete' is good enough for now - I was actually quite suprised when I controlled a circleci.com job through ssh that it ended passing even though I was debugging a failure :)
Possibly even autorestart?

And about the MAX_JOB_TIME: I would prefer to enlarge it, but that requires a call into the worker.

foursixnine · 2018-06-27T16:42:21Z

@coolo that was exactly my point :P, autorestart sounds good.

Let's add a ticket for the enlargement of the MAX_JOB_TIME. no pun intended.

foursixnine · 2018-06-27T16:50:34Z

@cyntss the first part of your comment has the image twice :P. But i'll send you a mail in any case :)

Martchus · 2018-06-28T10:13:50Z

Ok, this contains now replacements suggested by @foursixnine and no more 'developer session'. I didn't change the case (eg. 'pause' => 'Pause') because this would be inconsistent with the other panels of the kind. If we want to capitalize everything, then we should it consistently (and not within this PR).

Apparently popovers can not be in a form label because it otherwise instantly loses focus again.

Martchus · 2018-06-28T15:32:19Z

I fixed some more bugs in the JavaScript. Likely not the last ones, but still I'd like to merge this until the PR grows too big. Let's see whether it still passes the checks.

* Prevent overriding existing values with form defaults * Prevent updating form controls when the test hasn't been locked by anybody and the form is just shown initially * Don't submit any changes unless testStatus.running

Martchus · 2018-06-29T09:33:08Z

The tests pass now again. If there are no more objections, I'd like to merge this before continuing. The PR is big enough already.

coolo · 2018-06-29T09:49:43Z

Well, time to do some user testing with colleagues I'd say

Martchus · 2018-06-29T11:08:42Z

Ok, so I'll deploy the latest version on e212 and try to find some victims :-)

okurz · 2018-06-29T12:21:47Z

/me sneakingly trying it out on e212 in parallel.

Q1: I would like to pause the test at the bootloader. How to do that?
Q2: "openqaworker12 at port 112 with Shared mode (otherwise the job might be cancelled)". In old versions on hover the VNC port would be shown. I assume "port 112" should actually say "display 112" or "VNC port 6012". Ok the "assigned worker" already tells it. I guess somehow the information is a bit redundant

Martchus · 2018-06-29T14:00:07Z

I summarize the feedback we've got so far:

The info for connecting via VNC is confusing. Especially the part that the job might be cancelled.
It basically seems to work. No critical bugs so far.
The "ownership" should be explained further.
People are already asking for further features like pausing in the current state.

So nothing which prevents merging this from my point of view. I think @foursixnine can take care about 1. since he wrote that message. I will do 3. in some follow-up (likely with some more adjustments). 4. is out of the scope of this PR anyways.

Martchus · 2018-06-29T14:13:00Z

@okurz I over-read your comment.

Q1: You start the test with the setting PAUSE_AT=bootloader (possible without that PR). This job setting should be documented somewhere like all of this when it is in a stable state.
Q2: I already noted that we need to improve those instructions.

okurz · 2018-06-29T18:51:49Z

awesome!

foursixnine · 2018-07-02T08:15:57Z

@okurz let's make it "VNC port 6012" instead. @coolo already pointed that out too

foursixnine · 2018-07-02T08:31:11Z

PR open: #1702

okurz · 2018-07-02T09:11:54Z

@okurz let's make it "VNC port 6012" instead

Yes, but that information is already available from the info about the assigned worker which also outputs the hostname

foursixnine · 2018-07-02T11:06:34Z

@okurz this is only display explicitly when the test is paused. What else would you suggest here? (as the default information for the assigned worker, is to show the worker instance, and you only get to see the VNC port after placing the mouse on the element)

…

On Mon, Jul 2, 2018 at 11:11 AM Oliver Kurz ***@***.***> wrote: ***@***.*** let's make it "VNC port 6012" instead Yes, but that information is already available from the info about the assigned worker which also outputs the hostname — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1691 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN_eL2hOOj-Rxo5sG_SJXzkDM3QplwSks5uCePbgaJpZM4UoQ3f> .

Martchus mentioned this pull request Jun 14, 2018

WIP: Provide instructions on how to connect to SUT #1671

Closed

Martchus force-pushed the enhance_live_devel_ui branch from c78ed06 to 4d2564f Compare June 15, 2018 12:01

Martchus force-pushed the enhance_live_devel_ui branch 6 times, most recently from 28307f2 to 5e897ad Compare June 19, 2018 15:34

foursixnine reviewed Jun 20, 2018

View reviewed changes

Martchus force-pushed the enhance_live_devel_ui branch 8 times, most recently from 393b7d8 to e246fe8 Compare June 25, 2018 12:51

Martchus changed the title ~~WIP: Add UI controls for developer session~~ FEEDBACK: Add UI controls for developer session Jun 26, 2018

foursixnine reviewed Jun 26, 2018

View reviewed changes

Martchus and others added 2 commits June 26, 2018 16:19

Add UI controls for developer session

fbcd903

Expose VNC port to developer

93354f6

Martchus force-pushed the enhance_live_devel_ui branch from d06c35f to 02b46f6 Compare June 28, 2018 09:21

Martchus changed the title ~~FEEDBACK: Add UI controls for developer session~~ Add UI controls for developer session Jun 28, 2018

Martchus added 6 commits June 28, 2018 16:34

Rephrase to avoid 'developer session'

057842c

Don't update developer mode controls until confirmed

3c1a6b2

Never assume it is the own devel session when not logged in

4af9d62

devel mode: Handle case when module list still unknown

c7d3ae7

Fix help popover in developer panel

7b3eae7

Apparently popovers can not be in a form label because it otherwise instantly loses focus again.

Show developer mode form only when connected via web sockets

513ef24

Martchus force-pushed the enhance_live_devel_ui branch from 1f0f5c0 to 2ed5ac4 Compare June 28, 2018 15:25

Martchus added 2 commits June 29, 2018 11:03

Fix bugs in form controls for devel mode

77e88c6

* Prevent overriding existing values with form defaults * Prevent updating form controls when the test hasn't been locked by anybody and the form is just shown initially * Don't submit any changes unless testStatus.running

live view: Connect to ws server when testStatus.running is set

1066d02

Martchus force-pushed the enhance_live_devel_ui branch from 2ed5ac4 to 1066d02 Compare June 29, 2018 09:05

coolo merged commit 86bc960 into os-autoinst:master Jun 29, 2018

Martchus deleted the enhance_live_devel_ui branch June 29, 2018 15:06

		@@ -0,0 +1,18 @@
		<select class="custom-select" name="pause-at-module" id="developer-pause-at-module">
		<option selected style="font-style: italic;">Don't pause at a module</option>

Add UI controls for developer session #1691

Add UI controls for developer session #1691

Conversation

Martchus commented Jun 14, 2018

codecov bot commented Jun 15, 2018 • edited Loading

Codecov Report

Martchus commented Jun 19, 2018 • edited Loading

foursixnine commented Jun 20, 2018

Martchus commented Jun 20, 2018 • edited Loading

foursixnine left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foursixnine commented Jun 20, 2018

Martchus commented Jun 20, 2018

coolo commented Jun 20, 2018

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

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

Martchus commented Jun 26, 2018

coolo commented Jun 27, 2018

foursixnine commented Jun 27, 2018

foursixnine commented Jun 27, 2018

Martchus commented Jun 28, 2018 • edited Loading

Martchus commented Jun 28, 2018

Martchus commented Jun 29, 2018

coolo commented Jun 29, 2018

Martchus commented Jun 29, 2018

okurz commented Jun 29, 2018 • edited Loading

Martchus commented Jun 29, 2018

Martchus commented Jun 29, 2018 • edited Loading

okurz commented Jun 29, 2018

foursixnine commented Jul 2, 2018

foursixnine commented Jul 2, 2018

okurz commented Jul 2, 2018 via email

foursixnine commented Jul 2, 2018 via email

codecov bot commented Jun 15, 2018 •

edited

Loading

Martchus commented Jun 19, 2018 •

edited

Loading

Martchus commented Jun 20, 2018 •

edited

Loading

Martchus commented Jun 28, 2018 •

edited

Loading

okurz commented Jun 29, 2018 •

edited

Loading

Martchus commented Jun 29, 2018 •

edited

Loading