Project

General

Profile

Bug #31222

Lien invalide sur la liste des rôles d'un utilisateur

Added by Valentin Deniaud 3 months ago. Updated 3 months ago.

Status:
Solution déployée
Priority:
Normal
Category:
-
Target version:
-
Start date:
08 Mar 2019
Due date:
% Done:

0%

Patch proposed:
Yes
Planning:
No

Description

Par exemple sur /manage/users/<user_id>/roles/ si on clique sur une ligne (vraiment la ligne, pas le nom du rôle, ie le <tr>, pas le <td>) on arrive sur /manage/users/<user_id>/roles/<role_id> qui n'existe pas. On devrait tomber sur /manage/roles/<role_id>.
En fait il y a juste une typo dans le code.

0001-manager-access-url_name-through-table-object-31222.patch View (1.66 KB) Valentin Deniaud, 08 Mar 2019 03:49 PM

0001-manager-access-context-through-table-object-bis-3122.patch View (8.44 KB) Valentin Deniaud, 11 Mar 2019 04:37 PM

0001-manager-access-context-through-table-object-bis-3122.patch View (8.44 KB) Valentin Deniaud, 14 Mar 2019 11:52 AM

0002-manager-remove-some-unused-code-31222.patch View (4.66 KB) Valentin Deniaud, 14 Mar 2019 11:52 AM

0003-manager-remove-some-unused-code-31222.patch View (5.71 KB) Valentin Deniaud, 14 Mar 2019 12:08 PM

0001-manager-access-context-through-table-object-bis-3122.patch View (6.73 KB) Valentin Deniaud, 14 Mar 2019 12:08 PM

0002-manager-update-translations-31222.patch View (2.04 KB) Valentin Deniaud, 14 Mar 2019 12:08 PM

Associated revisions

Revision df8cc6df (diff)
Added by Valentin Deniaud 3 months ago

