Aufinal/refactor history #496
No reviewers
Labels
No labels
devtype -- backend
devtype -- docs
devtype -- frontend
devtype -- user interface
difficulty -- easy
difficulty -- hard
difficulty -- normal
Doing
domain -- bda
domain -- bds
domain -- cof
domain -- core
domain -- kfet
Good first issue
priority -- high
priority -- low
priority -- medium
priority -- staff-wanted
status -- development
status -- discussion
status -- need review
status -- production
status -- ready to merge
status -- todo
To Do
type -- bug
type -- hygiene
type -- improvement
type -- new feature
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: DGNum/gestioCOF#496
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "Aufinal/refactor_history"
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?
Historyconstruite surModelForest, avec toutes les classes d'opérations nécessaireshistory,kpsul,transfers,account_read.transfers_createsera réparée)Note: cette MR s'appuie fortement sur !173, et remplace !132
assigned to @delobell
added ~25 label
added 1 commit
c99e4f26- Move history initialisation as Config.reset callbackCompare with previous version
added 5 commits
565a0543- Add support for low stock cssfc3e86ae- Add websocket support to ArticleManager3465dd70- Change node.type to node.modelname for clarityfe965875- Merge branch 'Aufinal/refactor_articles' into Aufinal/refactor_history1fcd53d7- Continue renaming node.type to node.modelnameCompare with previous version
mentioned in issue #144
added 1 commit
aa6a50a6- Simplify JS-Python interface for cancel_opsCompare with previous version
added 9 commits
aureplop/kpsul_js_refactor0997d850- Merge branch 'aureplop/kpsul_js_refactor' of git.eleves.ens.fr:cof-geek/gestioCO…Compare with previous version
added 3 commits
5ff8f69b- Merge branch 'aureplop/kpsul_js_refactor' of git.eleves.ens.fr:cof-geek/gestioCO…e6735d44- Merge branch 'Aufinal/refactor_articles' into Aufinal/refactor_historya1c97618- Fix transfer sortCompare with previous version
added 7 commits
aureplop/kpsul_js_refactor7d93d91a- Merge remote-tracking branch 'origin/aureplop/kpsul_js_refactor' into Aufinal/refactor_articles2774dbb5- Merge branch 'Aufinal/refactor_articles' into Aufinal/refactor_historyCompare with previous version
added 341 commits
aureplop/kpsul_js_refactorb91edc9c- Merge remote-tracking branch 'origin/aureplop/kpsul_js_refactor' into Aufinal/refactor_articles3ce4dc5c- Add article stock management07290f6f- Merge branch 'Aufinal/refactor_articles' into Aufinal/refactor_historya173be4f- Use api_with_auth in history20b7156e- Improve type displayec9f4727- Fix WS update functionsCompare with previous version
added 1 commit
514f1da6- Fix SpecialOpeFormatterCompare with previous version
Le nom est malheureusement déjà pris par l'API pour manipuler l'historique du navigateur :
https://developer.mozilla.org/en-US/docs/Web/API/History_API
D'où les
khistoryà la place, si t'as un nom plus mignon je t'en prie ;-)Je t'apprends rien mais on a
OperationWebsocketA passer dans
resetdeKPsulManagerEdit: oublie, i'm a boulet
Il y a encore besoin de la class
table?Pour faire plus "propre" (à mon avis)
Ça peut rentrer dans la class
History:fetch(options)qui fait ça sans enregistrer dansapi_optionset qui fait l'appel àfromAPI.History.api_optionspermettrait d'avoir des valeurs par défaut sioptionsn'est pas spécifiée (doncHistory.resetferait juste unfetch()à la place dufromAPILa logique de récupération des options depuis les inputs reste ici.
A passer dans
KPsulManager.resetKPsulManager.resetdevrait faire unthis.focus. Lefocus_nextici peut alors être retiré.Afin d'avoir un peu plus de clarté, est-ce que tu pourrais mettre la logique de sélection dans une classe distincte ?
On peut imaginer qu'on veut pas forcément que l'historique soit sélectionnable. Une option pourrait indiquer ça. Seulement si elle est activée, on crée l'objet
HistorySelection.Tu fais pas confiance à l'API pour renvoyer les bonnes données ? ^^
Le
is_validsert à vérifier que lesopegroupsreçus sont conformes aux options de l'historique (c'est pour ça que je dois les conserver d'ailleurs)Du coup, il faudrait faire un
reset(options), ce qui me gêne un peu...Effectivement c'est pas joli. Mais sinon, j'me dis qu'il devrait juste pas y avoir de
fetchdansresetAu début de
fetch, tu peux faire un reset du coupadded 20 commits
aureplop/kpsul_js_refactor05156f37- Update addExistingPurchasee5791efe- Remove last traces of old articlescb28b928- Remove articleSelect from _env021937a3- Small bugfixes9c559d9e- Add articles reset to kpsul.reset3b9affb3- Add focus methodsa29de134- Move focus ; move is_low_stock to method840010b6- Merge remote-tracking branch 'origin/aureplop/kpsul_js_refactor' into Aufinal/refactor_articles1761c5f1- Change fromAPI logic360c442a- Remove useless classe051631a- Use WebSocket classes7ec7ed26- Rename History class290d4ecb- Merge branch 'Aufinal/refactor_articles' into Aufinal/refactor_history8d13c0a4- Add fetch method88f7ea94- Move selection logic to another classCompare with previous version
Y'a juste à passer un template qui n'a pas de champ
class="trigramme"en théoriehistoryest prisC'est jetable non ?
et je trouve que ça fait plus sens de soit passer un template sans
class="trigramme", soit une option qui switche modifie le template utilisé dans KHistory pour virer le div concernénope
added 2 commits
f57c2921- Rename history vared0a82ed- Add no_trigramme optionCompare with previous version
added 73 commits
aureplop/kpsul_js_refactor051231a0- Merge remote-tracking branch 'origin/aureplop/kpsul_js_refactor' into Aufinal/refactor_history688d5bba- Adapt history to new structureCompare with previous version
added 2 commits
5e875263- Add index for Day objects47da80f2- Add related objectsCompare with previous version
Tu peux le passer en
defaults_apiet créerdefaultsdont tu te sers au début du contructeur, les templates passent alors dedans et ça te permet de virer lesif (options..ouif (!options...dans le constructeurLe contenu du
else ifpasse dans le constructeur aussiA mon avis, le block
if (window.kpsul)devrait pas avoir à être là.Pour compenser, on peut créer un lancer un event que kpsul récupérerait. Ça serait plus pratique à faire si api_with_auth acceptait les callbacks de la même manière que
$.ajaxTu peux utiliser
to_update.update(update_data)qui fournit une meilleure encapsulation au cas où on changerait la structure interne deModelObjectY me semble que ça devrait plutôt être
==au cas oùverbose_name = new String('opegroup')Idem
A mon avis, il vaut mieux faire une classe par opération spéciale. Ça devrait grandement nous aider par la suite pour le panier & co.
Pour le système d'index, avec cette structure en forêts, ça a l'air de tenir la route.
Afin d'avoir un peu plus de flexibilité à l'utilisation de classes et sous-classes, est-ce que traverse ne prendrait pas plutôt une classe comme premier argument et checke avec isInstance plutôt que le nom du modèle ?
Bon pour l'instant ça a pas l'air de trop poser problème d'utiliser des arbres mais je maintiens que c'est une limitation
added 4 commits
cfb39b10- Better default options983a5578- Add cancel_history eventcd0e4c6f- Allow (basic) chaining on api_with_auth84d47827- Compatibility changes on history.jsCompare with previous version
Plutôt le lancer sur this (l'objet
KHistory) et je crois que jQuery propose qqch de plus portable pour lancer des events (à vérifier)Pour moi, justement, on les traite tous de façon similaire (sauf ptet
withdrawou il faut penser à le mettre en négatif), donc on perd plus à faire une classe différente pour tous.added 1 commit
ce3d8aa6- Change event triggered when canceling opesCompare with previous version
Effectivement j'avais en mémoire qu'on avait plein de fonctions différentes selon le type d'opération mais en fait ces fonctions sont très très proches... donc on peut s'en passer.
Après justement pour les petites personnalisations en fonction du type d'opération, on peut mettre la logique commune (donc presque tout) dans SpecialOperation, puis les petites personnalisations dans des sous-classes (type le - du retrait). Je propose de garder ça dans un coin de la tête quand ce type de problématique va apparaître.
Ah je pensais pas exactement à ça quand on en parlait. Pour ici faut lancer un event type
cancel_donesur this. T'as essayé si juste$(this).trigger('...')ça fonctionnait à tout hasard ?added 2 commits
2eba6892- Revert "Change event triggered when canceling opes"8c02e5da- Trigger event on history itselfCompare with previous version
Ça devrait plutôt être au début de
displayà mon avis.En plus l'historique précédent resterait affiché le temps de recevoir et traiter la réponse.
J'ai le sentiment que
add_nodeetupdate_nodedevrait être surModelForestÇa marche parce qu'on affiche un élément pour le jour. Mais si on remplace le template de
daypar''. L'item est placé au mauvais endroit.En créant un div "children", on devrait éviter ça.
Tu peux ajouter un warning et un todo ici stp ?
Comme quoi l'objet ajax n'est pas utilisable s'il y a une erreur d'authentification/permissions
Si on essaie d'éviter les arguments par défaut, faut changer ici
Il manque des parenthèses non ?
Ça concerne pas directement cette MR, mais je me demande s'il vaut mieux pas un bouton pour demander
updateHistory. Là c'est fait dès que y'a une modification dans les filtres, ça génère beaucoup de requêtes du coup.Ces deux là devraient plutôt être dans KHistory et ArticleManager à mon avis
Pas besoin du lambda ? T'as pas de problème sans le filter ?
et même chose 2 lignes en dessous
On s'en fiche un peu mais là il va te dire que y'a erreur d'alignement. Je ferais plutôt :
J'ai lu que à partir de Django v qqch, les
select_relatedne doivent se faire que sur les champs correspondants à des objets "entiers".En gros il faut juste faire
from_acc,to_accetcanceled_by.Y'en a aussi qq lignes au-dessus au passage. De manière générale tu peux les modifier quand t'en croises comme ça ;-)
Pour faire plus court, tu peux passer ça avant le
if accountsavec `if not request.user.has_perm...: accounts = [request.user.acc...]Le fait est que
update_nodeutilisethis.containeretthis.templates, donc c'est pas évident à bouger.Plus globalement, en voulant être un peu customisable, je passe mon temps à donner
templatesetcontaineren arguments... Tu penses que ça serait plus pratique de les mettre comme attributs et de les changer à chaque appel àdisplay?Quelque part, il reste utilisable ; il faut juste refaire le branchement selon
$xhr.responseJSON.Je suis d'accord, c'est assez relou...
J'ai oublié de le remettre en virant un changement, mais j'ai jamais eu d'erreurs. Ceci dit, ça peut pas faire de mal.
added 6 commits
de865c61- Move container clearing034a6614- Tweaks on kfet.js659b2089- Add filter to cancel_opesb544d6c5- Better alignment10068645- Change select_related for future compatibilityb2a5dfd6- Move permission checkCompare with previous version
Ah oui en fait ça gère et les données + de l'affichage. La chose étant que
ModelForestest dédiée aux données donc lui passer lestemplatesetcontainerdès le constructeur je suis pas super chaud de base. Cependant j'suis d'accord qu'on devrait pas devoir à s'embêter autant avec cestemplatesetcontaineralors qu'on est sur le composant qui gère l'historique.La question c'est: est-ce qu'on veut avoir plus affichages distincts (et simultanés) pour une seule
ModelForest? Si c'est pas le cas (ou qu'on dit que c'est pas le cas pour l'instant), on peut dire que ModelForest fait plus que juste gérer les données. Au pire les affichages supplémentaires peuvent être gérés "à la main" en dehors deModelForest...Qu'entends-tu par les changer à chaque appel à
display?ArticleManager certes, mais l'autre est spécifique à kpsul...
added 42 commits
aureplop/kpsul_js_refactor20d63513- Merge remote-tracking branch 'origin/aureplop/kpsul_js_refactor' into Aufinal/refactor_historye283439e- Create ForestDisplay class46ac82fd- Adapt history to changes5096e5f1- Adapt ArticleManagerCompare with previous version
added 1 commit
16dbfed9- Add chidren div in displayCompare with previous version
added 1 commit
8b8a3f8a- Update button in historyCompare with previous version
Solution retenue pour l'instant : une classe
ForestDisplayà part, qui demandecontainerettemplatesà la création, et qui n'a pas besoin d'être subclassée pour chaque implémentation deModelForest.Ça marche aussi de donner la référence à la
ModelForest? Si oui, tu peux retirer le début deupdate.La mise à jour de l'historique devrait directement être dans l'objet KHistory je pense, avec une option "live" pour activer ou non cette fonctionnalité
Ça marcherait, mais au départ mon idée était d'avoir un display totalement indépendant du data, et seulement lié à un
container+templates. Ceci dit, j'ai quand même besoin de conserverdatapour la structure.added 1 commit
31b742fd- Move ws update to respective classesCompare with previous version
added 1 commit
c12c705f- Bind ForestDisplay to initial dataCompare with previous version
resolved all discussions
Elle devrait s'appeler
khistoryIdem
A mon avis, les transferts ne devraient pas apparaître sur l'historique de K-Psul
Je pense qu'il faut mettre à jour des choses dans le js intégré à
kpsul.html.Par exemple, dans
addExistingPurchase:var article = kpsul.article_manager.get_article(parseInt(id));TypeError: kpsul.article_manager.get_article is not a functionOn a aussi perdu le contrôle du nombre d'article commandé au clavier
added 3 commits
f4cb1e2e- Add opesonly optionad426872- Fix tranfers pageac33e630- Fix addExistingPurchase + few other bugsCompare with previous version
Il ne sert même plus
Idem
Idem
added 1 commit
c2da055b- Remove duplicate ws updatesCompare with previous version
J'ai l'impression que c'est mieux de faire :
var options = $.extend({}, this.api_options, api_options);. On risque de perdre lesopesonlyou autres définis lors de la création d'un objetKHistorysinon.Il ne sert plus aussi
Il faut quand même conserver les options (pour tester les WS), mais un
$.extend(this.api_options, api_options)peut être pratique oui.Je l'utilise un peu partout pour trigger les reset (notamment de kpsul), ceci dit, il y a une typo dans
history.html.added 2 commits
93c8844b- typo43e77236- Extend history optionsCompare with previous version
Ah il se passe par le nom en chaine, pas par la variable ^^
resolved all discussions
Faudrait ajouter
if (this.selection) this.selection.reset();à on_success.Les 4 utilisations de
KHistoryle font de leur côté.added 1 commit
6a6fc38e- Add selection reset to cancel_opesCompare with previous version
merged
mentioned in commit
311e0c48bdresolved all discussions
mentioned in issue #77