Produits Entr'ouvert » Combo

(je complète pour moi)

aller sur https://agents-clisson.test.entrouvert.org/
chercher test1 : ...

on voit bien les infos du compte test1, mais cliquer dessus et ...

... on arrive sur sa propre fiche

URLs :

• https://agents-csma.test.entrouvert.org/fiche-usager/089666e8d39548f79e107e92099bee12/ (affiche "test1")
• https://agents-clisson.test.entrouvert.org/fiche-usager/089666e8d39548f79e107e92099bee12/ (affiche, dans mon cas, "fred")

(on note le même uuid)

Mais de fait, c'est le principe du "concerned_user" qui foire et ne permet pas d'avoir les cellules "données de cet usager" pour les autres cellules liées à un usager (combo/apps/family, combo/apps/wcs, combo/apps/fargo)... non ?

Le "soucis" c'est que les cellules concernant un usager demandent un usager connu localement, ne peuvent pas se fier à un uuid lancé en l'air.

    if 'name_id' in ctx and UserSAMLIdentifier:
        try:
            ctx['selected_user'] = UserSAMLIdentifier.objects.get(name_id=ctx['name_id']).user
        except UserSAMLIdentifier.DoesNotExist:
            pass

pourrait être modifié pour avoir à la place du "pass" un ctx['selected_user'] = FakeTransientUser(name_id=...); en s'assurant que cet objet contienne le nécessaire pour être accepté à différents endroits.

Thomas me pointe qu'en fait les modèles proxy existaient déjà en 1.8, je ne sais plus d'où j'avais l'idée inverse.

Mais en passant, comme j'ai trouvé get_user_model().add_to_class() qui est du monkeypatch pas trop dégueu, ça me va de rester ainsi.

Mais en passant, comme j'ai trouvé get_user_model().add_to_class() qui est du monkeypatch pas trop dégueu, ça me va de rester ainsi.

Surtout qu'en fait, en essayant quand même, ça échoue, ça ne semble pas bien se mixer le proxy let modèle User (User cannot proxy the swapped model 'profile.User').

Je n'arrive pas à voir si c'est un cas réaliste mais get_user_from_name_id ne renvoie None que si mellon n'est pas là, et dans lingo :

user = get_user_from_name_id(request.GET.get('NameId'))
if user is None:
   raise User.DoesNotExist()
.....
item = BasketItem.objects.get(id=request_body.get('basket_item_id'),
                user=user, cancellation_date__isnull=True)

Et donc erreur 500 si le nameId passé correspond à un utilisateur qui ne s'est jamais authentifié dans combo ?

Je vous lis, mais je ne comprend pas pourquoi on cherche localement un utilisateur dont toutes les données vont être prises à distance, ça a du être une facilité de porter des cellules faites initialement pour le portail usager je suppose.

Le truc ci-dessous pourrait se rajouter dans tests_profile.py (c'est un peu tordu mais ça passe dans du code de tes patchs), et pourquoi pas dans d'autre cellules concernées ?

ctx = {'synchronous': True, 'name_id': '6666'}
modify_global_context(None, ctx)
context = cell.get_cell_extra_context(ctx)
assert context['profile_fields']['first_name']['value'] == 'Foo'
assert context['profile_fields']['birthdate']['value'] == datetime.date(2018, 8, 10)
assert requests_get.call_args[0][0] == 'http://example.org/api/users/6666/'

Et à vrai dire, pour rebondir sur la remarque de Benjamin et sur l'idée de départ return '{%% load combo %%}%sapi/users/{%% firstof name_id concerned_user|name_id %%}/' % idp.get('url'), je trouverais ça plus lisible qu'on touche aux cellules concernées pour qu'elles puissent utiliser tel quel un name_id qui serait dans leur contexte.

Les contorsions pour maintenir l'abstraction du concerned_user sont un peu dure à suivre.

Benjamin :

ça a du être une facilité de porter des cellules faites initialement pour le portail usager je suppose.

L'objectif était bien de pouvoir utiliser dans le portail agent les cellules déjà développées pour le côté front, en utilisant ce qui existait déjà, c'est-à-dire get_concerned_user().

Emmanuel :

(...) je trouverais ça plus lisible qu'on touche aux cellules concernées pour qu'elles puissent utiliser tel quel un name_id qui serait dans leur contexte.

J'essaie de comprendre ce que tu vois, sans être bien sûr. J'imagine, tu peux voir l'objet User comme une enveloppe plus ou moins remplie autour du NameID, et logique alors, on pourrait se dire quelle nécessité à l'enveloppe, plutôt porter directement le NameID. C'est assez raisonnable, et sans doute que mon patch encourage cette lecture, en réunissant les différents endroits qui dupliquaient le code permettant d'obtenir un NameID à partir d'un User.

Mais avoir dans le contexte le user, ça reste de toute façon nécessaire pour les cellules fonctionnant avec les données de combo, les cellules notification, tableau de bord, panier, etc. Et ça veut donc dire qu'une cellule qui a besoin d'appeler un service tiers, elle doit le faire avec get_name_id(context['user']) plutôt que juste context['user_name_id'], ça ne me semble pas si problématique.

Aussi, même si ça devrait pouvoir être supprimé, il reste une cellule comme la cellule d'abonnement aux newsletters, qui appelle un service tiers, mais en utilisant l'email comme référence, pas le NameID.

On peut prendre du temps à l'occasion pour passer sur tout ça, au moins mettre par écrit le fonctionnement actuel, réfléchir à comment ça pourrait évoluer, mais pour le moment, ces patchs 1/ corrigent un vrai bug, signalé, 2/ refactorisent avec succès le code, vu que la part de duplication diminue.

Frédéric Péters a écrit :

J'essaie de comprendre ce que tu vois, sans être bien sûr.

J'ai juste l'impression que pour les cellules qui vont chercher leur données auprès d'un service externe sur la base du name_id, on pourrait s'en sortir en gérant de la façon suivante, que ça couvrirait les cas portail usager/agent, mono/multicollectivité, utilisateur provisionné ou pas :

• si il y a un name_id dans mon contexte prendre celui là
• sinon prendre le name_id de l'utilisateur authentifié

Et de là j'imagine un patch simplissime, genre une méthode :

def get_name_id(context, request):
    if 'name_id' in context:
        return context['name_id']
    return request.user.name_id

que les cellules concernées n'auraient qu'appeler, et voilà, comme disent les américains.

Je loupe peut-être quelque chose, etc, etc.

Je loupe peut-être quelque chose, etc, etc.

L'ajout de NameID aux appels se fait déjà de manière "automatique" à un seul endroit, dans combo/utils/request_wrapper.py, sur base d'un objet User.

Non mais dis donc, dans des endroits au on a accès au contexte :

return '/api/users/%s/drafts' % user_name_id
....
return '/api/users/%s/forms' % user_name_id
......
return '{%% load combo %%}%sapi/users/{{ concerned_user|name_id }}/' % idp.get('url')

Bref c'est quand même plus compliqué que ce que j'imaginais donc je retiens ton argument de non duplication.

(je trouve que ça mérite quand même largement des tests unitaires)