Refactor credit runout calculation and simplify related logic - #1351
Refactor credit runout calculation and simplify related logic#1351simbabimba-dev wants to merge 8 commits into
Conversation
MrWeez
left a comment
MrWeez
left a comment
There was a problem hiding this comment.
And general thing: previously message contained ranout date, but you're including value now. Doesn't make sense to show duplicate information. It should be either date or just text
resolved, i added the date instead |
|
I have read and agree to the CLA. |
|
@simbabimba-dev have you checked all scenarios? Including when time left should display 1 day and x hours until ranout? |
MrWeez
left a comment
MrWeez
left a comment
There was a problem hiding this comment.
Just one thing, otherwise LGTM
| 'unit' => 'minute', | ||
| 'bg' => self::TIME_LEFT_BG_DANGER | ||
| ]; | ||
| return $hoursLeft < 1 ? __('You ran out of Credits') : strval($hoursLeft); |
There was a problem hiding this comment.
| return $hoursLeft < 1 ? __('You ran out of Credits') : strval($hoursLeft); | |
| return $hoursLeft < 1 ? __('You ran out of Credits') : strval(number_format($hoursLeft, 0)); |
right now, it just shows hours, but im editing it to show it in days too |
|
| public function getTimeLeftBoxUnit(float $daysLeft, float $hoursLeft) | ||
| { | ||
| if (!$date) return null; | ||
|
|
||
| $now = now(); | ||
| $daysLeft = $now->diffInDays($date, false); | ||
| $hoursLeft = $now->diffInHours($date, false); | ||
| $minutesLeft = $now->diffInMinutes($date, false); | ||
|
|
||
| if ($daysLeft > 1) { | ||
| return [ | ||
| 'value' => floor($daysLeft), | ||
| 'unit' => 'days', | ||
| 'bg' => $daysLeft >= 15 ? self::TIME_LEFT_BG_SUCCESS : | ||
| ($daysLeft <= 7 ? self::TIME_LEFT_BG_DANGER : self::TIME_LEFT_BG_WARNING) | ||
| ]; | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Dead function: always returns null (marked deprecated in the docblock), but it's still called in index() and the result is passed into the view. Either drop the call + method, or leave a comment explaining why it's kept
| use Illuminate\Support\Facades\Hash; | ||
| use Illuminate\Support\Facades\Http; | ||
| use Illuminate\Support\Facades\Storage; | ||
| use Illuminate\Support\Facades\URL; |
| return $this->calculateCreditRunout($user, $credits); | ||
| }); | ||
| /** Build our Time-Left-Box */ | ||
| if ($credits > 10 && $usage > 0) { |
There was a problem hiding this comment.
Why 10 credits and not 0 as it was?
| 'simulation_steps' => [] | ||
| ]; | ||
| if ($daysLeft >= 15) { | ||
| return $this::TIME_LEFT_BG_SUCCESS; |
There was a problem hiding this comment.
Works, but since these are class constants, self:: is more idiomatic than $this::
| $usage = Auth::user()->creditUsage(); | ||
| $credits = Auth::user()->credits; |
There was a problem hiding this comment.
Auth::user() is called 4 times in index(). Previously it was cached in $user = Auth::user() once. Not a functional issue, but reintroducing the local variable would be more readable
|
After resolving this can be merged |
Very self explanatory, simplifies the credit runout estimate timer as it was before