Sammelthread: Update der Bestellungen im Admin

Mitmacher · January 20, 2014, 10:30pm

Hallo miteinander,

ich frage mich gerade, ob es nicht sinnvoll ist, die ganzen verschiedenen Probleme hier diskutierend zusammenzufassen, die den Bereich “order recalculation” im Admin betreffen? Da scheint ja ziemlich der Wurm drin zu sein, und man sollte es wohl ganzheitlicher betrachten, als immer nur Bruchstücke mit Hotfixes zu versehen, die überall verteilt liegen. Ich hatte schon meine Mühe, nur die ganzen Bugtracker-Relations dazu zu durchforsten, und wüsste nicht wirklich, wo man sich da am besten einklinkt in eine übergeordnete Debatte. Ich versuche mich mal an einem Überblick (alles auf den Admin bezogen):

Es besteht das generelle Problem, dass beim Neuberechnen der Bestellungen (order_main + order_articles) die aktuellen Artikel-Daten genommen werden, z.b. Preise, Discount, etc. und dadurch evtl. eine alte Bestellung ungewollt verändert wird.
Dazu gibt es mehrere Ansätze und vor allem dieses Jabommi-Modul, welches es wohl am besten vereint. Der Code macht für mich zumindest einen sinnvollen Eindruck. Am Ende dieses Eintrags kommt noch eine interessante Note von leofonic:

imho it is better to actually prevent recalculation than to fix it.

Damit hat er natürlich nicht ganz unrecht und verweist auch gleich auf ein anderes Modul, welches dies ermöglicht.

Das speziellere Problem mit den Auswahllisten (selectlists), welches entsteht, wenn man diese mit Varianten mischt. Die gewählten Values werden nämlich hintereinander (mit Leerzeichen getrennt) in oxorderarticles__oxselvariant gespeichert. Da für die Neuberechnung aber der String erst an “,” und dann an “:” getrennt geparst wird, hat man am letzten Selectlist-Value die Variante kleben. Also werden evtl. Aufschläge nicht berücksichtig, da kein passender Value gefunden wird.
Dazu gibt es z.b. diesen Bug-Eintrag, wo auch ein Bugfix von Aggrosoft dranhängt. Der sorgt dafür, dass die Variante mit “,” angehängt wird. Das ist eindeutig besser, aber auch noch nicht das Gelbe vom Ei, finde ich. Man darf an der Stelle eigentlich keine Varianten mit Selectlisten vermischen, also am besten auch gar nicht erst in oxorderarticles__oxselvariant speichern, oder mit einem eindeutigen Trennzeichen, was beim Parsen abgeschnitten werden muss/kann. Am konsequentesten wäre wohl ein eigenes DB-Feld.
In die gleiche Kerbe (nur ohne Varianten) schlägt eine Sache, die ich kürzlich selbst herausfand und bisher noch nicht meldete: Wenn man nämlich mal eine deutsche/englische Auswahlliste anlegt (mit abweichenden Values!) und damit einen Artikel im englischen Frontend bestellt, dann darf man nicht per deutschem Admin diese Bestellung aktualisieren, sonst wird es wirr, und die Auswahl kann sich ändern. Es wird dort also irgendwo nicht korrekt die Order-Language zum Vergleichen genutzt, sondern Tpl- oder Base-Language. Dadurch kann das “innerste” if in getOrderArticleSelectList() niemals greifen:

