Replacing jQuery to Vanilla JavaScript

[peregrineshahin]

Now that we don't need it in Bootstrap 5

[ppigazzini]

DEV updated.

application.js:

  const tasksCollapse = new bootstrap.Collapse(tasks, {
      toggle: true,
    });

[peregrineshahin]

Refactored all jQuery from the templates, and fixed bootstrap collapse instance,
I think this is ready to be tested again, EDIT: (not really)

[ppigazzini]

DEV updated.

$ git log --oneline -n 15
fc8bcb6 (HEAD -> master) Merge commit 'refs/pull/1388/head' of https://github.com/glinscott/fishtest
3646fd1 Merge branch 'task_duration_180' of https://github.com/ppigazzini/fishtest
1ea774e missing parentheses
2a32868 fix typo in document.head
b4831c7 Update tests_view.mak
3f8f595 Bootstrap 5 collapse instance
28217ad Update tests_run.mak
7d780a8 Update base.mak
b66085f Update actions.mak
0317aaa convert jQ to JS
917a17c Replace legacy `var` with ES6 `let` and `const`
d54aa7a Convert jQuery to JavaScript
423f4c1 (origin/master, origin/HEAD) Update forms Update events log form to bootstrap 5 Display the selected values after submitting in events log Make SPRT forms mobile friendly Update action buttons and forms in test view Update CSS forms for dark theme
7303459 Raise worker version to 175 (also server side)
e198edb Use ZipFile context manager

Not working:

• all show/hide buttons (it works only for workers but after a browser refresh, not with a clean start)
• diff missing in test page
• theme switch
• login "check_csrf_token(): Invalid token"

Solved:

• SPSA graph

Working:

• column content sorting
• SPRT calc
• liveElo graph (not tested yet the live updated)
• show/hide default nets

[peregrineshahin]

seems like one problem in the bootstrap collapse instance is making the script stop resulting is other problems,
and seems like I'm assigning a bootstrap instance to the divs target before they are loaded;

will have a look on how I can solve this

[ppigazzini]

Take your time, I know very little javascript so my help is limited to testing the PR.

[ppigazzini]

DEV updated.

[ppigazzini]

