Fix/server cancellation revoke#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.
There was a problem hiding this comment.
Why are you removed all comments? They can be useful. Leaving comments in the code is a good practice so that feature devs can easily understand code/function meaning without reading it
There was a problem hiding this comment.
It's very rare for contributors to leave comments in code. You do not write such remarks to them, so, please do not write to me too
There was a problem hiding this comment.
I'm not asking you to leave comments, I'm asking you not to delete existing ones without reason
There was a problem hiding this comment.
Please tell me, is this comment in the right place? ("delete server" in the logic of the cancel button)
There was a problem hiding this comment.
As for the full namespace of the controller, I agree, I will correct for a short class
There was a problem hiding this comment.
Please tell me, is this comment in the right place? ("delete server" in the logic of the cancel button)
I agree that the comment is incorrect, and was probably just copied from the delete method and forgotten to be renamed. But is that a reason to delete it? Why not just fix it if you notice a typo instead of deleting it?
There was a problem hiding this comment.
You mentioned not deleting comments without a reason. The reason here is that the comment was completely wrong and misleading (stating "Delete" instead of "Cancel")
The code fetch(route('servers.cancel', ...)) is already 100% self-explanatory and doesn't need a comment to explain what it does. Keeping obvious or broken comments just litters the codebase
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.