Test auteurs avec même email si _INTERDIRE_AUTEUR_MEME_EMAIL #4887

Open
opened 2 weeks ago by JLuc · 8 comments
JLuc commented 2 weeks ago

Quand il y a plusieurs auteurs avec le même email dans une base et qu'on définit _INTERDIRE_AUTEUR_MEME_EMAIL, actuellement formulaires_editer_auteur_verifier_dist introduit une dissymétrie dans le traitement auteurs :

  • la vérification du "1er" compte (dans l'ordre des réponses mysql) renvoie OK
  • mais la vérification d'un autre compte est rejetée.

Plutôt que ce résultat dissymétrique et que le double test en https://git.spip.net/spip/spip/src/branch/master/prive/formulaires/editer_auteur.php#L209 un simple test mysql produisant un résultat homogène serait préférable :

if (sql_countsel(
		'spip_auteurs',
		'email=' . sql_quote($email) . ' AND id_auteur != '.$id_auteur
	) > 0)
Quand il y a plusieurs auteurs avec le même email dans une base et qu'on définit `_INTERDIRE_AUTEUR_MEME_EMAIL`, actuellement `formulaires_editer_auteur_verifier_dist` introduit une dissymétrie dans le traitement auteurs : - la vérification du "1er" compte (dans l'ordre des réponses mysql) renvoie OK - mais la vérification d'un autre compte est rejetée. Plutôt que ce résultat dissymétrique et que le double test en https://git.spip.net/spip/spip/src/branch/master/prive/formulaires/editer_auteur.php#L209 un simple test mysql produisant un résultat homogène serait préférable : ``` if (sql_countsel( 'spip_auteurs', 'email=' . sql_quote($email) . ' AND id_auteur != '.$id_auteur ) > 0) ```
Poster

Mais par ailleurs, je me demande s'il est juste de verifier cela ici, car cela empêche de modifier l'un des comptes concernés (sauf le "1er" lol) lorsqu'il y a plusieurs comptes avec le même mail (et alors que c'est possible lorsque la constante n'est pas définie). Un commentaire précise que « cette fonctionalité nécessite que la base soit clean à l'activation : pas de doublon sur la requête select email,count(*) from spip_auteurs group by email ; » mais pourquoi cette restriction ?

Il serait préférable, me semble t il, qu'il n'y ait pas cette restriction et que la constante empêche la création d'un nouveau compte avec un mail déjà utilisé seulement, mais pas la modification de ces comptes s'il en existe déjà car s'ils sont là il y a peut être une raison et de toute façon la constante n'y peut rien.

Il faudrait pour cela supprimer le commentaire et tout le 'else' qui contient ce test.

