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

fix #1505 #1508

Merged
merged 8 commits into from
Jun 22, 2024
Merged

fix #1505 #1508

merged 8 commits into from
Jun 22, 2024

Conversation

hansmorb
Copy link
Contributor

@hansmorb hansmorb commented Feb 8, 2024

@nelarsen Kannst du beizeiten mal schauen, ob der Patch für dich funktioniert?
closes #1505

@hansmorb hansmorb added bug Something isn't working php Pull requests that update Php code labels Feb 8, 2024
Copy link

codecov bot commented May 29, 2024

Codecov Report

Attention: Patch coverage is 97.91667% with 1 line in your changes missing coverage. Please review.

Project coverage is 48.95%. Comparing base (ee5a693) to head (98e1679).
Report is 20 commits behind head on master.

Files Patch % Lines
src/Repository/Timeframe.php 90.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1508      +/-   ##
============================================
+ Coverage     48.86%   48.95%   +0.09%     
- Complexity     2651     2657       +6     
============================================
  Files            96       96              
  Lines         10601    10597       -4     
============================================
+ Hits           5180     5188       +8     
+ Misses         5421     5409      -12     

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

@hansmorb hansmorb marked this pull request as ready for review May 29, 2024 12:43
@hansmorb hansmorb added this to the 2.10 milestone May 29, 2024
# Conflicts:
#	tests/php/Repository/TimeframeTest.php
@nelarsen
Copy link
Contributor

Ok, ich werde es zeitnah testen (sorry, ich habe bisher nicht auf dem Schirm gehabt, dass ich angesprochen wurde).

Ich frage mich, was am Zeitpunkt 23:59:59 besonders ist. An anderen Stellen im Code ist es 23:59 (also 23:59:00).

@nelarsen
Copy link
Contributor

@nelarsen Kannst du beizeiten mal schauen, ob der Patch für dich funktioniert? closes #1505

Nein, der Patch hat das beschriebene Problem leider nicht geheilt (Kombination von langem Zeitrahmen und einem einzelnen zusätzlichen Öffnungstag mittendrin - bis zum zusätzlichen Einzel-Öffnungstag kann man leider aus dem langem Zeitrahmen nichts buchen).

@hansmorb
Copy link
Contributor Author

@nelarsen Kannst du mir mal zwei Beispielzeiträume dafür geben damit ich dafür einen Testcase schreiben kann?

@nelarsen
Copy link
Contributor

Ja, hier sind die Zeiträume mit denen das Problem am 15.06.2024 auftrat.
grafik

Wenn es für dich eine Erleichterung wäre, könnte ich auch den Testcase schreiben.

@hansmorb
Copy link
Contributor Author

@nelarsen Danke dir, das wäre bestimmt nicht schlecht. Aber vielleicht sollte ich am besten sowieso diesen Test refactoren mit einem DataProvider für die möglichen Kombinationen. Das wird jetzt schon unübersichtlich.

@nelarsen
Copy link
Contributor

@nelarsen Danke dir, das wäre bestimmt nicht schlecht. Aber vielleicht sollte ich am besten sowieso diesen Test refactoren mit einem DataProvider für die möglichen Kombinationen. Das wird jetzt schon unübersichtlich.

Ok, ich warte auf weitere Anweiseungen (-:

@hansmorb
Copy link
Contributor Author

@nelarsen Ich habe die Tests refactored und dabei ist mir auch noch ein Bug aufgefallen. Kannst du vielleicht deinen Testcase in den dataProvider provideGetClosestBookableTimeFrameForToday einfügen?

@hansmorb
Copy link
Contributor Author

@nelarsen Habe mal versucht den Testcase nachzustellen, bei mir läuft das durch. Habe ich was vergessen?

@nelarsen
Copy link
Contributor

@nelarsen Habe mal versucht den Testcase nachzustellen, bei mir läuft das durch. Habe ich was vergessen?

Ich glaube nicht. Aber ich glaube, dass du hast du Fehler behoben. 🥳 Jedenfalls habe ich noch mal manuell getestet. Zuerst der alte Stand (sha d528a8d) und dann der neue Stand (sha 98e1679). Das war ausschlagggebend, jetzt funktioniert es. Irgendwo dazwischen hast du wohl den Fehler behoben, kann es sein?

@hansmorb
Copy link
Contributor Author

Super, das freut mir. Ich hatte einen Operator falschrum gesetzt

@hansmorb hansmorb merged commit c60046f into master Jun 22, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working php Pull requests that update Php code
Projects
Development

Successfully merging this pull request may close these issues.

Startdatum für Kalender wird manchmal vom falschen Zeitrahmen genommen
2 participants