Bug 8083 - Multiple sessions are presented in an unorderly way
Summary: Multiple sessions are presented in an unorderly way
Status: CLOSED FIXED
Alias: None
Product: ThinLinc
Classification: Unclassified
Component: Web Administration (show other bugs)
Version: trunk
Hardware: PC Unknown
: P2 Normal
Target Milestone: 4.15.0
Assignee: Linn
URL:
Keywords: relnotes, tobfa_tester
Depends on:
Blocks:
 
Reported: 2023-02-02 10:55 CET by Linn
Modified: 2023-05-11 08:33 CEST (History)
4 users (show)

See Also:
Acceptance Criteria:
* Each session should have its own row in the session table * The columns in the session table should be the same as those in tlctl * When clicking a session, the corresponding info should be shown in the popup * When clicking a session that is no longer alive, the popup should indicate that the session is unavailable * When _not_ checking the confirm checkbox and terminating a session: 1) you should stay in the popup and, 2) a hint about the checkbox being required should be presented * When checking the confirm checkbox and terminating a session, the termination message should be shown * When clicking "Shadow session", the client launched by the downloaded file should contain the following: - Shadowing should be enabled - The clicked username should show in field "User to shadow"

Attachments
Add an attachment (proposed patch, testcase, etc.)

Description Linn cendio 2023-02-02 10:55:56 CET 
Currently, multiple sessions for one user are presented all at once in a large table. This makes sense since the session table shows the username and how many sessions that user have, but if a user have several multiple sessions the current layout quickly feels cumbersome. 

Also, the way the session table is designed right now, the focus is on the users rather than on the sessions themselves.

One suggestion to make the session table give a better overview of the sessions is by having each session as a separate row. Then it would also be natural to show information about one session at a time.
Comment 1 Linn cendio 2023-02-02 11:37:27 CET 
This would also mean that the columns in the session table would have to change to something more informative. 

As part of implementing tlctl for session info (bug 425) we used username, display number and agent name as identifiers, so that data is useful to have in Web Admin too.
Comment 13 Frida Flodin cendio 2023-02-22 12:20:57 CET 
When I terminate a session I get a new popup with "Session is not available" and N/A in all fields. I have not investigated why, but I believe it could be due to changes on this bug.
Comment 23 Linn cendio 2023-03-14 13:13:27 CET 
When testing that the termination message was correctly presented, I tested the following scenarios:

 * Terminating the last active session (out of all sessions across all users)

- The scenarios below had extra sessions for other users in the session table, to check that the session list was updated correctly.

 * User has 2 sessions, terminate one of them. Got message:
> Terminated session 11 for tester2 on agent 10.47.1.10. 
 * User has 1 session, terminate it. Got message:
> Terminated session 12 for tester2 on agent 10.47.1.10. 
 * User has 2 sessions, one session has ended but it is still visible in the session database. When terminating the ended session I got message:
> Error terminating session: Could not find session 10 for tester on agent 10.47.1.10 
 * User has 1 session that has ended but it is still visible in the session database. Got message:
> Error terminating session: Could not find session 13 for tester on agent 10.47.1.10

There is one more code-way that I couldn't see a way to test practically - when a session first is missing from the session database, but it then shows up when getting verified session data. There are unit tests for this and the behaviour of updating the session list is unchanged to that before commit r39888.


Tips for tester
---------------
To get an ended session that is still visible in the session database, I did the following steps:

1. Start a new session. Do not click past the profile chooser.
2. In webadmin, go to Status -> Sessions. Click on the session you are planning to terminate to show details.
3. Go back to your session. Press 'Quit' in the profile chooser.
4. Go back to webadmin, check the terminate checkbox and click Terminate. The termination message should be the same as described above.
Comment 24 Linn cendio 2023-03-14 14:27:12 CET 
When doing this bug, we noticed that get_sessions() had a different return format when only filtering on usernames ( { username: [] } ), but another return format for all other filtering ({}).

A fix for this was done in r39728, this is the mapping I did to find affected files:
 - greping after 'get_sessions', check if we were filtering only on username
 - greping after the functions in sessionstore (get_user_session* and get_all_sessions_public)


