entrouvert/wcs
entrouvert
/
wcs
14
1
Fork 0
Code Issues Pull Requests 34 Releases Activity

misc: limit /live calls (#83905) #867

Merged
fpeters merged 1 commits from wip/83905-limit-live-calls-on-errors into main 2023-11-28 12:23:49 +01:00
Conversation 0 Commits 1 Files Changed 7 +102 -78
fpeters commented 2023-11-24 12:57:13 +01:00
Owner
Copy Link
No description provided.
fpeters reviewed 2023-11-24 12:57:55 +01:00
wcs/qommon/static/js/qommon.forms.js Outdated
@ -501,1 +503,4 @@
$('form[data-live-url]').on('wcs:change', function(ev, data) {
if (live_evaluation_pause_timeout_id) {
return
fpeters commented 2023-11-24 12:57:30 +01:00
Author
Owner
Copy Link

Si jamais on a une pause en cours, on ne fait rien.

Si jamais on a une pause en cours, on ne fait rien.
wcs/qommon/static/js/qommon.forms.js Outdated
@ -690,0 +703,4 @@
live_evaluation_error_count++
live_evaluation_pause_timeout_id = setTimeout(
() => { live_evaluation_pause_timeout_id = null },
live_evaluation_error_count * 1000
fpeters commented 2023-11-24 12:57:52 +01:00
Author
Owner
Copy Link

S'il y a une erreur on force une pause, de plus en plus longue.

S'il y a une erreur on force une pause, de plus en plus longue.
fpeters commented 2023-11-24 13:54:17 +01:00
Author
Owner
Copy Link

De manière inattendue mais finalement heureuse (je pense), s'il y a un .abort() d'un appel, parce que nouvel événement, ça va aussi entrer dans la gestion d'erreur, et on va donc gagner un rate-limiting aussi sur ce cas.

De manière inattendue mais finalement heureuse (je pense), s'il y a un .abort() d'un appel, parce que nouvel événement, ça va aussi entrer dans la gestion d'erreur, et on va donc gagner un rate-limiting aussi sur ce cas.
tnoel commented 2023-11-24 17:46:46 +01:00
Owner
Copy Link

Question que je me pose : le « abort() » va arriver aussi dans des moments normaux. Typiquement sur un /live un peu lent, qui prendrait une ou deux secondes à être calculé... c'est un peu lent mais pas bogué. Or si l'usager tape tranquillement, on va avoir pas mal de abort() au fur et à mesure de la saisie... et j'ai l'impression qu'on pourrait se retrouver sur une pause indue... (oui, j'ai un peu de mal à lire ce code)

Question que je me pose : le « abort() » va arriver aussi dans des moments normaux. Typiquement sur un /live un peu lent, qui prendrait une ou deux secondes à être calculé... c'est un peu lent mais pas bogué. Or si l'usager tape tranquillement, on va avoir pas mal de abort() au fur et à mesure de la saisie... et j'ai l'impression qu'on pourrait se retrouver sur une pause indue... (oui, j'ai un peu de mal à lire ce code)
tnoel requested changes 2023-11-24 17:47:23 +01:00
Dismissed
tnoel left a comment
Owner
Copy Link

Pas vraiment une demande de modif du patch, mais un peu plus de demande d'explication, peut-être de commentaires dans le code pour se rappeler la mécanique.

Pas vraiment une demande de modif du patch, mais un peu plus de demande d'explication, peut-être de commentaires dans le code pour se rappeler la mécanique.
wcs/qommon/static/js/qommon.forms.js Outdated
@ -502,2 +507,4 @@
}
if (live_evaluation) {
live_evaluation.abort();
tnoel commented 2023-11-24 17:40:58 +01:00
Owner
Copy Link

De ce que je comprends de l'explication donnée plus bas sur la section « error: » de l'appel ajax, ce abort() va partir sur cette section (j'ai bien compris ?). Si c'est bien ça je verrais bien un commentaire qui le précise ici, parce que perso ça me parait absolument pas évident (je veux dire, je veux bien croire que ça marche, mais je ne m'y attendais pas).

De ce que je comprends de l'explication donnée plus bas sur la section « error: » de l'appel ajax, ce abort() va partir sur cette section (j'ai bien compris ?). Si c'est bien ça je verrais bien un commentaire qui le précise ici, parce que perso ça me parait absolument pas évident (je veux dire, je veux bien croire que ça marche, mais je ne m'y attendais pas).
fpeters force-pushed wip/83905-limit-live-calls-on-errors from 14d7243b18 to a29e59837b 2023-11-25 15:02:49 +01:00 Compare
fpeters reviewed 2023-11-25 15:52:23 +01:00
wcs/forms/common.py Outdated
@ -820,24 +820,24 @@ class FormStatusPage(Directory, FormTemplateMixin):
for field in displayed_fields:
result[field.id] = {'visible': field.is_visible(formdata.data, formdata.formdef)}
modified_field_ids = get_request().form.get('modified_field_id[]') or []
fpeters commented 2023-11-25 15:48:14 +01:00
Author
Owner
Copy Link

Une approche totalement différente, qui s'éloigne du ticket, on ne prête plus d'attention particulière aux erreurs.

Ici côté serveur on change les choses pour accepter une liste de champs modifiés.

Une approche totalement différente, qui s'éloigne du ticket, on ne prête plus d'attention particulière aux erreurs. Ici côté serveur on change les choses pour accepter une liste de champs modifiés.
wcs/qommon/static/js/qommon.forms.js Outdated
@ -506,4 +504,0 @@
if (data && data.modified_field) {
new_data += '&modified_field_id=' + data.modified_field;
if (data.modified_block) new_data += '&modified_block_id=' + data.modified_block;
if (data.modified_block_row) new_data += '&modified_block_row=' + data.modified_block_row;
fpeters commented 2023-11-25 15:52:13 +01:00
Author
Owner
Copy Link

Ces paramètres de bloc ne seront pas repris, ils ne sont pas utilisés côté serveur.

Ces paramètres de bloc ne seront pas repris, ils ne sont pas utilisés côté serveur.
wcs/qommon/static/js/qommon.forms.js Outdated
@ -510,0 +505,4 @@
var $form = $(this)
if (data.modified_field) {
live_evaluation_params[data.modified_field] = true
}
fpeters commented 2023-11-25 15:49:08 +01:00
Author
Owner
Copy Link

Lors d'une modification, on alimente la variable live_evaluation_params pour garder trace de ce qui a changé.

Lors d'une modification, on alimente la variable live_evaluation_params pour garder trace de ce qui a changé.
wcs/qommon/static/js/qommon.forms.js Outdated
@ -510,0 +508,4 @@
}
if (live_evaluation === null) {
// no delay for first call
call_live_url($form)
fpeters commented 2023-11-25 15:49:25 +01:00
Author
Owner
Copy Link

Si jamais c'est le tout début, on fait l'appel à /live.

Si jamais c'est le tout début, on fait l'appel à /live.
wcs/qommon/static/js/qommon.forms.js Outdated
@ -510,0 +512,4 @@
} else {
if (live_evaluation && live_evaluation.readyState != 4 /* done */ ) {
// live call being processed, delay this one
timeout = 500
fpeters commented 2023-11-25 15:49:44 +01:00
Author
Owner
Copy Link

Sinon, s'il y a déjà un appel en cours, on reporte d'une demi-seconde.

Sinon, s'il y a déjà un appel en cours, on reporte d'une demi-seconde.
wcs/qommon/static/js/qommon.forms.js Outdated
@ -510,0 +515,4 @@
timeout = 500
} else {
// always call after mini-delay
timeout = 150
fpeters commented 2023-11-25 15:50:12 +01:00
Author
Owner
Copy Link

Et s'il n'y a pas d'appel en cours, on reporte quand même, de 150ms.

Et s'il n'y a pas d'appel en cours, on reporte quand même, de 150ms.
wcs/qommon/static/js/qommon.forms.js Outdated
@ -510,0 +520,4 @@
if (live_evaluation_delay_id) {
clearTimeout(live_evaluation_delay_id)
}
live_evaluation_delay_id = setTimeout(() => { call_live_url($form) }, timeout)
fpeters commented 2023-11-25 15:51:05 +01:00
Author
Owner
Copy Link

Une fois le timeout expiré (et ceux-ci peuvent se succéder en cas d'appels successifs), on envoie alors vers le serveur.

Une fois le timeout expiré (et ceux-ci peuvent se succéder en cas d'appels successifs), on envoie alors vers le serveur.
wcs/qommon/static/js/qommon.forms.js Outdated
@ -510,1 +530,4 @@
for (let modified_field_id of Object.keys(live_evaluation_params)) {
new_data += '&modified_field_id[]=' + modified_field_id
}
live_evaluation_params = Object() // reset
fpeters commented 2023-11-25 15:51:38 +01:00
Author
Owner
Copy Link

Pour l'appel, on alimente des paramètres modified_field_id[], pour que ça soit bien tout le temps traité comme une liste.

Pour l'appel, on alimente des paramètres modified_field_id[], pour que ça soit bien tout le temps traité comme une liste.
fpeters dismissed tnoel’s review 2023-11-25 15:52:36 +01:00
Reason:

nouvelle approche

fpeters changed title from misc: limit /live calls on errors (#83905) to misc: limit /live calls (#83905) 2023-11-25 15:53:13 +01:00
pducroquet approved these changes 2023-11-27 13:43:32 +01:00
pducroquet left a comment
Owner
Copy Link

Ça me semble bon

Ça me semble bon
fpeters force-pushed wip/83905-limit-live-calls-on-errors from a29e59837b to 94ad344364 2023-11-27 13:46:39 +01:00 Compare
fpeters merged commit 2dc915671b into main 2023-11-28 12:23:49 +01:00
fpeters deleted branch wip/83905-limit-live-calls-on-errors 2023-11-28 12:23:49 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
pducroquet
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/wcs#867
No description provided.
Delete Branch "wip/83905-limit-live-calls-on-errors"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?