Rq : Dans ma situation, ce sont plusieurs comptes admins qui partagent le même mail by design, et je voudrais empêcher que les utilisateurs non-admins se créent plusieurs compte à la suite (comme ça leur arrive parfois de le faire quand il n'y a pas la constante).

Mais par ailleurs, je me demande s'il est juste de verifier cela ici, car cela empêche de modifier l'un des comptes concernés (sauf le "1er" lol) lorsqu'il y a plusieurs comptes avec le même mail (et alors que c'est possible lorsque la constante n'est pas définie). [Un commentaire](https://git.spip.net/spip/spip/src/branch/master/prive/formulaires/editer_auteur.php#L197) précise que « cette fonctionalité nécessite que la base soit clean à l'activation : pas de doublon sur la requête `select email,count(*) from spip_auteurs group by email ;` » mais pourquoi cette restriction ? Il serait préférable, me semble t il, qu'il n'y ait pas cette restriction et que la constante empêche la *création* d'un nouveau compte avec un mail déjà utilisé seulement, mais pas la modification de ces comptes s'il en existe déjà car s'ils sont là il y a peut être une raison et de toute façon la constante n'y peut rien. Il faudrait pour cela supprimer le commentaire et [tout le 'else' qui contient ce test](https://git.spip.net/spip/spip/src/branch/master/prive/formulaires/editer_auteur.php#L209). Rq : Dans ma situation, ce sont plusieurs comptes admins qui partagent le même mail by design, et je voudrais empêcher que les utilisateurs non-admins se créent plusieurs compte à la suite (comme ça leur arrive parfois de le faire quand il n'y a pas la constante).
b_b commented 2 weeks ago
Owner

Mais par ailleurs, je me demande s'il est juste de verifier cela ici, car cela empêche de modifier l'un des comptes concernés (sauf le "1er" lol) lorsqu'il y a plusieurs comptes avec le même mai

Ça me semble logique, tu peux modifier, et donc supprimer le doublon, pour le deuxième compte, qui est donc arrivé après le premier. Non ?

> Mais par ailleurs, je me demande s'il est juste de verifier cela ici, car cela empêche de modifier l'un des comptes concernés (sauf le "1er" lol) lorsqu'il y a plusieurs comptes avec le même mai Ça me semble logique, tu peux modifier, et donc supprimer le doublon, pour le deuxième compte, qui est donc arrivé après le premier. Non ?
Owner

et quand la constante est activée, tu ne veux pas que la modification d'un compte entraine la création d'un nouveau doublon, il faut donc bien une vérification à la modification des auteurs

et quand la constante est activée, tu ne veux pas que la modification d'un compte entraine la création d'un nouveau doublon, il faut donc bien une vérification à la modification des auteurs
b_b commented 2 weeks ago
Owner

Donc on ferme ?

Donc on ferme ?
Poster

@b_b : le morceau de code cité ici n'est pas concerné par le dédoublonnage car il teste le nouvel email. Que la constante soit définie ou pas, et que l'email précédent soit un doublon ou pas, on peut toujours modifier un compte existant en lui affectant un nouvel email qui n'est pas déjà utilisé.

@cerdic : en effet, on ne veut pas qu'une modification provoque un doublon d'email. On peut alors mieux cerner afin de ne pas interdire la modification d'un compte déjà doublonné = interdire que l'email soit un doublon uniquement lorsque ce n'était pas déjà un doublon avant.

Pour mieux cibler, le else devient alors :

else {
  # Changement d'email vers un email déjà attribué à un autre auteur ?
  $email_ancien = sql_getfetsel('email', 'spip_auteurs', 'id_auteur=' . intval($id_auteur));
  if (
    $email != $email_ancien
    and (sql_countsel('spip_auteurs', 'email=' . sql_quote($email)) > 0)
  ) {
    $erreurs['email'] = _T('erreur_email_deja_existant');
  }
}

Outre la possibilité de gérer des comptes doublons pré-existants, cette modif réétablit l'homogénéité des comportements des différents comptes doublons. Et ça permet de gérer utilement la situation dont le commentaire dit qu'elle n'est pas gérée.

EDIT : On peut vouloir réutiliser l'email d'un auteur à la poubelle donc il ne faut pas les compter :
sql_countsel('spip_auteurs', 'email=' . sql_quote($email) . " AND statut != '5poubelle'") > 0

@b_b : le morceau de code cité ici n'est pas concerné par le dédoublonnage car il teste le *nouvel* email. Que la constante soit définie ou pas, et que l'email précédent soit un doublon ou pas, on peut toujours modifier un compte existant en lui affectant un nouvel email qui n'est pas déjà utilisé. @cerdic : en effet, on ne veut pas qu'une modification provoque un doublon d'email. On peut alors mieux cerner afin de ne pas interdire la modification d'un compte déjà doublonné = interdire que l'email soit un doublon uniquement lorsque ce n'était *pas déjà* un doublon avant. Pour mieux cibler, le `else` devient alors : ``` else { # Changement d'email vers un email déjà attribué à un autre auteur ? $email_ancien = sql_getfetsel('email', 'spip_auteurs', 'id_auteur=' . intval($id_auteur)); if ( $email != $email_ancien and (sql_countsel('spip_auteurs', 'email=' . sql_quote($email)) > 0) ) { $erreurs['email'] = _T('erreur_email_deja_existant'); } } ``` Outre la possibilité de gérer des comptes doublons pré-existants, cette modif réétablit l'homogénéité des comportements des différents comptes doublons. Et ça permet de gérer utilement la situation dont le commentaire dit qu'elle n'est pas gérée. *EDIT* : On peut vouloir réutiliser l'email d'un auteur à la poubelle donc il ne faut pas les compter : `sql_countsel('spip_auteurs', 'email=' . sql_quote($email) . " AND statut != '5poubelle'") > 0`
b_b added the
amélioration
label 2 weeks ago
Owner

Non non et non JLuc : si on active cette constante c'est pour ne pas avoir de doublon en base.

Donc si il y avait des doublons existants c'est déjà problématique, et il est légitime que lors de toute tentative de modification on en profite pour dire "hola toi, change ton email qui ne saurait rester un doublon"

Et le commentaire dit explicitement

# Ne pas autoriser d'avoir deux auteurs avec le même email
# cette fonctionalité nécessite que la base soit clean à l'activation : pas de
# doublon sur la requête select email,count(*) from spip_auteurs group by email ;

Donc on ne va pas introduire une dérogation pour conserver les doublons, alors que justement c'est une fonctionnalité pour les interdire

Non non et non JLuc : si on active cette constante c'est pour *ne pas avoir de doublon* en base. Donc si il y avait des doublons existants c'est déjà problématique, et il est légitime que lors de toute tentative de modification on en profite pour dire "hola toi, change ton email qui ne saurait rester un doublon" Et le commentaire dit explicitement ``` # Ne pas autoriser d'avoir deux auteurs avec le même email # cette fonctionalité nécessite que la base soit clean à l'activation : pas de # doublon sur la requête select email,count(*) from spip_auteurs group by email ; ``` Donc on ne va pas introduire une dérogation pour conserver les doublons, alors que justement c'est une fonctionnalité pour les interdire
Poster

Ce n'est pas une dérogation mais un complément de spécification, car 1) ce commentaire indique que la constante doit être utilisée seulement lorsqu'il n'y a pas encore de doublons, et 2) ce ticket propose la spéc et l'implémentation de ce qu'elle fait dans le cas contraire, quand il y a déjà des doublons. C'est complémentaire et pas dérogatoire.

Pour le signalement des doublons problématiques, il serait mieux de le faire sans attendre qu'on demande à enregistrer une modification : le faire dès l'affichage du compte, voire même en introduction de la liste des auteurs ou visiteurs.

PS : En attendant, j'ai défini une fonction test_inscription() pour gérer le besoin.

Ce n'est pas une dérogation mais un complément de spécification, car 1) ce commentaire indique que la constante doit être utilisée seulement lorsqu'il n'y a pas encore de doublons, et 2) ce ticket propose la spéc et l'implémentation de ce qu'elle fait dans le cas contraire, quand il y a déjà des doublons. C'est complémentaire et pas dérogatoire. Pour le signalement des doublons problématiques, il serait mieux de le faire sans attendre qu'on demande à enregistrer une modification : le faire dès l'affichage du compte, voire même en introduction de la liste des auteurs ou visiteurs. PS : En attendant, j'ai défini une fonction test_inscription() pour gérer le besoin.
JLuc commented 1 week ago
Poster

Voici mieux d'explications.

En SPIP 4.0

  • la constante est prévue uniquement pour le cas où il n'y a pas de doublon dans la BDD au moment où on la définit (cf le commentaire cité plus haut). Son effet est d'empêcher la création de doublons d'email dans les comptes auteurs.
  • Mais rien empêche de la définir alors qu'il y a déjà des doublons dans la BDD. Et dans le cas ou on définit la constante alors qu'il y a déjà des doublons dans la BDD, le comportement est foireux. Le principal problème est que le comportement est différent selon le compte dont l'email est doublon qu'on veut modifier : parfois la modification est acceptée, parfois pas, sans raison "éditoriale" valable puisque ça dépend de spécificités d'implémentations internes à MYSQL.

Avec le fix proposé :

  • le comportement est identique dans le cas prévu par la constante en SPIP 4.0 = quand il n'y a pas de doublon d'emails d'auteurs en BDD. C'est à dire qu'on ne peut PAS créer de nouveaux comptes doublonnant un email déjà existant.
    Les sites utilisant la constante dans les conditions nécessaires indiquées ne voient aucune différence.
  • NEW : on peut maintenant définir la constante aussi lorsqu'il y a déjà des doublons d'emails dans la BDD.
    • Dans ce cas, on ne peut pas créer un nouveau doublon d'email
    • Mais on peut éditer les données d'un compte dont le mail est doublonné, et gérer au mieux la situation selon ce qui est utile, soit en dédoublonnant le mail, soit sans changer le mail si c'est intentionnel.

Globalement et quelle que soit la situation de la BDD, la définition de la constante c'est qu'elle INTERDIT LA CRÉATION DE DOUBLONS. La nouveauté avec le fix est qu'elle peut s'appliquer aussi lorsqu'il y a des comptes doublons au moment de sa définition, et qu'alors elle laisse la possibilité de les gérer au mieux.

Voici mieux d'explications. En SPIP 4.0 - la constante est prévue uniquement pour le cas où il n'y a pas de doublon dans la BDD au moment où on la définit (cf le commentaire cité plus haut). Son effet est d'empêcher la création de doublons d'email dans les comptes auteurs. - Mais rien empêche de la définir alors qu'il y a déjà des doublons dans la BDD. Et dans le cas ou on définit la constante alors qu'il y a déjà des doublons dans la BDD, le comportement est foireux. Le principal problème est que le comportement est différent selon le compte dont l'email est doublon qu'on veut modifier : parfois la modification est acceptée, parfois pas, sans raison "éditoriale" valable puisque ça dépend de spécificités d'implémentations internes à MYSQL. Avec le fix proposé : - le comportement est identique dans le cas prévu par la constante en SPIP 4.0 = quand il n'y a pas de doublon d'emails d'auteurs en BDD. C'est à dire qu'on ne peut PAS créer de nouveaux comptes doublonnant un email déjà existant. Les sites utilisant la constante dans les conditions nécessaires indiquées ne voient aucune différence. - NEW : on peut maintenant définir la constante *aussi* lorsqu'il y a déjà des doublons d'emails dans la BDD. * Dans ce cas, on ne peut pas créer un nouveau doublon d'email * Mais on peut éditer les données d'un compte dont le mail est doublonné, et gérer au mieux la situation selon ce qui est utile, soit en dédoublonnant le mail, soit sans changer le mail si c'est intentionnel. Globalement et quelle que soit la situation de la BDD, la définition de la constante c'est qu'elle INTERDIT LA CRÉATION DE DOUBLONS. La nouveauté avec le fix est qu'elle peut s'appliquer aussi lorsqu'il y a des comptes doublons au moment de sa définition, et qu'alors elle laisse la possibilité de les gérer au mieux.
Sign in to join this conversation.
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.