These are the files I found and my evaluation of them:

- handler_getsessions: The cause of the bug, needed changes

- tlwebadm/status: Relied on the previous return format of handler_getsessions, needed changes

- tlprinter: Used no filters when retrieving sessions with get_sessions(), not affected by the bug.

- tlctl/session: Filters for get_sessions() is chosen by user, so file is affected by the bug. However, the functions that are using the return value are side stepping the issue by using loops, so in practice they are unaffected. These functions are _create_table_contents() and count_sessions().

- handler_getpublicsessioninfo: Called get_user_sessions_public() directly, not affected by the bug.
Comment 27 Linn cendio 2023-03-17 17:20:39 CET 
We should also consider how the sessions are sorted in the sessions table. In tlctl, the sessions are sorted first on username and then on display number. In Web Admin, they are sorted first on username and then creation time, where the oldest sessions are presented first. 

We should probably keep the sorting synced between tlctl and Web Admin.

Order tlctl:
> USER     DISPLAY  AGENT      STATUS     AGE     
> ================================================
> tester   10       127.0.0.1  connected  15 min  
> tester   13       127.0.0.1  connected  3 day(s)
> tester2  11       127.0.0.1  connected  15 min  
> tester2  12       127.0.0.1  connected  1 min 
Order Web Admin:
> Username 	Display Agent 	        Status 	        Age
> tester 	13 	127.0.0.1 	connected 	3 day(s)
> tester	10 	127.0.0.1 	connected 	14 min
> tester2 	11 	127.0.0.1 	connected 	14 min
> tester2 	12 	127.0.0.1 	connected 	<1 min
Comment 39 Linn cendio 2023-03-21 17:26:50 CET 
The issue with usernames not being sorted in the current locale has been broken out to bug 8122.
Comment 40 Linn cendio 2023-03-22 09:06:05 CET 
(In reply to Linn from comment #27)
> We should also consider how the sessions are sorted in the sessions table.
> In tlctl, the sessions are sorted first on username and then on display
> number. In Web Admin, they are sorted first on username and then creation
> time, where the oldest sessions are presented first. 
> 
> We should probably keep the sorting synced between tlctl and Web Admin.
> 
> Order tlctl:
> > USER     DISPLAY  AGENT      STATUS     AGE     
> > ================================================
> > tester   10       127.0.0.1  connected  15 min  
> > tester   13       127.0.0.1  connected  3 day(s)
> > tester2  11       127.0.0.1  connected  15 min  
> > tester2  12       127.0.0.1  connected  1 min 
>
> Order Web Admin:
> > Username 	Display Agent 	        Status 	        Age
> > tester 	13 	127.0.0.1 	connected 	3 day(s)
> > tester	10 	127.0.0.1 	connected 	14 min
> > tester2 	11 	127.0.0.1 	connected 	14 min
> > tester2 	12 	127.0.0.1 	connected 	<1 min

The sorting is now synced between tlctl and Web Admin, sorting on username and display number.
Comment 42 Linn cendio 2023-03-22 12:28:21 CET 
Tested the acceptance criteria along the way on Fedora 37 and Firefox, and things seems to be working:

> * Each session should have its own row in the session table
> 
> * The columns in the session table should be the same as those in tlctl
Yes, each row is a different session and the columns are synced with tlctl.


> * When clicking a session, the corresponding info should be shown in the popup
Yes, the popup is shown and the session info shown in session table is the same as shown in the popup.


> * When clicking a session that is no longer alive, the popup should indicate that the session is unavailable
Yes, session info is all N/A and there is a an error notification saying "Session is not available". For tips on how to trigger this, see comment 23.


> * When _not_ checking the confirm checkbox and terminating a session:
>   1) you should stay in the popup and, 
>   2) a hint about the checkbox being required should be presented
> 
> * When checking the confirm checkbox and terminating a session, the termination message should be shown in the popup
Yes, see comment 23  for full details.


