don't try to bulk update if egg doesn't even have a url - #1887
Conversation
keeps it from spamming error messages from update_url=null
📝 WalkthroughWalkthroughSkips eggs with a null Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI / Trigger
participant Action as UpdateEggBulkAction
participant Egg as Egg (each)
participant Updater as EggUpdater::fromUrl
participant Cache as Cache
participant Reporter as ExceptionReporter
participant Notifier as Notification
UI->>Action: Trigger bulk update
Action->>Egg: iterate eggs
Note right of Egg: For each egg
Egg->>Action: provide update_url
alt update_url is null
Action->>Action: skipped_count += 1
else update_url present
Action->>Updater: fromUrl(update_url)
alt success
Updater-->>Action: success
Action->>Action: success_count += 1
Action->>Cache: forget(egg cache key)
else failure
Updater-->>Action: throws
Action->>Action: failure_count += 1
Action->>Reporter: report(exception)
end
end
Note over Action: after loop, build notification body using failed and skipped counts
Action->>Notifier: send(status = (failure_count > 0 ? warning : success), body)
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
I'd prefer the change to be in the actual action. UpdateEggBulkAction.php |
|
did that now, i was thinking, to filter out all records with no url before doing the action |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Filament/Components/Actions/UpdateEggBulkAction.php (1)
50-75: Track and report skipped eggs for better user feedback.Eggs with null
update_urlare silently skipped and not reflected in the success/failure counts. This could confuse users when the notification shows "Updated X out of Y" without explaining what happened to the remainder.For example, if 10 eggs are selected but 3 have null
update_url, the notification might show "Updated 7 out of 10" without clarifying that 3 were skipped (not failed). If all eggs have null URLs, it would show "Updated 0 out of N" with success status, which is misleading.Apply this diff to track and report skipped eggs:
$success = 0; $failed = 0; + $skipped = 0; /** @var Egg $egg */ foreach ($records as $egg) { if ($egg->update_url !== null) { try { $eggImporterService->fromUrl($egg->update_url, $egg); $success++; cache()->forget("eggs.$egg->uuid.update"); } catch (Exception $exception) { $failed++; report($exception); } + } else { + $skipped++; } } Notification::make() ->title(trans_choice('admin/egg.updated', 2, ['count' => $success, 'total' => $records->count()])) - ->body($failed > 0 ? trans('admin/egg.updated_failed', ['count' => $failed]) : null) + ->body( + collect([ + $failed > 0 ? trans('admin/egg.updated_failed', ['count' => $failed]) : null, + $skipped > 0 ? trans('admin/egg.updated_skipped', ['count' => $skipped]) : null, + ])->filter()->join(' ') + ) ->status($failed > 0 ? 'warning' : 'success') ->persistent() ->send();Note: You'll need to add the translation key
admin/egg.updated_skippedto your language files.
🧹 Nitpick comments (1)
app/Filament/Components/Actions/UpdateEggBulkAction.php (1)
55-67: Consider using early continue for better readability.The null check correctly prevents errors when
update_urlis null. However, using an earlycontinuewould reduce nesting depth and improve readability.Apply this diff to use early continue:
/** @var Egg $egg */ foreach ($records as $egg) { - if ($egg->update_url !== null) { - try { - $eggImporterService->fromUrl($egg->update_url, $egg); + if ($egg->update_url === null) { + continue; + } - $success++; + try { + $eggImporterService->fromUrl($egg->update_url, $egg); - cache()->forget("eggs.$egg->uuid.update"); - } catch (Exception $exception) { - $failed++; + $success++; - report($exception); - } + cache()->forget("eggs.$egg->uuid.update"); + } catch (Exception $exception) { + $failed++; + + report($exception); } }
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Filament/Components/Actions/UpdateEggBulkAction.php(1 hunks)lang/en/admin/egg.php(1 hunks)
🔇 Additional comments (3)
lang/en/admin/egg.php (1)
104-104: LGTM!The translation key follows the existing pattern and is appropriately placed with other update-related messages.
app/Filament/Components/Actions/UpdateEggBulkAction.php (2)
52-52: LGTM!The skipped counter initialization follows the same pattern as the existing success and failed counters.
56-70: Guard clause effectively prevents null URL errors.The logic correctly:
- Skips eggs without
update_urlinstead of attempting (and failing) to update them- Tracks skipped eggs separately from failures
- Clears cache only on successful updates
This addresses the PR objective of preventing error spam when
update_urlis null.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Filament/Components/Actions/UpdateEggBulkAction.php (1)
73-83: Consider handling the "all skipped" edge case for clearer UX.The current implementation works correctly, but there's a minor UX consideration: if all eggs are skipped (none have
update_url), the notification will show:
- Title: "0 updated out of X"
- Body: "X skipped"
- Status: success (green)
While technically correct, you might consider adding a check similar to lines 41-48 to provide more specific feedback when all eggs are skipped, such as an info-level notification stating no eggs had update URLs. This would make the outcome clearer to users.
Example implementation:
if ($success === 0 && $failed === 0 && $skipped > 0) { Notification::make() ->title(trans('admin/egg.all_skipped')) ->body(trans('admin/egg.all_skipped_description')) ->info() ->send(); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Filament/Components/Actions/UpdateEggBulkAction.php(2 hunks)
🔇 Additional comments (3)
app/Filament/Components/Actions/UpdateEggBulkAction.php (3)
52-52: LGTM! Counter initialization is correct.The
$skippedcounter is properly initialized alongside the existing$successand$failedcounters.
56-59: Excellent! This directly addresses the PR objective.The null check for
update_urlprevents attempting bulk updates on eggs without URLs, eliminating the error spam mentioned in the PR description. The logic is clean:
- Uses strict comparison (
===)- Increments the skipped counter
- Continues to the next egg without processing
75-80: LGTM! Notification body correctly includes skipped count.The notification body now properly displays both failed and skipped counts when applicable:
- Conditionally includes messages only when counts > 0
- Filters null values and joins messages with spaces
- Syntax is correct (closing parenthesis properly placed, as noted in previous review)
The user will see clear feedback about what happened during the bulk update.
4 participants
just checking if update_url is set, before doing bulk update
keeps it from spamming error messages from update_url=null