Improve UX with two users that have identical names

[mrjones-plip]

Right now if you have two identically named users on different systems, you can't tell them apart (see #7). We should fix this so you can tell who is who.

Some ideas:

• detect users with identical names, conditionally show IP addresses for them
• always show IP addresses, instead of only showing when there's an error
• allow users to have "Remote User Name" vs "Display Name in Timekpr-next-remote" in config.py.
• something else!?

[hydrian]

I've been working on this issue on my fork/branch hydrian/timekpr-next-remote:UI-improvements

I believe I've gotten all of the UI & JS issues resolved. I'm getting some inconsistencies with the /get_usage rest call returning {"result":"fail","time_left":0,"time_spent":0} which returns a 'authentication error.' Some user are loaded successfully and others fail on the same server with the same service account user. The users that are successful and fail are different each page refresh.

I checked my auth.log logs on the target timekpr-next machine and the ssh logs are saying the service account user logged in successfully for all 3 of the queried users. The python server response gives the above failed response in the REST json HTTP request.

I suspect some type of race/timing condition is happening with the python server.

[mrjones-plip]

Yay! that's really great that you're into making a contribution to this project - thank you!

As well, thanks for sharing your WIP branch - looking good!

I have some early feedback:

• it'd be great to have a draft PR of this for easier early code review
• please break out the jquery upgrade into it's own PR, we can go ahead and merge that now after I do a bit of a smoke test
• generally speaking, please try and keep the PR as narrowly scoped as possible. The only goal should be to solve the improvement of two identically named users. This reduces the load needed for the review and encourages you to focus only on the task at hand. For example breaking out jquery to it's own PR (per above) and not changing intervals (pretty sure this is out of scope, but maybe it's important for your PR - lemme know!). I'd much rather work with you to merge 3 or 4 (or 5!) small PRs than merge one larger one that has semi or unrelated changes.

thanks again!

[mrjones-plip]

Oops! I see maybe you had some questions re "getting some inconsistencies with the /get_usage rest call" . Please give me steps to reproduce the issue on your branch and I'll try and help!

As well, please confirm the problem doesn't exist on main to ensure it's not a pre-existing bug.

[hydrian]

The problem with your main branch is that it has a JS issue with having the same username over multiple servers. That's why I created the UI-improvement branch. It was a two-fold fix to fix issue #8 and issue #7.

[hydrian]

Oops! I see maybe you had some questions re "getting some inconsistencies with the /get_usage rest call" . Please give me steps to reproduce the issue on your branch and I'll try and help!

This exists when you query multiple users on the same server in quick succession. When the index.html loads and fires off /get_usage ajax calls in a quick manner. Of my three users, usually only 1-2 of the user calls come back successful. On each page reload, the users that fail are different. If I check my system's auth log, all 3 of the ssh sessions made for each user are successful. This seems like a 'threading-like' issue.

[mrjones-plip]

The problem with your main branch is that it has a JS issue with having the same username over multiple servers. That's why I created the UI-improvement branch. It was a two-fold fix to fix issue #8 and issue #7.

wow! you're taking on two tickets - rockstar 😎

That said, checking on #7 I see that it's closed because there wasn't actually an issue with having identically named users on different boxes. Checking, I see there's good defense against this with the use of the rand_dom in the code. But maybe you've reproduced this bug in a different way that actually persists in current code? If yes, can you provide steps to reproduce? Maybe we can fix two bugs with one PR!

If you just didn't notice that #7 is closed, then let's tighten up the scope of this PR to just add the one UX feature.

[hydrian]

If I go to the /get_usage url directly with curl/web browser I get the same inconsistent results. My branch only UI changes, no python changes.

[hydrian]

Here is a sanitized version of my conf.py:

trackme = {
    'school2.mgmt.tygerclan.lan': ["morgan","brody","malcolm"],
    'school1.mgmt.tygerclan.lan': ["morgan","brody","malcolm"],
}
ssh_user = "srvs_timekpr-next-remote"
ssh_password = "somepassword"
ssh_timekpra_bin = '/usr/bin/timekpra'
ssh_key = "./id_rsa"

