Support for redirects with a condition before date #2434
Conversation
|
I've checked this in diagonal, but it looks good in general. I'll approve the CI execution so that you can fix anything that fails, and I'll try to come back to it in a couple weeks and do a proper review. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2434 +/- ##
=============================================
+ Coverage 93.81% 93.82% +0.01%
- Complexity 1707 1713 +6
=============================================
Files 277 277
Lines 5918 5928 +10
=============================================
+ Hits 5552 5562 +10
Misses 366 366 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
If everything is fine and no edits are needed, I’ll create the after-date condition in the same way. |
|
Hi! Let's continue working? I want to use the feature in production! |
I think I clearly explained it would take quite some time for me to be able to focus on reviewing this #2431 (comment) Also, after merging, it will have to wait for the rest of the features in 4.5 to be implemented before releasing it, so don't expect this to be available before the end of the year. |
There was a problem hiding this comment.
Thanks for your patience, but I wanted to avoid spreading my attention among multiple topics, as that usually means getting nothing done.
I suggested a small change to type the date as Chronos in RedirectCondition::forBeforeDate(), but other than that, this looks good to me.
You'll need to rebase develop, as there seems to be some conflicts.
| public static function forBeforeDate(string $date): self | ||
| { | ||
| return new self(RedirectConditionType::BEFORE_DATE, $date); | ||
| } |
There was a problem hiding this comment.
I'm wondering if this should be typed as Chronos, and do the cast to string here.
| public static function forBeforeDate(string $date): self | |
| { | |
| return new self(RedirectConditionType::BEFORE_DATE, $date); | |
| } | |
| public static function forBeforeDate(Chronos $date): self | |
| { | |
| return new self(RedirectConditionType::BEFORE_DATE, $date->toAtomString()); | |
| } |
That would be more in line with methods like forDevice, where DeviceType is expected, not string, and the casting is done there appropriately, avoiding user errors.
That means the value needs to be converted from the input string to Chronos somewhere, but I think that's fine, as that can be done with normalizeDate
| yield 'date later than current' => [Chronos::now()->addHours(1)->toIso8601String(), true]; | ||
| yield 'date earlier than current' => [Chronos::now()->subHours(1)->toIso8601String(), false]; |
There was a problem hiding this comment.
As per my previous comment, you can probably pass a raw Chronos object here.
| yield 'date later than current' => [Chronos::now()->addHours(1)->toIso8601String(), true]; | |
| yield 'date earlier than current' => [Chronos::now()->subHours(1)->toIso8601String(), false]; | |
| yield 'date later than current' => [Chronos::now()->addHours(1), true]; | |
| yield 'date earlier than current' => [Chronos::now()->subHours(1), false]; |
Added one of the date-based conditions (before-date)
#2431
https://github.com/shlinkio/shlink/discussions/2101#discussioncomment-9108182