> * When clicking "Shadow session", the client launched by the downloaded file should contain the following:
>  - Shadowing should be enabled
>  - The clicked username should show in field "User to shadow"
Yes, the client opened had both shadowing enable and showed the correct username in this field. This was tested on Ubuntu 18.04.
Note that in order to test this your machine needs to run an Apache HTTP Server, see tips for how to set it up here:
https://www.cendio.com/resources/docs/tag-devel/html/client_web_integration.html#clientplatforms-web-integration
Comment 44 Linn cendio 2023-03-22 16:58:15 CET 
Forgot to add release notes before setting as resolved - they are in place now.
Comment 45 Samuel Mannehed cendio 2023-03-24 18:56:46 CET 
I tested ThinLinc server build 3160 on RHEL 9.

Acceptance criteria:

> * Each session should have its own row in the session table
Yes, and it looks wonderful!

> * The columns in the session table should be the same as those in tlctl
Yep, with more clear, non-truncated names in Web Admin.

> * When clicking a session, the corresponding info should be shown in the popup
Yes, more detailed information is available. And all information available in the overview table is also available in the popup.

> * When clicking a session that is no longer alive, the popup should indicate that the session is unavailable
Yes, I opened the sessions page with 3 sessions alive, logged out of one of them using the ThinLinc Client, and then clicked that session on the sessions page, without refreshing first.

Doing so quickly gets me a popup saying “Session is not available”. The error message is clear. The title is correct. The popup table shows “N/A” values for all fields. The “Terminate session”, confirm checkbox, checkbox label, and “Shadow session” buttons are all disabled properly.

After closing the popup, the logged-out session is gone from the sessions list.

> * When _not_ checking the confirm checkbox and terminating a session:
>   1) you should stay in the popup and, 
>   2) a hint about the checkbox being required should be presented
Yes, it works well for an alive session. The popup doesn't close, and the “(* Required)” text shows next to the checkbox.

I also tried opening the popup for a session in Web Admin, then logging out from that using the ThinLinc Client, then trying to terminate without checking the confirm-box. That correctly lands me in the “Session is not available” state – nice!

> * When checking the confirm checkbox and terminating a session, the termination message should be shown in the popup
Yes, I get a pleasant popup saying “Terminated session 11 for test2 on agent lab-30.lkpg.cendio.se”.

Doing the same on a logged-out session, gives me a nice popup saying “All sessions for test1 already terminated.”

> * When clicking "Shadow session", the client launched by the downloaded file should contain the following:
>  - Shadowing should be enabled
>  - The clicked username should show in field "User to shadow"
Yes. I added a user as an allowed shadower. I then followed the instructions from our wiki for setting up Web Integration on RHEL 9:

https://intranet.lkpg.cendio.se/ThinLinc/Testing/Functions/Web%20integration

Then I clicked “Shadow session” on an active session. I got presented with a download of a “launch.tlclient” file, which correctly started a ThinLinc client with shadowing enabled and the username of the user I clicked on pre-filled in.

---

I have looked over the commits made for this bug (as they were committed) and the code looks good.

I also checked both the release notes and the modified documentation, and they both look good.

All good, closing.
Comment 48 Linn cendio 2023-04-06 11:12:33 CEST 
Reopening. 

The session page is the only place where a message is shown the pop-up, and we want to be consistent with how messages are shown in Web Admin. Other pages simply use the message styling (text in a gray "bubble") to show their messages, whether those messages are shown as part of the pop-up or elsewhere. The session page should use the same styling for the termination message.

Note that there is also bug 8135 to make sure the messages are shown and presented consistently over the Web Admin pages. The aim of this bug (8083) is to just move the message out of the pop-up, to make it look "close enough" to other messages.
Comment 52 Linn cendio 2023-04-11 10:48:34 CEST 
The termination message has now been moved to the top-level session page and is presented in our usual "message notification" formatting.

Also checked the following:
 - The termination message gets a corresponding anchor tag
 - On termination error, the user stays in the pop-up
 - Details are correctly shown for a session
 - Details with N/A values are shown for an ended session


Note that a possible presentation of the termination message could have been to stay in the details popup, with the details data grayed out and showing the message notification at the top of the popup. The more simple and quick solution with the top-level message was deemed good enough for now, and hence it was the preferred solution in this case.
Comment 54 Tobias cendio 2023-04-12 14:42:13 CEST 
Tested server build #3190 on Fedora37 in Firefox111.0.1.