manager: access context through table object bis (#31222)

Related to #31092.

Revision 26dd7e67 (diff)
Added by Valentin Deniaud 3 months ago

manager: update translations (#31222)

Revision 14361e00 (diff)
Added by Valentin Deniaud 3 months ago

manager: remove some unused code (#31222)

History

#1 Updated by Valentin Deniaud 3 months ago

  • File 0001-fix-invalid-url-on-clickable-row.patch added
  • Tracker changed from Support to Bug
  • Status changed from Nouveau to Solution proposée
  • Patch proposed changed from No to Yes

#2 Updated by Paul Marillonnet 3 months ago

Je crois que tu peux t'inspirer de #31092 pour ajouter une ligne de test, aussi.

#3 Updated by Benjamin Dauvergne 3 months ago

Il faut peut-être faire le tour de tous les templates de tableau à l'occasion puisque c'est le même souci que #31092 et pas une typo.

#4 Updated by Valentin Deniaud 3 months ago

  • File deleted (0001-fix-invalid-url-on-clickable-row.patch)

#5 Updated by Valentin Deniaud 3 months ago

J'ai ajouté un test et modifié le message de commit, puisque je n'avais pas compris d'où venait le bug initialement.

#6 Updated by Benjamin Dauvergne 3 months ago

Je vois active_role dans src/authentic2/manager/templates/authentic2/manager/role_members_table.html qui a le même problème je pense, liste des templates à regarder :

$ git grep -l block\ table -- \*.html
src/authentic2/manager/templates/authentic2/manager/ous_table.html
src/authentic2/manager/templates/authentic2/manager/role_children_table.html
src/authentic2/manager/templates/authentic2/manager/role_members_table.html
src/authentic2/manager/templates/authentic2/manager/role_permissions_table.html
src/authentic2/manager/templates/authentic2/manager/service_roles_table.html
src/authentic2/manager/templates/authentic2/manager/table.html
src/authentic2/manager/templates/authentic2/manager/user_roles_table.html

#7 Updated by Valentin Deniaud 3 months ago

Bon j'ai passé un certain temps à comprendre comment était structuré le code et à découvrir la magie de django au niveau du contexte des templates.

Sauf si il y a une couche de sorcellerie que j'ai raté, le paramètre active_role que tu mentionnes n'est jamais utilisé (git grep active_role ne renvoie rien hors des templates, et j'ai l'impression qu'il a été retiré en 2015 par 25ad9166a1a0367c659e858346db7f9b8b7fa3f3).
À chaque fois, il est utilisé comme suit :

{% blocktrans with name=row.record.name role=active_role.name %}Do you really want to remove role &quot;{{ name }}&quot; from role &quot;{{ object }}&quot;&nbsp;?{% endblocktrans %}

Donc j'ai l'impression qu'il y a un genre de typo, par ex {{ object }} devrait être {{ role }} (mais à ce moment là ça ne marche plus, parce que active_role n'est jamais passé).
Bref, c'est cassé de toute façon avec la version de django-tables2 qui nous préoccupe, parce que object n'existe plus et il faut aller le chercher dans table.context.object.
Et pour une raison mystérieuse, table.context.object n'est pas accessible à l'intérieur du bloc blocktrans.
La solution que je propose est donc de faire :
{% blocktrans with name=row.record.name role=table.context.object.name %}Do you really want to remove role &quot;{{ name }}&quot; from role &quot;{{ role }}&quot;&nbsp;?{% endblocktrans %}

Pour l'instant j'ai tout mis dans un commit, je suis conscient qu'il faudrait en faire deux (suppression de active_role et correctif avec table.context) mais c'est assez laborieux à rebase comme tout est sur la même ligne. Dis moi si ça serait vraiment mieux, je le ferai.

Il y a le même genre d'incohérence dans service_roles_table.html.

À part ça, deux des fichiers que tu as listé ne sont plus utilisés, j'ai l'impression, et n'ont pas été touchés depuis 2015. Donc pour l'instant je n'y ai pas touché. Ces fichiers sont :

src/authentic2/manager/templates/authentic2/manager/ous_table.html
src/authentic2/manager/templates/authentic2/manager/role_children_table.html

Leur abandon a l'air de bien dater, je peux m'occuper d'ouvrir un autre ticket et de les supprimer si besoin. Il y a potentiellement plus que ça à nettoyer, par exemple la vue inutilisée RoleChildrenView référence une template qui n'existe plus depuis 2016 (voir 76b070f482712e46ef711c1187766838a6240e6d par ex).

Dernier truc, est-ce qu'il faut ajouter des tests pour chaque modif dans les templates ?

#8 Updated by Valentin Deniaud 3 months ago

  • Status changed from En cours to Solution proposée

#9 Updated by Benjamin Dauvergne 3 months ago

  • Status changed from Solution proposée to En cours

Oui blocktrans ne permet pas d'utiliser des chemins {{ x.y.z }} parce qu'il transforme ça xxx {{ uuu }} en ça "xxx %(uuu)s" et l'interpolation classique ne supporte pas le déréférencement.

Ok avec la modification plutôt réécrire comme cela (et en un seul commit pas de soucis) :

{% blocktrans with name=row.record.name role=table.context.object.name %}Do you really want to remove role &quot;{{ name }}&quot; from role &quot;{{ role }}&quot;&nbsp;?{% endblocktrans %}

car les rôles ont une méthode unicode et donc savent s'afficher dans un template, pas besoin de désigner leur attribut name.

Tu peux virer les deux templates inusités dans un deuxième commit.

#10 Updated by Valentin Deniaud 3 months ago

  • Status changed from En cours to Solution proposée

#12 Updated by Frédéric Péters 3 months ago

Je déconseille de mêler les traductions au code, ça rend les rebase/merge trop difficiles.

#13 Updated by Paul Marillonnet 3 months ago

Quitte à virer la vue de table, autant sortir la table associée aussi (RoleChildrenTable).

#15 Updated by Benjamin Dauvergne 3 months ago

  • Status changed from Solution proposée to Solution validée

Ok.

#16 Updated by Valentin Deniaud 3 months ago

  • Status changed from Solution validée to Résolu (à déployer)
commit 14361e000bb8660498969eb00d9ddc8fccb82882
Author: Valentin Deniaud <vdeniaud@entrouvert.com>
Date:   Thu Mar 14 11:44:32 2019 +0100

    manager: remove some unused code (#31222)

commit 26dd7e67b60d84026b09d8fd471937219e1ffb0f
Author: Valentin Deniaud <vdeniaud@entrouvert.com>
Date:   Thu Mar 14 12:02:57 2019 +0100

    manager: update translations (#31222)

commit df8cc6dff595a634996c19da3230b1795fc018d5
Author: Valentin Deniaud <vdeniaud@entrouvert.com>
Date:   Fri Mar 8 13:56:25 2019 +0100

    manager: access context through table object bis (#31222)

    Related to #31092.

#17 Updated by Frédéric Péters 3 months ago

  • Status changed from Résolu (à déployer) to Solution déployée

Also available in: Atom PDF