This throws error when not in homepage:

  const machinesButton = document.querySelector("#machines-button");
  machinesButton?.addEventListener("click", (e) => {

This throws error when in homepage:

  const tasksButton = document.querySelector("#tasks-button");
  tasksButton?.addEventListener("click", (e) => {

Commenting out the second one, and loading the homepage, the theme switch works fine.

[ppigazzini]

DEV updated, the collapse.js error in console seems disappeared in every page.

In a test page (eg https://dfts-0.pigazzini.it/tests/view/62e0ba678e4fa6ae4726947e) there are 2 SyntaxErrors:

[ppigazzini]

Login works now!
(use "user01"/"user01" to login as normal user and "user00"/"user00" as admin)
Error in the new test page.

[ppigazzini]

DEV updated, I was able to submit a new SPRT test.
However, I was not able to change the type of test (eg NumGames, SPSA) or TC.

[ppigazzini]

DEV updated.
https://dfts-0.pigazzini.it/tests/view/62e121e18e4fa6ae4726a43e

[ppigazzini]

DEV updated. In homepage the show/hide buttons are changing status, to load the table require a F5, though.

[peregrineshahin]

I will try to fix that lastly as I'm thinking about using CSS classes and not using this bootstrap instance instead, will see,

there are still some errors I still see jQuery even in the last commit and there are a couple of syntax errors,

[ppigazzini]

I'm thinking about this PR as a marathon, not a 100m dash. There are so much things to be well tested before the merge. Take your time.

[ppigazzini]

DEV updated.
2 Uncaught SyntaxError in console for a test page.

[ppigazzini]

DEV updated.

[ppigazzini]

DEV updated

[ppigazzini]

Updated

[ppigazzini]

Updated and added by hand in tests.mak, tests_view.mak:

<%
from fishtest.util import get_cookie
%>

All show/hide buttons seems to work fine in a fast test.

EDIT_000: all the show/hide button (but the "workers" one) have the label reversed (eg "show" when the table is already shown)

[ppigazzini]

I'm a bit confused to why we even need this in here

The code for show/hide buttons was at first wrote in application.js, see the version from 18 months ago,
https://github.com/glinscott/fishtest/blame/27bb5700ae26d0e95c1d3d61dcb96078d2eaac5b/server/fishtest/static/js/application.js

Please mind that the function get_cookie() was added as workaround for a bug with chrome, see
fdee74e

[ppigazzini]

Updated.
In tests_view.mak the import of get_cookie can be simplified in this way:

<%
from fishtest.util import get_cookie, worker_name

if 'spsa' in run['args']:
  import json
  spsa_data = json.dumps(run["args"]["spsa"])
%>

[ppigazzini]

Pyramid works in this way.
In views.py we set the mako template to be used to render the page and the variables passed to that template, eg

https://github.com/glinscott/fishtest/blob/7a732ef837612cca4113540e4a4dfcdd8d2ff434/server/fishtest/views.py#L1294-L1325

Please mind the last lines, where we read a cookie to set a variable used in the mako template.

Here is some documentation about pyramid request, views etc.

https://docs.pylonsproject.org/projects/pyramid/en/latest/api/request.html#request-module
https://docs.pylonsproject.org/projects/pyramid/en/latest/glossary.html#term-request
https://docs.pylonsproject.org/projects/pyramid/en/latest/narr/views.html

Here is some doc about mako template (default template in Pyramid):
https://www.makotemplates.org/
https://docs.makotemplates.org/en/latest/

To inspect the variables content in a page simply add a field in the mako template:

<div>
  some_variable = ${some_variable} 
</div>

EDIT_000: in a mako template we can set a javascript variable using the python variable, eg:
https://github.com/glinscott/fishtest/blob/7a732ef837612cca4113540e4a4dfcdd8d2ff434/server/fishtest/templates/base.mak#L11-L13
https://github.com/glinscott/fishtest/blob/7a732ef837612cca4113540e4a4dfcdd8d2ff434/server/fishtest/templates/tests_view.mak#L13-L21

[ppigazzini]

DEV updated.

[dav1312]

Instead of using querySelector() for IDs why not just use getElementById() ?

[ppigazzini]

DEV updated.

[ppigazzini]

Updated:

• non default nns cookie works fine
• diff in test page is now broken Uncaught SyntaxError: Unexpected token '}' (at 62e1b66c8e4fa6ae4726bbfe:2247:3)

I don't recall someone reporting the rate limit message in the diff window (hard to be ignored because the long one line message pushes the "Actions", "Stats", "Time" fields in the far right). I triggered the rate limit because I created a very high number of new runs to test the vanilla js code.
If a proper solution is hard to be coded or is against the best practice, for me me is fine to revert 517999b

[dav1312]

What about checking if the user has reached its rate limit (or that it has enough left) before making a request? https://api.github.com/rate_limit

[ppigazzini]

What about checking if the user has reached its rate limit before making a request? https://api.github.com/rate_limit

Nice idea but IMO better to no add new features to this PR (BTW I started requesting new features so I'm to blame here :) )

[peregrineshahin]

Please notice that the jQuery Ajax solution(master) also has the same problem of 403 (the rate limit problem)with diff box, I have been experiencing it since a very long time, when you refresh enough times on a particular test diff doesn't show anymore,
if you tried the same test on the master you will also get the same error.

[ppigazzini]

I think to have hit a bug that IMO already affects master.

I do know, I wrote it in my old post.

[peregrineshahin]

I think this should make it equivalent to master
didn't notice at first that there are nested fetching in the code

[ppigazzini]

DEV Updated. Looks good to me!

PROD updated by hand to test the PR with the sharks :)

[ppigazzini]

I will run PROD with this PR for few days to collects feedbacks from the users.

@peregrineshahin please at your convenient time squash the commits or in one commit or in few logical commits, writing a/some final commit message/s (I use git rebase -i master to do the work, ymmv)

EDIT_000: we auto format the front end code with prettier, see #634 (comment):

npx prettier --write "server/fishtest/static/{css/*.css,html/*.html,js/*[!highlight.diff.min].js}"

I can do it during the merge, let me know.

[vdbergh]

Very substantial PR!

[ppigazzini]

DEV updated, I submitted 11 tests in a row using the back button, it seems to work (with the previous code I got a random true/false value for form_submitted when pushing the back button).
PROD updated by hand to have a bigger number of testers.

[peregrineshahin]

Just for the record, I don't think you get random as garbage memory random before the fix, I think the randomity comes from if the back button triggered a full reload or rendered the cached page and that depends on how fast you click it - it seems (bc I'm still not able to reproduce it)-

please @ppigazzini feel free to format the PR with Prettier when merging, and I think I can squash the commits after learning how git commands, tonight or tomorrow

[ppigazzini]

@peregrineshahin squashing to only one commit it's very easy, change all the "pick" to "squash" but the first one.

git checkout jQ-to-JS
git checkout -b jQ-to-JS-backup
git checkout master
git fetch -p upstream
git reset --hard upstream master
git checkout jQ-to-JS
git rebase -i master
# change all the "pick" to "squash" but the commits that you want to keep, save and exit
# write the final commit messages for the commits, save and exit
# if a commit doesn't depend on previous ones, you can change the order moving the line
# you can rebase several times to make little refactoring of the commits
git push -f origin jQ-to-JS

[ppigazzini]

I test the code on PROD for a few more hours to collect the users feedback.

[ppigazzini]

PROD and DEV updated.

[ppigazzini]

Merged, PROD updated, thank you @peregrineshahin :)