The reason why this bug was reopened was that termination messages did not follow the style of other pages, i.e. a message at the top of the base page. This seems to be working as intended now, as the user exits the popup upon confirmed termination and the termination message appears properly.

Furthermore, some changes were made in commits r40045 and r40047 that affected how the details element is handled. While most testing was addressed in comment 45, I decided to have a look at most of the acceptance criteria once more (disregarding shadowing).

> Each session should have its own row in the session table
Check.

> The columns in the session table should be the same as those in tlctl
Check.

> When clicking a session, the corresponding info should be shown in the popup
Check.

> When clicking a session that is no longer alive, the popup should indicate that the session is unavailable
Check.

Note that this is working as intended for disconnected sessions. However, the page hangs for terminated sessions for some reason. For instance, if a session has been terminated via tlctl, clicking on that session in tlwebadm hangs the page. Same thing happens if two tlwebadm session pages are open in parallel and sessions are terminated through one of them. There is a bug on the subject though, bug 7839, so this we should view this criterion as fulfilled.

> When _not_ checking the confirm checkbox and terminating a session:
>  1) you should stay in the popup and, 
>  2) a hint about the checkbox being required should be presented
Check.

> When checking the confirm checkbox and terminating a session, the termination message should be shown
Check.

Conclusion:
The acceptance criteria are fulfilled and termination messages are conveyed consistent with other tlwebadm pages.

Closing.
Comment 55 Frida Flodin cendio 2023-04-24 15:54:43 CEST 
I think I found a regression from this bug. When master service is turned off you get an error page when surfing to the session page. And this is the traceback in tlwebadm.log: 

> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1] ----------------------------------------
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1] Exception happened during processing of request from ('::1', 52962, 0, 0)
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1] Traceback (most recent call last):
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thinlinc/modules/thinlinc/forkingserver.py", line 62, in process_request
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     self . finish_request ( request , client_address )
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thinlinc/modules/thinlinc/httpserver.py", line 407, in finish_request
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     super ( ) . finish_request ( request , client_address )
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thinlinc/modules/thinlinc/tlstunnel.py", line 71, in finish_request
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     self . TLSRequestHandlerClass ( request , client_address , self )
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thinlinc/modules/thinlinc/httpserver.py", line 78, in __init__
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     super ( ) . __init__ ( request , client_address , server )
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/usr/lib64/python3.10/socketserver.py", line 747, in __init__
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     self.handle()
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thinlinc/modules/thinlinc/httpserver.py", line 380, in handle
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     super ( ) . handle ( )
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/usr/lib64/python3.10/http/server.py", line 433, in handle
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     self.handle_one_request()
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thinlinc/modules/thinlinc/httpserver.py", line 155, in handle_one_request
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     super ( ) . handle_one_request ( )
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/usr/lib64/python3.10/http/server.py", line 421, in handle_one_request
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     method()
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thinlinc/modules/thinlinc/tlwebadm/server.py", line 168, in do_GET
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     self . post_or_get ( "do_GET" , ooOOOoOO0 )
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thin
> linc/modules/thinlinc/tlwebadm/server.py", line 182, in post_or_get
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     oO00o00OO , iIi1 = getattr ( OoOo00 , action ) ( i1iI1i , query , oO00o00OO )
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thinlinc/modules/thinlinc/tlwebadm/main.py", line 133, in do_GET
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     IiII = self . _content_to_bytes (
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thinlinc/modules/thinlinc/tlwebadm/main.py", line 112, in _content_to_bytes
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     return self . _render_page ( page_name , content ) . encode ( 'utf-8' )
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thinlinc/modules/thinlinc/tlwebadm/main.py", line 105, in _render_page
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     return str ( self . _main_page )
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thinlinc/modules/thinlinc/Cheetah/Template.py", line 1053, in __unicode__
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     return getattr(self, mainMethName)()
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "_opt_thinlinc_share_tlwebadm_templates_main_tmpl.py", line 121, in respond
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thinlinc/modules/thinlinc/Cheetah/Filters.py", line 131, in filter
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     output = super(MaxLen, self).filter(val, **kw)
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thinlinc/modules/thinlinc/Cheetah/Filters.py", line 40, in filter
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     return unicode(val)
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thinlinc/modules/thinlinc/Cheetah/Template.py", line 1053, in __unicode__
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     return getattr(self, mainMethName)()
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "_opt_thinlinc_share_tlwebadm_templates_status_sessions_tmpl.py", line 217, in respond
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thinlinc/modules/thinlinc/Cheetah/NameMapper.py", line 293, in valueFromSearchList
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     _raiseNotFoundException(key, searchList)
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]   File "/opt/thinlinc/modules/thinlinc/Cheetah/NameMapper.py", line 178, in _raiseNotFoundException
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1]     raise NotFound(excString)
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1] thinlinc.Cheetah.NameMapper.NotFound: cannot find 'sess_details'
> 2023-04-24 14:27:07 ERROR tlwebadm[45859]: [::1] ----------------------------------------
Comment 56 Frida Flodin cendio 2023-04-25 16:35:00 CEST 
When digging deeper into this, I found that this was not due to this bug. It was a consequence of adding anchors to the sessions page for bug 7909.
Comment 58 Linn cendio 2023-04-27 09:32:35 CEST 
Did a small change to the documentation related to this bug, do not need retesting.
Comment 59 Linn cendio 2023-05-03 09:05:18 CEST 
There are some minor comments on the commits for this bug that I think it is worth to reopen this bug for:

 * Show unreachable connection status in the same way as tlctl and the client does, i.e. by displaying the status as 'unreachable'.

 * Change how the table in the popup is created for both load and sessions pages. Right now it is hidden away in a function, which makes it harder to get an overview of the template file. There is also a more complex data structure used, these should be avoided in our templates to keep them as simple as possible.

 * In the popup for last connection, add info about the age of that date.
