search

cmsimple-xh / xhshop

A simple shop for CMSimple_XH
GNU General Public License v3.0
4 stars 3 forks source link

Catalogue physically sorted?? #213

Closed manu37 closed 3 years ago

manu37 commented 4 years ago

https://github.com/cmsimple-xh/xhshop/blob/7dc4d6c36704423b87b44476145f71fad0d7d800/classes/Catalogue.php#L58-L65 Mir scheint, hier wird versucht, den Catalogue anhand vom SortIndex physisch zu sortieren. Meiner Ansicht nach wird aber der gleiche SortIndex zurückgeschrieben, aber die products Tabelle nicht neu (sortiert) aufgebaut. Bug or Leerlauf? Ich bin draufgekommen, weil die addToCartButtons auf den Seiten (wenn es mehrere Artikel sind) nicht gemäss SortIndex aufgelistet werden.

frase-git commented 4 years ago

Der Katalog wird immer nur in der Reihenfolge des Anlegens gespeichert. Jeder Artikel hat aber einen sortIndex, der durch das Verschieben mit den Pfeilen in der Katalog-Übersicht verändert werden kann (und wird). Offensichtlich wird dieser sortIndex bei der Ausgabe mittels addToCartButton überhaupt nicht berücksichtigt. BUG :(

Du kannst "tricksen": Verändere die Reihenfolge der Artikel in der catalog.php (im content-Ordner - immer einen ganzen Artikel-Block verschieben). Im Frontend in der Shop-Übersicht hat diese Änderung keinen Einfluss. Hier greift der sortIndex. Bei der Ausgabe über addToCartButton wird dann aber nach der manuellen Sortierung angezeigt.

manu37 commented 4 years ago

Danke Frank. Das habe ich bereits so getan. Mir scheint aber, dass es trotzdem die Absicht ist, den Catalogue sortiert nach SortIndex zu speichern. Sonst macht ja der markierte Code keinen Sinn.

frase-git commented 4 years ago

Mir scheint aber, dass es trotzdem die Absicht ist, den Catalogue sortiert nach SortIndex zu speichern. Sonst macht ja der markierte Code keinen Sinn.

Möglicherweise ein Relikt aus dem alten Wellrad-Shop?

manu37 commented 4 years ago

Eine Lösung ginge in etwa so: https://github.com/cmsimple-xh/xhshop/commit/dddedd2e8804a5276e098df00efc525a6f735936#diff-1ac375d7ab9336a6dce8ab59a810abd2 ist aber noch nicht getestet.

manu37 commented 4 years ago

Der Katalog wird immer nur in der Reihenfolge des Anlegens gespeichert. Jeder Artikel hat aber einen sortIndex, der durch das Verschieben mit den Pfeilen in der Katalog-Übersicht verändert werden kann (und wird). Offensichtlich wird dieser sortIndex bei der Ausgabe mittels addToCartButton überhaupt nicht berücksichtigt. BUG :(

Wäre es nicht gescheiter, den Catalogue generell sortiert abzuspeichern? Gespeichert (+sortiert) wird selten, aufgerufen definitiv häufiger.

frase-git commented 4 years ago

Eine Lösung ginge in etwa so:

Getestet und für gut befunden ;-) catalog.php wird tatsächlich komplett neu geschrieben und die Artikel sind absteigend darin angeordnet. Ich sehe auch keine Gefahr, dass an anderer Stelle etwas kaputt gegengen sein könnte.

frase-git commented 4 years ago

Wäre es nicht gescheiter, den Catalogue generell sortiert abzuspeichern? Gespeichert (+sortiert) wird selten, aufgerufen definitiv häufiger.

Das passsiert doch jetzt nach deiner Änderung genau so. Nur neue Artikel werden oben (sortIndex = 1) eingefügt. Diese kann man mit den Pfeilen verschieben und der Katalog wird dadurch immer neu geschrieben (neu geordnet). (Habe dich übrigens als maintainer eingeladen)

cmb69 commented 4 years ago

Ich stimme zu, dass es sinnvoll ist den Katalog bereits sortiert zu speichern. Ich bin aber nicht ganz sicher, wie das am besten gemacht werden sollte. In PR #221 habe ich mal eine Alternative zu @manu37's Lösung bereit gestellt. Diese hat den Vorteil, dass beim Lesen nicht erneut sortiert wird, aber die ursprünglich festgelegte Reihenfolge wird erst wieder hergestellt, nachdem ein Produkt gespeichert wurde. Müsste dann halt in der Release-Ankündigung explizit erwähnt werden.

Welche Variante würdet ihr bevorzugen? Oder eben eine Kombination (@manu37's Variante, und zusätzlich das uasort() nach dem Lesen entfernen)?

frase-git commented 4 years ago

Das Ergebnis ist bei beiden gleich - scheint mir.

... aber die ursprünglich festgelegte Reihenfolge wird erst wieder hergestellt, nachdem ein Produkt gespeichert wurde

Das wäre vielleicht ein minimaler Nachteil für bestehende Shops, aber auch kein Beinbruch.

Welche Variante würdet ihr bevorzugen? Oder eben eine Kombination (@manu37's Variante, und zusätzlich das uasort() nach dem Lesen entfernen)?

Die Kombination scheint mir ganz gut.

Problem bei beiden neuen Varianten: Legt man einen neuen Artikel an und speichert die Eingaben, dann landet man anschließend in der "Erbsen"-Bearbeitung (= letzter Artikel). Das war in Version 1.0 nicht so. Da landete man nach dem Speichern auf der Bearbeiten-Seite des gerade angelegten Artikels. Das wäre - wenn nicht gerade ein Fehler - so doch zumindest verwirrend.

cmb69 commented 4 years ago

Ich habe mich bei PR #221 bezüglich der Branches vertan, und das noch einmal als PR #222 eingestellt.

Legt man einen neuen Artikel an und speichert die Eingaben, dann landet man anschließend in der "Erbsen"-Bearbeitung (= letzter Artikel).

Danke! Das ist natürlich Quatsch, und sollte nun in PR #222 mit dem zweiten Commit auch behoben sein.

manu37 commented 4 years ago

Ich habe mich bei PR #221 bezüglich der Branches vertan, und das noch einmal als PR #222 eingestellt.

Legt man einen neuen Artikel an und speichert die Eingaben, dann landet man anschließend in der "Erbsen"-Bearbeitung (= letzter Artikel).

Danke! Das ist natürlich Quatsch, und sollte nun in PR #222 mit dem zweiten Commit auch behoben sein.

Kann ich jetzt nicht grad folgen. Aber der Fix ist wohl, dass der neue Artikel ans Ende und nicht an den Anfang gelegt wird.

manu37 commented 4 years ago

... Welche Variante würdet ihr bevorzugen? Oder eben eine Kombination (@manu37's Variante, und zusätzlich das uasort() nach dem Lesen entfernen)?

Bei meinem quickfix ist der uasort beim Lesen natürlich überflüssig. Deine Lösung ist wohl im Umgang mit Objekten etwas robuster. Da bin ich mir nie ganz Sicher, was bei einer Zuweisung (by reference) so passiert.

manu37 commented 4 years ago

... aber die ursprünglich festgelegte Reihenfolge wird erst wieder hergestellt, nachdem ein Produkt gespeichert wurde. Müsste dann halt in der Release-Ankündigung explizit erwähnt werden.

Dies ist wohl beim SwapUp/Down der Fall. Hier Jedesmal den catalogue neu sortieren und abspeichern ist sicher overkill. Der Hinweis, dass die Sortierung erst das Speichern eines beliebigen Artikel wieder hergestellt, ist zwar unschön aber korrekt. Ich frage mich langsam, ob das Sortieren beim Speichern nicht ein Schuss ins Knie war. Jeweils Sortieren beim Anzeigen ist zwar aufwändiger, macht aber das handling beim Speichern/Reihenfolge ändern viel einfacher.

cmb69 commented 4 years ago

Aber der Fix ist wohl, dass der neue Artikel ans Ende und nicht an den Anfang gelegt wird.

Genau.

Bei meinem quickfix ist der uasort beim Lesen natürlich überflüssig.

Ich denke, auch da ist das nötig, weil sonst die unsortierte Reihenfolge beim Speichern übernommen wird.

Dies ist wohl beim SwapUp/Down der Fall. Hier Jedesmal den catalogue neu sortieren und abspeichern ist sicher overkill.

Das war sowieso schon so, wenn ich mich nicht irre.

Ich frage mich langsam, ob das Sortieren beim Speichern nicht ein Schuss ins Knie war.

Ich denke, das Sortieren beim Speichern ist schon sinnvoll. Letztlich wird eben viel häufiger gelesen als gespeichert, und da zahlt es sich aus.

manu37 commented 3 years ago

@cmb69: ist hier jetzt der PR #222 der aktuelle Stand?

cmb69 commented 3 years ago

Entweder PR #222 oder PR #214. (Ein Test wäre wünschenswert, ist aber wohl erst mal nicht vernünftig machbar.)