Add possibility to cancel server cancellation - #1355
Conversation
|
All contributors have signed the CLA! ✅ Thank you for taking the time to complete this step. We'll now proceed with reviewing your pull request. We appreciate your contribution to Ctrlpanel! 🙌 |
|
I have read and agree to the CLA. |
| Route::get('notifications/readAll', [NotificationController::class, 'readAll'])->name('notifications.readAll'); | ||
| Route::resource('notifications', NotificationController::class); | ||
| Route::patch('/servers/cancel/{server}', [ServerController::class, 'cancel'])->name('servers.cancel'); | ||
| Route::patch('/servers/{server}/uncancel', [App\Http\Controllers\ServerController::class, 'uncancel'])->name('servers.uncancel'); |
There was a problem hiding this comment.
- Why are you not following existing route naming? We have
/servers/cancel/:id, so why are you using/servers/:id/uncancel. But at the same time I can agree that/servers/:id/:actionformat is better from hierarchy point, so if you update old route to use same format it will be ok. - You're using full namespace while we already have ServerController class imported on L27
There was a problem hiding this comment.
Here's exactly the same style: /servers/{server}/billing_priority
There was a problem hiding this comment.
Here's exactly the same style: /servers/{server}/billing_priority
I'm not arguing that you chose a more correct format, but we're talking about uncancel route that does opposite of cancel so logically new route should follow its naming pattern OR old one should be adjusted to have same naming pattern as new and other routes.
MrWeez
left a comment
MrWeez
left a comment
There was a problem hiding this comment.
- No check of current server state before uncancel. If the server isn't currently canceled (duplicate click / race between two tabs),
uncancel()just silently setscanceled = nullagain. Not harmful, but also isn't good - Naming
uncancelalso stands out. It's a negation of a verb rather than an action verb itself, so it worth changing the method/route/name to something likeresume.servers.resume,ServerController::resume()reads and sound way better
MrWeez
changed the title
MrWeez
changed the title
MrWeez
left a comment
MrWeez
left a comment
There was a problem hiding this comment.
Except this one small thing everything is ok.
Special thanks for changing servers.cancel route, which was the only one that did not comply with RESTful standards in servers related part.
| use Exception; | ||
| use Illuminate\Database\Eloquent\Builder; | ||
| use Illuminate\Http\Client\Response; | ||
| use Illuminate\Http\Client\Response as ClientResponse; |
There was a problem hiding this comment.
This import isn't used anywhere in this file, so could be removed completely instead of importing it under different name
2 participants
This pull request improves the server uncancel (restore) workflow by introducing proper JSON response handling for API/AJAX requests and updating the UI logic across themes to fully support asynchronous operations and error states.
Backend: Improved Response Handling & Security
uncancelmethod inServerControllernow checks for JSON expectations and returns a204 No Contentresponse on success, or a500 JSONerror on failure.Frontend: Enhanced Uncancel Workflow & Theme Consistency
handleServerUncancelJavaScript function sending properAccept: application/jsonandX-Requested-Withheaders to trigger the new backend JSON logic.handleServerCancelwhere theresponsevariable was undefined in the callback.index.blade.php), ensuring seamless page reloads on success and explicit error alerts on failure.500 RouteNotFoundExceptionon the server settings page in the Phoenix theme by ensuring the template aligns with the defined routing structure.