Comment 64 Linn cendio 2023-05-04 12:31:34 CEST 
Tested after fixing the points mentioned in comment 59, and they all seem to be resolved.

* Unreachable sessions now get status "unreachable" in the sessions table, and session status "unreachable" and connection status "unknown" in the popup.

* The code for creating the details table in the popup for load and sessions pages have been updated, and the tables still have the same content as before.

* In the session details popup, info about the age of the last connection has been added. The choice to use a simple <br> between the timestamp and the age was to harmonise with how the titles for each field handle line breaks. To me, the spacing with a <br> looks ok but on the cramped side. Once bug 8067 which handles line height is done, there should be a bit more fluff between the timestamp and the age.


Tips for tester:
To get an unreachable session, start a session and then stop the agent service on the machine the session is on.
Comment 65 Alexander Zeijlon cendio 2023-05-09 09:31:36 CEST 
I have gone through the code that's been changed after the latest reopen. Overall, everything looks good!

There are a couple of things that could be good to take a second look at:

* The function added in comment 60 that reduces the results of multiple status variables into one, looks trivial on the surface, but exists to make it clear that we are mimicking the behavior of another, separate component. This fact should be reflected f.ex. in a doc string to make it clear that this is duplicated behavior.

* The updated documentation in comment 63 for the session status variable is currently a bit too ambiguous. It could benefit from a more detailed explanation, such as what values it can hold or which signals it listens to.
Comment 68 Linn cendio 2023-05-09 13:43:04 CEST 
> * The function added in comment 60 that reduces the results of multiple
> status variables into one, looks trivial on the surface, but exists to make
> it clear that we are mimicking the behavior of another, separate component.
> This fact should be reflected f.ex. in a doc string to make it clear that
> this is duplicated behavior.
I added a comment for the function in question, and renamed it to better reflect its purpose.


> * The updated documentation in comment 63 for the session status variable is
> currently a bit too ambiguous. It could benefit from a more detailed
> explanation, such as what values it can hold or which signals it listens to.
Updated the documentation to add that this status is how the master machine sees the session.


With that, the remaining comments have been taken care of, setting to resolved.
Comment 69 Alexander Zeijlon cendio 2023-05-11 08:33:49 CEST 
The issues mentioned in comment 60 and comment 63 have been addressed in comment 68, and I agree with the changes, this bug can be closed.
Note You need to log in before you can comment on or make changes to this bug.