[hydrian]

Had to take a break this because I'm waiting on the bug to hit beta for testing: https://bugs.launchpad.net/timekpr-next/+bug/2061003

[mrjones-plip]

Thanks for the update! I'm in no rush, so take your time.

I took a closer look at the code and have some suggestions

• I still recommend we update jQuery in another PR, I see you're not even using it in your code ;)
• we don't actually need to introduce the concept of DOM IDs being computer + user + rand_dom. Having just user + rand_dom as it is today is fine - indeed rand_dom is meant to be random and thus unique!
• if we don't need computer, then we don't need the new escapeCSS() function. I believe that all linux usernames are valid CSS classes/IDs as well, so escapeCSS(user) isn't needed either
• we further don't need to update any other functions to specify the computer or the cleansed escapeCSS(computer) as all existing user + rand_dom should work as is. I did write this to be semi-future proof as is hopefully the case here!

looking at the code you wrote was really helpful! I see the need for another inner loop to iterate over each user. by adding just one more function, I think this is clean, has a limited amount of changes, but adds all the functionality.

        $.getJSON( "/config", function( data ) {
            $.each( data, function( computer, userList ) {
                $("#people").append('<h2 class="computer">' + computer + '</h2>');
                $.each( userList, function( userId, user ) {
                    let rand_dom = '_' + uuidv4();
                    // todo - click doesn't work and radio spans accross users'
                    $("#people").append(get_person_html(user, rand_dom, computer));

                    $('#' + user  + rand_dom + " .notify").show().html("Loading...");
                    $('#' + user + rand_dom + ' .btn').addClass("disabled");
                    $('#' + user  + rand_dom + ' .btn-check').addClass("disabled");

                    setInterval(function () {
                        $('#' + user  + rand_dom + " .loading").fadeIn(1300).fadeOut(1300);
                    }, 1400);
                    $.getJSON( "/get_usage/" + computer + "/" + user, function( usage ) {
                        render_user_to_dom(user, computer, usage, rand_dom);
                    });
                });
            });
        });

        function get_person_html(user, rand_dom, computer){
            let times = get_times(rand_dom);
            return "<span class='user' id='" + user + rand_dom + "'>" +
                "<p  class='h3'>" + user + "<span class='notify loading'></span></p>" +
                "<span class='time_wrapper'>" +
                    "Unused: <span class='time time_left'>...</span> " +
                    "Used: " +
                        "<span class='time time_spent'>...</span>" +
                "</span><br/>" +
                "<div class='btn-group' role='group' aria-label='Action'>" +
                    "<input type='radio'  class='btn-check' id='add" + rand_dom + "' value='add' name='action' />" +
                    "<label  class='btn btn-outline-primary' for='add" + rand_dom + "'>Add</label>" +
                    "<input type='radio'  class='btn-check' id='remove" + rand_dom + "' value='remove' name='action' />" +
                    "<label  class='btn btn-outline-primary' for='remove" + rand_dom + "'>Remove</label> " +
                "</div> " +
                "<div class='btn-group' role='group' aria-label='Times'>" +
                    times +
                "</div>" +
                "<div class='btn-group' role='group' aria-label='save'>" +
                    "<button class='btn btn-outline-primary save_me' " +
                        "value='save' user='" + user + "' computer='" + computer + "' >Save</button>" +
                "</div> " +
            "</span>"
        }

[hydrian]

The jquery update should be a different PR.

[hydrian]

That looks to be a cleaner implementation than mine.

[mrjones-plip]

Glad that code helps! If you're excited to see this PR through I'd be happy to have you revise yours and submit a PR. If you've lost interest I'd be happy to press ahead with what we've pieced together.

Again no rush, but let me know how you'd like to proceed!

[hydrian]

Sorry. Had to take a break frome this because I was working with/waiting for the timekpr-next project to fix bug in their software. I thought it may have been causing some of my issues here but it didn't fix the unconsistencies I'm getting with the REST call.

I think is my be an issue with the SSHd connectiins. It could be getting messed up with so many so fast. Ssshd maybe rate limiting the connects. I have an idea of how to fix this.