if ( $oStr->strtolower($oSel->name) == $sOrderArtSelValue ) {

Nochmals ergänzend dazu greift dieses “if” von eben auch nicht, wenn in den Auswahllisten z.b. Aufschläge hinterlegt sind. Das wird dann nämlich hinter den Values auch in oxorderarticles__oxselvariant gespeichert, z.b. “+1,00 €”, was ich ebenso für falsch, bzw. überflüssig halte. Dieses Feld ist so überfrachtet, dass man es insgesamt kaum noch sinnvoll parsen kann, und ich denke, da sollte man zuerst ansetzen.
Eine weitere Sache die mir auffiel ist, dass Artikel-Bundles einfach gelöscht werden (also die Zusatzartikel), wenn man Bestellungen aktualisiert. Dies hielt ich eindeutig für einen Bug und habe ihn auch hier eingetragen. Erst durch die dortige Relation wurde ich auf den ganzen Berg an weiteren Problem aufmerksam und entschloss mich, es hier zu sammeln.

So, das müsste es wohl erstmal gewesen sein. Wer macht nun einen kompakten (zweizeiligen) Bug-Eintrag daraus? Im Ernst: was fängt man nun am besten mit diesen Erkenntnissen an? Jede Resonanz und Ergänzung ist herzlich wollkommen!

Mitmacher

Hebsacker · January 21, 2014, 7:47am

das hier ist der Sammel-Bugeintrag, wo alle Untermeldungen zugeordnet sind:
https://bugs.oxid-esales.com/view.php?id=4624

Mitmacher · January 21, 2014, 10:47am

Moin Ray,

ja, danke! Aber ich weiß (und habe ihn oben auch erwähnt), aber erstens geht es dort nicht wirklich voran, und zweitens ist es zu verzettelt. Außerdem denke ich, dass eine deutsche Community das Thema auch deutsch diskutieren darf/soll, bevor evtl. Entscheidungen getroffen werden (die dann auch gerne wieder in englisch), oder? Zumindest hoffe ich, dass das nun nicht die einzige Resonanz war, denn so kommen wir dort scheinbar nicht weiter…

Grüße

PS: gibt es denn eigentlich keinen deutschen Core-Programmierer?

Hebsacker · January 21, 2014, 7:04pm

[QUOTE=Mitmacher;138350]… denke ich, dass eine deutsche Community das Thema auch deutsch diskutieren darf/soll…[/QUOTE]
Klar! War nur als Info / Einstieg für alle anderen gedacht.

[QUOTE=Mitmacher;138350]Zumindest hoffe ich, dass das nun nicht die einzige Resonanz war[/QUOTE]

…ich auch

[QUOTE=Mitmacher;138350]
PS: gibt es denn eigentlich keinen deutschen Core-Programmierer?[/QUOTE]

im Forum eher nicht

aggrosoft · January 22, 2014, 1:05pm

Das ganze Thema ist mir auch schon lange ein Dorn im Auge - ich frage mich bis heute warum die Auswahllisten so schlecht normalisiert in der DB abgespeichert werden. Das ganze gilt auch für die Varianten - und da liegt auch das Problem der ganzen Neuberechnung etc.

Es wird mit Strings jongliert statt das einmal sauber neu zu machen und auf einen ordentlichen Stand zu bringen - das ist eine historisch entstandene Sache denke ich. Einer hat das mal so programmiert und seitdem ist das halt so - toll ist das aber wirklich nicht und es führt nur zu fehlern. Das ganze in eine Wawi oder so zu importieren ist grausamst, davon mal abgesehen wenn man das in den Shop spielen will.

Allein wenn man ein Modul schreibt und bestimmte Werte abfragen will - dann geht immer dieses String Vergleich Zeugs los. Das dann mit Multilanguage = Kopfschuss

Und was die Neuberechnung im Backend angeht, das sollte einfach mal deaktiviert werden im Core - wenn jemand das unbedingt so braucht wie es jetzt ist lässt sich daraus eventuell auch eine Option oder ein Modul machen.

marco.steinhaeuser · January 23, 2014, 12:39am

Hi Aggro,

[QUOTE=aggrosoft;138430]lässt sich daraus eventuell auch eine Option oder ein Modul machen.[/QUOTE]

Falls möglich, tendiere ich persönlich immer noch zum Pull Request:

Allerdings bezweifele ich, dass dieses Thema erschöpfend mit ein paar Stunden erledigt werden kann. Hier braucht es ein anderes Konzept.

Gruß

aggrosoft · January 23, 2014, 1:36pm

Also mit einem Pull Request ist das wohl nicht getan, das ist eine Umfangreiche Änderung auf ein neues System für Varianten und Auswahllisten - das würde ein Haufen Module und Shops den Jordan runter jagen. Ich bin der Meinung das ist etwas für das Core Team - denn da hängen eine Menge Sachen dran und das ist etwas für einen Major Release.

Mitmacher · January 23, 2014, 1:43pm

Allerdings bezweifele ich, dass dieses Thema erschöpfend mit ein paar Stunden erledigt werden kann. Hier braucht es ein anderes Konzept.

Ja, das fürchte ich auch. Wobei sich dann evtl. die Frage nach der Abwärtskompatibilität stellen muss. Aber man hat wohl eher Bedarf, nur aktuellere Bestellungen zu verwalten, ab einem gewissen Punkt sollte man die wirklich “sperren”, so wie es leofonic umsetzte (vorhandenes Rechnungsdatum oder anderer Ordner als “Neu” o.ä.).

Das dann mit Multilanguage = Kopfschuss

Genau, dieses String-Gefriemel ist echt suboptimal und macht wenig Spaß! Und an WaWi hatte ich noch gar nicht gedacht, ojeh. Aber mir hat es trotzdem keine Ruhe gelassen und ich habe einen weiteren Hotfix gebastelt (für Punkt 2 + 3 + 4). Auch dies ist alles andere als optimal aber scheint fürs erste zu gehen. Und wenn man den mit Jabommis und leofonics Modul und dem Aggrosoft-Hotfix bündelt, hätte man die Kuh erstmal grob vom Eis (da setze ich mich wohl nochmal ran). Aber folgende Zeile zeigt deutlich genug, dass es natürlich Quatsch ist, erst den Preisaufschlag in oxselvariant zu speichern, um ihn dann mit solchen kruden Mitteln wieder zu entfernen, falls vorhanden:

$aCleaned[] = preg_replace( "/\s+[+-]\d[\d.]*%?\s*(".implode( '|', $this->_getCurrencySigns() ).")?$/", '', trim( $sItem ) );

Dabei gibt es die zwei Fälle, dass Preise mit “,” oder “.” formatiert sind, und falls deutsch, also mit Komma, ist das alleine schon total falsch, wg. oben erwähntem “explode(‘,’…”. Da bleibt von z.b. " +1,73 €" eh nur “+1” übrig!

Noch kurz die skizzierte Idee, wie ich es mit multilang-selectlists löse:
Die Funktion “getOrderArticleSelectList” (oxorderarticle) quasi wrappen und vorher setBaseLanguage auf die Order-Lang setzen und danach wieder zurück zum alten Wert. So ist es halbwegs einfach und updatesicher als Modul, aber sauberer wäre ein Durchschleusen der Order-Lang bis in oxarticle, sodass man dort in getSelectLists() z.b. schreiben könnte:

$sSLViewName = getViewName( 'oxselectlist', $iOrderLang );

Mitmacher · January 23, 2014, 1:46pm

Ups, fast zeitgleich…

Also mit einem Pull Request ist das wohl nicht getan

Glaube ich auch nicht, dass geht wirklich eher Richtung major release, da man ja neue DB-Felder braucht (wenigstens eins, besser zwei), um es sauber zu lösen.

PS: habe gerade noch eine Sache entdeckt. Man schaue sich mal die erste/einzige Bestellung von Marc Muster im Admin eines Demo-Shops an. Dort steht als Summe total: 1.950,59 €, korrekt wäre aber doch 1.969,59 € da die 19,- Zahlungsart-Gebühren noch hinzukämen, oder? Und auch wenn man es neu berechnen lässt, fehlt es, dafür wird der Warenkorb-Rabatt dann zu einem Artikelrabatt, was zwar mathematisch korrekt bleibt, aber trotzdem unnötig die Darstellung verändert. Will sagen: diese Test-Order ist ja bereits einige Jahre alt, und falls OXID seitdem nicht mal solche “banalen” Additionen nicht beherrscht, muss man sich wohl nicht wundern, dass der Rest evtl. auch mal hakt.

leofonic · January 28, 2014, 12:06pm

Falsche Zahlart-Gebühren: https://bugs.oxid-esales.com/view.php?id=5626
Warenkorb-Rabatt wird zu Artikel-Rabatt: https://bugs.oxid-esales.com/view.php?id=5618

Wegen der Speicherung Varianten/Auswahllisten und Stringparsen bei der Neuberechnung etc: Das lässt sich so nicht lösen. Varianten sind imho korrekt, weil hier die Artikelnummer zählt. Wie der Name abgelegt wird ist nicht wichtig. Auswahllisten sind vom Konzept her manuell zu verarbeiten. Wenn man das ändern wollte müsste man das Konzept der Auswahllisten ändern, so dass hier auch unterschiedliche Artikelnummern oder Auswahl-IDs erzeugt werden. Momentan sind Auswahllisten Generatoren für einen Text zur Bestellung, so ähnlich wie: “Bitte teilen Sie uns bei der Bestellung Ihren Farbwunsch mit”. Den Auf/Abschlag kann man nicht neu berechnen, muss man aber eigentlich auch nicht, ist ja bereits berücksichtigt.

Eigentlich ist eine Neuberechnung sowieso immer sinnlos. Die Neuberechnung ist überhaupt nur wegen der Features notwendig, eine Bestellung im Backend zu ändern. Was ist da der Use-Case dafür? Kann mir da nichts wirklich sinnvolles vorstellen, kann mich da mal jemand aufklären?

Der Ansatz von Oxid, die Bestellung einfach bei jeder Änderung neu zu berechnen mit aktuellen Daten ist von der Logik her schon falsch. Deshalb ist auch der Code dazu falsch, und einen Code, der auf falschen Voraussetzungen beruht so zu ändern dass er funktioniert, ist schwer. Meiner Ansicht nach wäre die richtige Lösung, alle Bearbeitungsmöglichkeiten komplett zu entfernen, ebenso den kompletten Code für die Neuberechnung. Das würde als Pull-Request aber leider kaum durchgehen.

Damit hätte man aber alle Probleme mit der Neuberechnung gelöst und könnte sich ein besseres Konzept für Auswahllisten überlegen. Aus Performancegründen wären Auswahllisten ja sinnvoll, aber so wie sie jetzt implementiert sind, sind sie für wenig zu gebrauchen.

Mitmacher · January 28, 2014, 4:54pm

Hi leofonic,
wo du recht hast, hast du recht, und danke für die weiteren Bug-Einträge! Je mehr ich darüber nachdachte, desto mehr wurde mir auch klar, dass es nicht die eine Lösung geben kann. Jeder hat einen etwas anderen Bedarf. In kleineren Shops ohne Wawi kann man z.b. sicherlich noch den einen oder anderen Artikel ändern oder ergänzen, nach evtl. telonischer Absprache und vor dem Versand. Aber sobald echte Buchhaltung ins Spiel kommt, ist es wohl schlicht falsch und verboten, sondern man muss eben dafür einen neuen Vorgang anlegen.
Ebenso die Frage, ob bei einer Neuberechnungen die alten oder die aktuellen Preise genommen werden sollen: je nach Szenario kann beides Sinn machen. Da hilft kein einzelner Hotfix oder PR, sondern nur ein gänzliches anderes Konzept, richtig.

Falls man die Neuberechnungen in der jetzigen Form aber doch braucht und nutzen möchte, wäre es trotzdem schön, wenn es korrekt funktionieren würde. Und hier hege ich leise Zweifel:

Den Auf/Abschlag kann man nicht neu berechnen, muss man aber eigentlich auch nicht, ist ja bereits berücksichtigt.

Was heißt, man kann nicht? Es ist ja genauso vorgesehen, funktioniert nur in einigen Fällen nicht. Und dann wird es eben auch nicht berücksichtigt beim recalc. Da dies der Punkt war, der mich am meisten störte, habe ich dafür noch einen Hotfix geschrieben, der die Punkte 2-4 meines ersten Beitrags in der meisten Fällen behebt. Ich hänge den mal hier mit an, falls es jmd. interessiert. Aber mehr Energie lohnt sich auch aus meiner Sicht nicht zu investieren, um nur die Symptome zu bekämpfen.

PS: ups, ich meinte sogar Punkt 2-5, aber egal…

leofonic · January 28, 2014, 6:09pm

[QUOTE=Mitmacher;138743] In kleineren Shops ohne Wawi kann man z.b. sicherlich noch den einen oder anderen Artikel ändern oder ergänzen, nach evtl. telonischer Absprache und vor dem Versand. Aber sobald echte Buchhaltung ins Spiel kommt, ist es wohl schlicht falsch und verboten, sondern man muss eben dafür einen neuen Vorgang anlegen.[/QUOTE]
Ja genau, irgendwer wollte wahrscheinlich mal das Feature haben und dann ist das eingebaut worden und seitdem ist es so. Ich frage mich wie oft das wohl vorkommen mag dass jemand telefonisch die Bestellung ändern mag. So gut wie nie schätze ich.

[QUOTE=Mitmacher;138743]
Ebenso die Frage, ob bei einer Neuberechnungen die alten oder die aktuellen Preise genommen werden sollen: je nach Szenario kann beides Sinn machen. Da hilft kein einzelner Hotfix oder PR, sondern nur ein gänzliches anderes Konzept, richtig.
Falls man die Neuberechnungen in der jetzigen Form aber doch braucht und nutzen möchte, wäre es trotzdem schön, wenn es korrekt funktionieren würde. Und hier hege ich leise Zweifel:[/QUOTE]
Das größte Ärgernis ist ja dass die Neuberechnung auch zuschlägt wenn man Dinge ändert die überhaupt nichts mit der eigentlichen Bestellung zu tun haben.

[QUOTE=Mitmacher;138743]
Was heißt, man kann nicht? Es ist ja genauso vorgesehen, funktioniert nur in einigen Fällen nicht. Und dann wird es eben auch nicht berücksichtigt beim recalc. Da dies der Punkt war, der mich am meisten störte, habe ich dafür noch einen Hotfix geschrieben, der die Punkte 2-4 meines ersten Beitrags in der meisten Fällen behebt. Ich hänge den mal hier mit an, falls es jmd. interessiert. Aber mehr Energie lohnt sich auch aus meiner Sicht nicht zu investieren, um nur die Symptome zu bekämpfen.[/QUOTE]
Das könnte doch auf jeden Fall als Pull-Request rein. Mit “kann man nicht” meinte ich dass man da dann eben Stringparsen muss oder ähnliches. Es ist zwar so vorgesehen aber nur weil es notwendig ist wenn die Bestellung mit aktuellen Daten neu berechnet wird, eigentlich hat der Kunde den Artikel ja schon bestellt und es gibt überhaupt keinen Grund das alles in seine Bestandteile zu zerlegen und wieder neu zusammenzusetzen, zumindest fällt mir keiner ein.

Mitmacher · January 28, 2014, 7:10pm

Klar, ich gebe dir komplett recht, vor allem:

Das größte Ärgernis ist ja dass die Neuberechnung auch zuschlägt wenn man Dinge ändert die überhaupt nichts mit der eigentlichen Bestellung zu tun haben.

Hm, aber ob mein Hotfix-Modul schon als P-R taugt, kann ich schlecht einschätzen. Im Prinzip führt es die (an dieser Stelle) quere OXID-Logik konsequent zu Ende, und alle Tests waren erfolgreich, egal welche Währung, welche Sprache, eine oder mehrere Auswahllisten, mit oder ohne Varianten, mit oder ohne Aufschlag/Rabatt, äh, das wars wohl. Allerdings wollte ich Redundanz vermeiden, und habe die Preisberechnung so belassen (kein “Jabommi”), d.h. es werden immer die aktuellen Daten genommen.
Da ich aber auch davon ausgehe, der Bedarf ist eher selten und eher in kleineren Shops vorhanden. Dann ist die Wahrscheinlichkeit, dass die Preise sich schnell ändern, auch nicht allzu groß, oder? Wie auch immer, man wird es nie allen 100% Recht machen können, fürchte ich. Aber es kann sich ja mal wer meine Bugfix-Collage anschauen. Ist auch gar nicht soviel Code…

(Um weitere PRs anzuschieben, muss ich mich aber eh noch etwas mehr mit Github beschäftigen. Ist doch a bissl anders als SVN.)

Mitmacher · January 28, 2014, 8:44pm

Ach herrjeh, so kann das natürlich keiner testen, sorry! Nun nochmal korrekt gepackt mit allen Ordnern. Außerdem braucht es die oxbasketitem nicht, die war ja auch wieder für die Preise, das reicht so aber eh nicht. Hier also nochmal das Ganze…