Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Beständige Bookingcodes #1497

Merged
merged 10 commits into from
May 29, 2024
Merged

Conversation

nelarsen
Copy link
Contributor

@nelarsen nelarsen commented Jan 20, 2024

Mit diesem PR

Copy link

codecov bot commented Jan 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.50%. Comparing base (72ead46) to head (0272882).
Report is 163 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1497      +/-   ##
============================================
- Coverage     45.70%   45.50%   -0.20%     
+ Complexity     2560     2548      -12     
============================================
  Files            93       93              
  Lines         10095    10038      -57     
============================================
- Hits           4614     4568      -46     
+ Misses         5481     5470      -11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nelarsen nelarsen changed the title Ewige Bookingcodes Beständige Bookingcodes Jan 20, 2024
@nelarsen nelarsen marked this pull request as ready for review January 20, 2024 21:31
@nelarsen nelarsen mentioned this pull request Jan 20, 2024
@chriwen
Copy link
Member

chriwen commented Feb 10, 2024

@nelarsen Vielen Dank für den PR. Wir würden das Thema gerne noch einmal gründlicher beleuchten, bevor wir das in die Produktivumgebung ausrollen. Dazu müssen wir aber noch um etwas Geduld bitten. Der PR bleibt hier aber weiter offen und sobald wir es zeitlich einrichten können, werden wir das Thema prüfen. Viele Grüße

…uld not change behavior). The new function is similar to getCode() but does not generate codes.
…code already exists for the given day, item, timeframe and location. do not test for timeframeId and LocationId of bookingcodes because they are now deprecated
…st if new codes should be generated. Instead, do a thorough test with lookupCode for each day.
…ve associated location and timeframe anymore
@nelarsen
Copy link
Contributor Author

Ich habe den PR auf den aktuellen master rebased, was sehr einfach ging. Ich werde noch mal manuell testen (die automatisierten Tests haben sofort funktioniert) und melde mich wieder.

@nelarsen
Copy link
Contributor Author

nelarsen commented May 9, 2024

Hallo, hallo, ich würde gern zu diesem PR zurückkommen. Der PR macht die Spalten 'timeframe' und 'location' in der cb_bookingcodes-Tabelle obsolet. Nach aktuellem Stand werden weder die Spalten noch die vorhandenen Daten gelöscht, und werden neue Codes erzeugt, werden location und timeframe bloß mit 0 gefüllt und nicht weiter beachtet. Das bedeutet aber, dass man CB danach nicht ohne Weiteres downgraden kann, weil CB ohne diesen PR nicht mit den Nullen korrekt umgehen kann. Muss eine derartige Rückwärtskompatibilität gewährleistet werden? Ich denke nicht - man soll halt eine Wordpress-Sicherung erstellen, bevor man eine neue CB-Version installiert, und wenn die neue Version nicht gefällt, alles über die Sicherung wiederherstellen. Wenn man auf derartige Rückwärtskompatibilität eh verzichtet, wäre es sinnvoll, location und timeframe nicht bloß zu ignorieren, sondern beim Plugin-Upgrade zu löschen bzw. bei Neuinstallation nicht anzulegen. Das habe ich allerdings (noch) nicht implementiert. Vielleicht können wir das demnächst kurz in einer Videokonferenz oder so besprechen, damit wir es bald abschließen können?

@chriwen
Copy link
Member

chriwen commented May 11, 2024

@nelarsen Ich würde gerne eine rückwärtskompatibilität beibehalten, vor allem für Test und Entwicklungsszenarien. Da switchen wir ja u.U. zwischen Versionen und es wäre ungünstig wenn das dann nicht ginge. Vielleicht kann man den PR so bauen dass das trotzdem klappt?

@nelarsen
Copy link
Contributor Author

@nelarsen Ich würde gerne eine rückwärtskompatibilität beibehalten, vor allem für Test und Entwicklungsszenarien. Da switchen wir ja u.U. zwischen Versionen und es wäre ungünstig wenn das dann nicht ginge. Vielleicht kann man den PR so bauen dass das trotzdem klappt?

Ich kann den PR ergänzen, so dass location und timeframe in der Funktion persist() wie gehabt in die cb_bookingcodes-Tabelle eingefügt werden. Ich würde ein Kommentar machen, dass die Felder nicht mehr zu verwenden sind und deprecated sind. Mehr müsste man für Rückwärtskompatibilität m.E. nicht tun.

@nelarsen
Copy link
Contributor Author

Gibt es darüber hinaus Abstimmungsbedarf?

@hansmorb hansmorb added this to the 2.10 milestone May 29, 2024
@hansmorb hansmorb added the php Pull requests that update Php code label May 29, 2024
@hansmorb hansmorb merged commit bd4edc1 into wielebenwir:master May 29, 2024
10 checks passed
@hansmorb
Copy link
Contributor

Sorry, der merge war aus Versehen, falls du das nochmal neu aufsetzen könntest wäre das toll! Ich wollte das eigentlich nur in einen TEstbranch mergen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
php Pull requests that update Php code
Projects
3 participants