-
Notifications
You must be signed in to change notification settings - Fork 0
controller refactor #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
AZabolotnikov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why PR name "refactor: test"?
Task about controller refactoring.
Please rename PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This refactor introduces form request validation and API resource classes to improve structure and validation handling for the ExpoController endpoints. The changes replace manual validation and JSON response creation with Laravel's built-in form request validation and API resources.
- Adds form request classes (SubscribeRequest, UnsubscribeRequest) with validation rules
- Introduces API resource classes to standardize response formatting
- Updates ExpoController to use form requests and resources instead of manual validation
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ExpoControllerTest.php | Updates test mocks to use specific request classes instead of generic Request |
| src/Http/Resources/*.php | Adds new API resource classes for subscribe, unsubscribe, validation errors, and base functionality |
| src/Http/Requests/*.php | Adds form request classes with validation rules and custom error handling |
| src/Http/ExpoController.php | Refactors controller methods to use form requests and API resources |
| composer.json | Adds dependency on ronasit/laravel-helpers package |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Pull Request Test Coverage Report for Build 19215259525Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
migrations/create_exponent_push_notification_interests_table.php.stub:26
- The
up()anddown()methods are missing return type declarations. For consistency with modern Laravel conventions and the test migration attests/database/migrations/2014_10_12_000000_create_users_table.php(lines 9 and 18), these methods should declare: voidas their return type.
public function up()
{
Schema::create(config('exponent-push-notifications.interests.database.table_name'), function (Blueprint $table) {
$table->increments('id');
$table->string('key')->index();
$table->string('value');
$table->unique(['key','value']);
});
}
/**
* Reverse the migrations.
*/
public function down()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../database/migrations/2014_10_12_000000_create_exponent_push_notification_interests_table.php
Outdated
Show resolved
Hide resolved
.../database/migrations/2014_10_12_000000_create_exponent_push_notification_interests_table.php
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,24 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| $table->string('email')->unique(); | ||
| $table->timestamps(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure we need extra fields in the users table for tests
| $table->string('email')->unique(); | |
| $table->timestamps(); |
| protected $table = 'users'; | ||
|
|
||
| use Notifiable; | ||
| use ModelTrait; | ||
| use HasFactory; | ||
|
|
||
| protected $fillable = [ | ||
| 'email', | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need it for the just abstract class?
#10