feat: add-on types crud#558
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesSummitSponsorshipAddOnType CRUD Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant OAuth2SummitSponsorshipAddOnTypesApiController
participant SummitSponsorshipAddOnTypeService
participant ISummitSponsorshipAddOnTypeRepository
participant ISummitSponsorshipAddOnRepository
Client->>OAuth2SummitSponsorshipAddOnTypesApiController: POST /api/v1/summits/all/add-on-types
OAuth2SummitSponsorshipAddOnTypesApiController->>SummitSponsorshipAddOnTypeService: add(payload)
SummitSponsorshipAddOnTypeService->>ISummitSponsorshipAddOnTypeRepository: getByName(name)
SummitSponsorshipAddOnTypeService->>ISummitSponsorshipAddOnTypeRepository: add(type)
SummitSponsorshipAddOnTypeService-->>OAuth2SummitSponsorshipAddOnTypesApiController: created type
OAuth2SummitSponsorshipAddOnTypesApiController-->>Client: 201 response
Client->>OAuth2SummitSponsorshipAddOnTypesApiController: DELETE /api/v1/summits/all/add-on-types/{id}
OAuth2SummitSponsorshipAddOnTypesApiController->>SummitSponsorshipAddOnTypeService: delete(type_id)
SummitSponsorshipAddOnTypeService->>ISummitSponsorshipAddOnTypeRepository: getById(type_id)
SummitSponsorshipAddOnTypeService->>ISummitSponsorshipAddOnRepository: countByAddOnType(type_id)
SummitSponsorshipAddOnTypeService->>ISummitSponsorshipAddOnTypeRepository: delete(type)
OAuth2SummitSponsorshipAddOnTypesApiController-->>Client: 204 response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-558/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
database/migrations/model/Version20260615000001.php (1)
55-61: 💤 Low valueConsider making the seed INSERT idempotent.
The INSERT statement will fail if re-executed (e.g., after a partial migration failure) due to the unique constraint on
Name. While the constraint provides data integrity, usingINSERT IGNOREor anON DUPLICATE KEY UPDATEclause would make re-runs safer.♻️ Optional: Make INSERT idempotent
- $this->addSql(<<<SQL - INSERT INTO `SummitSponsorshipAddOnType` (`Created`, `LastEdited`, `Name`) VALUES - (NOW(), NOW(), 'Booth'), - (NOW(), NOW(), 'Meeting_Room'), - (NOW(), NOW(), 'Schedule_Spot'), - (NOW(), NOW(), 'Signage_Spot') - SQL); + $this->addSql(<<<SQL + INSERT IGNORE INTO `SummitSponsorshipAddOnType` (`Created`, `LastEdited`, `Name`) VALUES + (NOW(), NOW(), 'Booth'), + (NOW(), NOW(), 'Meeting_Room'), + (NOW(), NOW(), 'Schedule_Spot'), + (NOW(), NOW(), 'Signage_Spot') + SQL);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/migrations/model/Version20260615000001.php` around lines 55 - 61, The INSERT statement for SummitSponsorshipAddOnType in the addSql call will fail if the migration is re-executed due to the unique constraint on the Name column. Make the INSERT idempotent by adding either INSERT IGNORE or an ON DUPLICATE KEY UPDATE clause to the SQL statement so that re-running the migration after a partial failure does not cause the statement to fail, allowing the migration to proceed safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/Http/Controllers/Apis/Protected/Summit/Factories/SummitSponsorshipAddOnsValidationRulesFactory.php`:
- Around line 27-29: The validation rules for type_id and type fields in
SummitSponsorshipAddOnsValidationRulesFactory allow both fields to be submitted
simultaneously, creating ambiguous type resolution. To enforce mutual
exclusivity, add a `prohibits` validation rule (or equivalent mutual exclusion
constraint) at lines 27-29 so that type_id prohibits the presence of type, and
at lines 36-37 so that type prohibits the presence of type_id. This ensures only
one field can be provided per request, eliminating conflicting values and
non-deterministic behavior.
In
`@app/Http/Controllers/Apis/Protected/Summit/Factories/SummitSponsorshipAddOnTypeValidationRulesFactory.php`:
- Around line 30-34: The validation rule for the 'name' field in the
buildForUpdate method of SummitSponsorshipAddOnTypeValidationRulesFactory
currently permits empty strings because it only validates that the field is a
string when present. Add the 'filled' validation rule to the 'name' field
validation chain to ensure that when the name is provided, it must not be empty,
preventing blank lookup names from being persisted during updates.
In `@app/ModelSerializers/Summit/SummitSponsorshipAddOnTypeSerializer.php`:
- Around line 27-32: The serialize method in
SummitSponsorshipAddOnTypeSerializer is mutating the canonical name field by
replacing underscores with spaces on line 31, which prevents round-tripping of
the original stored value and breaks idempotent update/filter operations.
Instead of modifying the name value directly, create a separate display field
(such as name_display or similar) to hold the transformed version with spaces,
leaving the canonical name field unchanged so it reflects the actual stored
value in the database.
In `@app/Repositories/Summit/DoctrineSummitSponsorshipAddOnRepository.php`:
- Around line 85-88: The getMetadata() method in
DoctrineSummitSponsorshipAddOnRepository currently returns an empty array but
should return populated metadata following the pattern used in similar
repositories. Examine how DoctrineSummitRegistrationPromoCodeRepository and
DoctrineSummitLocationRepository implement their getMetadata() methods—they call
Model::getMetadata() on their related entity types to populate the array. Apply
the same approach to the getMetadata() method in
DoctrineSummitSponsorshipAddOnRepository by calling getMetadata() on
SummitSponsorshipAddOn and/or SummitSponsorshipAddOnType entity classes and
returning the resulting metadata array instead of an empty array.
In `@app/Services/Model/ISummitSponsorshipAddOnTypeService.php`:
- Around line 41-45: The PHPDoc comment for the delete method in the
ISummitSponsorshipAddOnTypeService interface only documents that it throws
EntityNotFoundException, but the implementation also throws ValidationException
when the type is in use. Add a `@throws` ValidationException line to the PHPDoc
comment block of the delete method to fully document all exceptions that callers
need to handle.
In `@app/Services/ModelServicesProvider.php`:
- Around line 351-354: Since ModelServicesProvider is a deferred provider with
`$defer = true`, you must declare all service bindings registered in the
`register()` method within the `provides()` method as well. Add
`ISummitSponsorshipAddOnTypeService::class` to the `provides()` method array in
ModelServicesProvider so that Laravel can properly resolve this binding at
runtime without failures.
In `@tests/oauth2/OAuth2SummitSponsorshipAddOnTypesApiControllerTest.php`:
- Around line 41-57: The tearDown method only removes self::$default_type but
does not clean up add-on types created by testAdd(), causing test data leaks.
Add a class-level collection variable (such as self::$created_types) to track
all SummitSponsorshipAddOnType instances created during test execution. In
testAdd() and any other test methods that create add-on types, append the newly
created type to this tracking collection after persistence. Then in tearDown(),
iterate through all tracked add-on types in the collection and remove each one
from the entity manager before clearing the collection, ensuring complete
cleanup of all test-created data alongside the existing self::$default_type
cleanup.
---
Nitpick comments:
In `@database/migrations/model/Version20260615000001.php`:
- Around line 55-61: The INSERT statement for SummitSponsorshipAddOnType in the
addSql call will fail if the migration is re-executed due to the unique
constraint on the Name column. Make the INSERT idempotent by adding either
INSERT IGNORE or an ON DUPLICATE KEY UPDATE clause to the SQL statement so that
re-running the migration after a partial failure does not cause the statement to
fail, allowing the migration to proceed safely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 024ec028-2fd4-4c04-a1b6-c185cf3a1ae0
📒 Files selected for processing (28)
app/Http/Controllers/Apis/Protected/Summit/Factories/SummitSponsorshipAddOnTypeValidationRulesFactory.phpapp/Http/Controllers/Apis/Protected/Summit/Factories/SummitSponsorshipAddOnsValidationRulesFactory.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorshipAddOnTypesApiController.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorshipsApiController.phpapp/ModelSerializers/SerializerRegistry.phpapp/ModelSerializers/Summit/SummitSponsorshipAddOnSerializer.phpapp/ModelSerializers/Summit/SummitSponsorshipAddOnTypeSerializer.phpapp/Models/Foundation/Summit/Factories/SummitSponsorshipAddOnFactory.phpapp/Models/Foundation/Summit/Factories/SummitSponsorshipAddOnTypeFactory.phpapp/Models/Foundation/Summit/Repositories/ISummitSponsorshipAddOnRepository.phpapp/Models/Foundation/Summit/Repositories/ISummitSponsorshipAddOnTypeRepository.phpapp/Models/Foundation/Summit/SummitSponsorshipAddOn.phpapp/Models/Foundation/Summit/SummitSponsorshipAddOnType.phpapp/Repositories/RepositoriesProvider.phpapp/Repositories/Summit/DoctrineSummitSponsorshipAddOnRepository.phpapp/Repositories/Summit/DoctrineSummitSponsorshipAddOnTypeRepository.phpapp/Services/Model/ISummitSponsorshipAddOnTypeService.phpapp/Services/Model/Imp/SummitSponsorshipAddOnTypeService.phpapp/Services/Model/Imp/SummitSponsorshipService.phpapp/Services/ModelServicesProvider.phpdatabase/migrations/config/Version20260615000000.phpdatabase/migrations/model/Version20260615000000.phpdatabase/migrations/model/Version20260615000001.phpdatabase/seeders/ApiEndpointsSeeder.phproutes/api_v1.phptests/InsertSummitTestData.phptests/oauth2/OAuth2SummitSponsorshipAddOnTypesApiControllerTest.phptests/oauth2/OAuth2SummitSponsorshipApiControllerTest.php
💤 Files with no reviewable changes (1)
- app/Models/Foundation/Summit/Factories/SummitSponsorshipAddOnFactory.php
Comment on lines
85
to
+88
| public function getMetadata(Summit $summit): array | ||
| { | ||
| return SummitSponsorshipAddOn::getMetadata(); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find getMetadata callsites and inspect expectations.
rg -n -C4 '\bgetMetadata\s*\(' app tests
# Narrow to sponsorship add-on repository references.
rg -n -C4 'ISummitSponsorshipAddOnRepository|DoctrineSummitSponsorshipAddOnRepository' app testsRepository: OpenStackweb/summit-api
Length of output: 50379
🏁 Script executed:
# Search for method calls to getMetadata on sponsorship add-on repository or interface
rg -n 'ISummitSponsorshipAddOnRepository|sponsorshipAddOn.*getMetadata|getMetadata.*sponsorship' app tests --type php
# Search for services/controllers that use ISummitSponsorshipAddOnRepository
rg -n -A5 -B5 'ISummitSponsorshipAddOnRepository' app/Services app/Http --type php
# Look for API endpoints that expose sponsorship add-on metadata
rg -n -i 'sponsorship.*metadata|metadata.*sponsorship' app/Http --type phpRepository: OpenStackweb/summit-api
Length of output: 8715
🏁 Script executed:
# Get context around line 742 in the controller to see how getMetadata is used
sed -n '730,760p' app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorshipsApiController.php | cat -nRepository: OpenStackweb/summit-api
Length of output: 898
🏁 Script executed:
# Check if SummitSponsorshipAddOn model has a getMetadata method
rg -n 'class SummitSponsorshipAddOn' app/Models --type php
# Find the model and check for metadata pattern
find app/Models -name '*SponsorshipAddOn*' -type f
# Search for tests of this endpoint
rg -n 'getMetadata.*sponsorship|getSponsorshipAddOn.*etadata' tests --type phpRepository: OpenStackweb/summit-api
Length of output: 985
🏁 Script executed:
# Check the SummitSponsorshipAddOn model for getMetadata method
rg -n 'getMetadata|public static' app/Models/Foundation/Summit/SummitSponsorshipAddOn.php -A3
# Check for tests of the sponsorship endpoint
rg -n 'testGetMetadata|getMetadata.*sponsorship' tests --type php
# Look at the full SummitSponsorshipAddOn model to understand its structure
wc -l app/Models/Foundation/Summit/SummitSponsorshipAddOn.phpRepository: OpenStackweb/summit-api
Length of output: 331
🏁 Script executed:
# Read the SummitSponsorshipAddOn model to understand its structure
cat -n app/Models/Foundation/Summit/SummitSponsorshipAddOn.php
# Also check if there are any tests for the sponsorship add-on getMetadata endpoint
rg -n 'SponsorshipAddOn.*etadata|sponsorshipAddOn.*getMetadata' tests --type php -A5Repository: OpenStackweb/summit-api
Length of output: 3492
🏁 Script executed:
# Search for tests that call the sponsorship add-on metadata endpoint
rg -n 'getSponsorshipAddOns.*Metadata|sponsorship.*add.*on.*metadata' tests --type php -i -A5
# Check the complete test file for the controller to see if there are any metadata tests
rg -n 'OAuth2SummitSponsorshipsApiTest|testGetMetadata' tests --type php -l
# Check if such a test file exists
find tests -name '*SponsorshipAddOn*' -o -name '*Sponsorship*Api*Test*' | head -10Repository: OpenStackweb/summit-api
Length of output: 427
🏁 Script executed:
# Check the OAuth2SummitSponsorshipApiControllerTest for getMetadata tests
rg -n 'getMetadata|testGet' tests/oauth2/OAuth2SummitSponsorshipApiControllerTest.php -A10 | head -50Repository: OpenStackweb/summit-api
Length of output: 1847
🏁 Script executed:
# Get the full testGetAddsMetadata test
sed -n '305,330p' tests/oauth2/OAuth2SummitSponsorshipApiControllerTest.php | cat -nRepository: OpenStackweb/summit-api
Length of output: 755
Add sponsorship add-on metadata to match pattern of other repository implementations.
The getMetadata() method currently returns an empty array, but the API endpoint is documented as returning "Add-ons metadata" and the test only verifies it's not null, not that it contains expected metadata keys. Similar repositories (DoctrineSummitRegistrationPromoCodeRepository, DoctrineSummitLocationRepository) return populated metadata arrays by calling Model::getMetadata() on related entity types. Add metadata for SummitSponsorshipAddOn and/or SummitSponsorshipAddOnType following the established pattern to provide consumers with the schema of available add-on attributes.
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 85-85: Avoid unused parameters such as '$summit'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Repositories/Summit/DoctrineSummitSponsorshipAddOnRepository.php` around
lines 85 - 88, The getMetadata() method in
DoctrineSummitSponsorshipAddOnRepository currently returns an empty array but
should return populated metadata following the pattern used in similar
repositories. Examine how DoctrineSummitRegistrationPromoCodeRepository and
DoctrineSummitLocationRepository implement their getMetadata() methods—they call
Model::getMetadata() on their related entity types to populate the array. Apply
the same approach to the getMetadata() method in
DoctrineSummitSponsorshipAddOnRepository by calling getMetadata() on
SummitSponsorshipAddOn and/or SummitSponsorshipAddOnType entity classes and
returning the resulting metadata array instead of an empty array.
romanetar
force-pushed
the
feature/add-on-types-crud
branch
from
f34a772 to
de1edb8
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-558/ This page is automatically updated on each push to this PR. |
romanetar
force-pushed
the
feature/add-on-types-crud
branch
from
de1edb8 to
32ec9ba
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-558/ This page is automatically updated on each push to this PR. |
romanetar
force-pushed
the
feature/add-on-types-crud
branch
from
32ec9ba to
4dd14e8
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-558/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/Http/Controllers/Apis/Protected/Summit/Factories/SummitSponsorshipAddOnTypeValidationRulesFactory.php (1)
30-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject blank
namevalues on update.Line 33 uses
sometimes|string|max:255, soname: ""is accepted when present. That leaves this layer allowing empty lookup names.Suggested fix
- 'name' => 'sometimes|string|max:255', + 'name' => 'sometimes|filled|string|max:255',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Http/Controllers/Apis/Protected/Summit/Factories/SummitSponsorshipAddOnTypeValidationRulesFactory.php` around lines 30 - 34, The validation rule for the name field in the buildForUpdate method of SummitSponsorshipAddOnTypeValidationRulesFactory is missing a check to reject empty string values. The current rule sometimes|string|max:255 allows blank names to pass validation. Add the filled validation rule to ensure that when the name field is present in the payload, it cannot be an empty string. Update the rule to include filled alongside the existing validators.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@app/Http/Controllers/Apis/Protected/Summit/Factories/SummitSponsorshipAddOnTypeValidationRulesFactory.php`:
- Around line 30-34: The validation rule for the name field in the
buildForUpdate method of SummitSponsorshipAddOnTypeValidationRulesFactory is
missing a check to reject empty string values. The current rule
sometimes|string|max:255 allows blank names to pass validation. Add the filled
validation rule to ensure that when the name field is present in the payload, it
cannot be an empty string. Update the rule to include filled alongside the
existing validators.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3178fecb-bdd5-47b1-89ec-085ef20189df
📒 Files selected for processing (29)
app/Http/Controllers/Apis/Protected/Summit/Factories/SummitSponsorshipAddOnTypeValidationRulesFactory.phpapp/Http/Controllers/Apis/Protected/Summit/Factories/SummitSponsorshipAddOnsValidationRulesFactory.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorshipAddOnTypesApiController.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorshipsApiController.phpapp/ModelSerializers/SerializerRegistry.phpapp/ModelSerializers/Summit/SummitSponsorshipAddOnSerializer.phpapp/ModelSerializers/Summit/SummitSponsorshipAddOnTypeSerializer.phpapp/Models/Foundation/Summit/Factories/SummitSponsorshipAddOnFactory.phpapp/Models/Foundation/Summit/Factories/SummitSponsorshipAddOnTypeFactory.phpapp/Models/Foundation/Summit/Repositories/ISummitSponsorshipAddOnRepository.phpapp/Models/Foundation/Summit/Repositories/ISummitSponsorshipAddOnTypeRepository.phpapp/Models/Foundation/Summit/SummitSponsorshipAddOn.phpapp/Models/Foundation/Summit/SummitSponsorshipAddOnType.phpapp/Repositories/RepositoriesProvider.phpapp/Repositories/Summit/DoctrineSummitSponsorshipAddOnRepository.phpapp/Repositories/Summit/DoctrineSummitSponsorshipAddOnTypeRepository.phpapp/Services/Model/ISummitSponsorshipAddOnTypeService.phpapp/Services/Model/Imp/SummitSponsorshipAddOnTypeService.phpapp/Services/Model/Imp/SummitSponsorshipService.phpapp/Services/ModelServicesProvider.phpdatabase/migrations/config/Version20260615000000.phpdatabase/migrations/model/Version20260615000000.phpdatabase/migrations/model/Version20260615000001.phpdatabase/seeders/ApiEndpointsSeeder.phproutes/api_v1.phptests/InsertSummitTestData.phptests/Unit/Entities/SummitSponsorshipTest.phptests/oauth2/OAuth2SummitSponsorshipAddOnTypesApiControllerTest.phptests/oauth2/OAuth2SummitSponsorshipApiControllerTest.php
💤 Files with no reviewable changes (1)
- app/Models/Foundation/Summit/Factories/SummitSponsorshipAddOnFactory.php
🚧 Files skipped from review as they are similar to previous changes (21)
- app/ModelSerializers/Summit/SummitSponsorshipAddOnTypeSerializer.php
- app/Repositories/Summit/DoctrineSummitSponsorshipAddOnTypeRepository.php
- app/Models/Foundation/Summit/Factories/SummitSponsorshipAddOnTypeFactory.php
- app/Models/Foundation/Summit/SummitSponsorshipAddOnType.php
- app/ModelSerializers/SerializerRegistry.php
- app/Models/Foundation/Summit/Repositories/ISummitSponsorshipAddOnTypeRepository.php
- app/Services/Model/ISummitSponsorshipAddOnTypeService.php
- tests/oauth2/OAuth2SummitSponsorshipApiControllerTest.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorshipsApiController.php
- app/Models/Foundation/Summit/Repositories/ISummitSponsorshipAddOnRepository.php
- database/migrations/model/Version20260615000000.php
- app/Repositories/Summit/DoctrineSummitSponsorshipAddOnRepository.php
- tests/InsertSummitTestData.php
- app/ModelSerializers/Summit/SummitSponsorshipAddOnSerializer.php
- database/seeders/ApiEndpointsSeeder.php
- app/Models/Foundation/Summit/SummitSponsorshipAddOn.php
- app/Services/Model/Imp/SummitSponsorshipService.php
- app/Services/Model/Imp/SummitSponsorshipAddOnTypeService.php
- routes/api_v1.php
- database/migrations/model/Version20260615000001.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorshipAddOnTypesApiController.php
romanetar
force-pushed
the
feature/add-on-types-crud
branch
from
4dd14e8 to
69ddb58
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/Http/Controllers/Apis/Protected/Summit/Factories/SummitSponsorshipAddOnsValidationRulesFactory.php (1)
24-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMutual-exclusion check is currently unreachable from the controller path.
On Line 24 and Line 36, the conflict check uses
$payload, but upstream calls build rules with no argument, so the throw path never runs. Enforce exclusivity in the rule strings themselves so it always applies duringgetJsonPayload(...)validation.Suggested fix
public static function buildForAdd(array $payload = []): array { - if (isset($payload['type_id']) && isset($payload['type'])) - throw new ValidationException("type_id and type are mutually exclusive."); - return [ 'name' => 'required|string|max:255', - 'type_id' => 'required_without:type|integer', - 'type' => 'required_without:type_id|string|max:255', + 'type_id' => 'required_without:type|integer|prohibited_with:type', + 'type' => 'required_without:type_id|string|max:255|prohibited_with:type_id', ]; } public static function buildForUpdate(array $payload = []): array { - if (isset($payload['type_id']) && isset($payload['type'])) - throw new ValidationException("type_id and type are mutually exclusive."); - return [ 'name' => 'sometimes|string|max:255', - 'type_id' => 'sometimes|integer', - 'type' => 'sometimes|string|max:255', + 'type_id' => 'sometimes|integer|prohibited_with:type', + 'type' => 'sometimes|string|max:255|prohibited_with:type_id', ]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Http/Controllers/Apis/Protected/Summit/Factories/SummitSponsorshipAddOnsValidationRulesFactory.php` around lines 24 - 46, The mutual-exclusion checks in buildForAdd and buildForUpdate methods are unreachable because these methods are called without arguments upstream, making $payload empty and causing the isset conditions to never trigger. Remove the ValidationException checks from both methods and instead enforce the mutual exclusivity directly in the validation rule strings using Laravel's conditional validation rules such as exclude_if or similar mechanisms. This ensures the exclusivity constraint is always validated during the getJsonPayload validation phase regardless of how the methods are called.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@app/Http/Controllers/Apis/Protected/Summit/Factories/SummitSponsorshipAddOnsValidationRulesFactory.php`:
- Around line 24-46: The mutual-exclusion checks in buildForAdd and
buildForUpdate methods are unreachable because these methods are called without
arguments upstream, making $payload empty and causing the isset conditions to
never trigger. Remove the ValidationException checks from both methods and
instead enforce the mutual exclusivity directly in the validation rule strings
using Laravel's conditional validation rules such as exclude_if or similar
mechanisms. This ensures the exclusivity constraint is always validated during
the getJsonPayload validation phase regardless of how the methods are called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05b9d066-ef6e-4605-b611-e12876436d56
📒 Files selected for processing (29)
app/Http/Controllers/Apis/Protected/Summit/Factories/SummitSponsorshipAddOnTypeValidationRulesFactory.phpapp/Http/Controllers/Apis/Protected/Summit/Factories/SummitSponsorshipAddOnsValidationRulesFactory.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorshipAddOnTypesApiController.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorshipsApiController.phpapp/ModelSerializers/SerializerRegistry.phpapp/ModelSerializers/Summit/SummitSponsorshipAddOnSerializer.phpapp/ModelSerializers/Summit/SummitSponsorshipAddOnTypeSerializer.phpapp/Models/Foundation/Summit/Factories/SummitSponsorshipAddOnFactory.phpapp/Models/Foundation/Summit/Factories/SummitSponsorshipAddOnTypeFactory.phpapp/Models/Foundation/Summit/Repositories/ISummitSponsorshipAddOnRepository.phpapp/Models/Foundation/Summit/Repositories/ISummitSponsorshipAddOnTypeRepository.phpapp/Models/Foundation/Summit/SummitSponsorshipAddOn.phpapp/Models/Foundation/Summit/SummitSponsorshipAddOnType.phpapp/Repositories/RepositoriesProvider.phpapp/Repositories/Summit/DoctrineSummitSponsorshipAddOnRepository.phpapp/Repositories/Summit/DoctrineSummitSponsorshipAddOnTypeRepository.phpapp/Services/Model/ISummitSponsorshipAddOnTypeService.phpapp/Services/Model/Imp/SummitSponsorshipAddOnTypeService.phpapp/Services/Model/Imp/SummitSponsorshipService.phpapp/Services/ModelServicesProvider.phpdatabase/migrations/config/Version20260615000000.phpdatabase/migrations/model/Version20260615000000.phpdatabase/migrations/model/Version20260615000001.phpdatabase/seeders/ApiEndpointsSeeder.phproutes/api_v1.phptests/InsertSummitTestData.phptests/Unit/Entities/SummitSponsorshipTest.phptests/oauth2/OAuth2SummitSponsorshipAddOnTypesApiControllerTest.phptests/oauth2/OAuth2SummitSponsorshipApiControllerTest.php
💤 Files with no reviewable changes (1)
- app/Models/Foundation/Summit/Factories/SummitSponsorshipAddOnFactory.php
✅ Files skipped from review due to trivial changes (1)
- app/Models/Foundation/Summit/Repositories/ISummitSponsorshipAddOnTypeRepository.php
🚧 Files skipped from review as they are similar to previous changes (24)
- app/Repositories/Summit/DoctrineSummitSponsorshipAddOnTypeRepository.php
- app/Models/Foundation/Summit/Factories/SummitSponsorshipAddOnTypeFactory.php
- app/ModelSerializers/SerializerRegistry.php
- app/Models/Foundation/Summit/Repositories/ISummitSponsorshipAddOnRepository.php
- app/ModelSerializers/Summit/SummitSponsorshipAddOnSerializer.php
- app/Repositories/RepositoriesProvider.php
- tests/Unit/Entities/SummitSponsorshipTest.php
- app/Services/Model/ISummitSponsorshipAddOnTypeService.php
- app/ModelSerializers/Summit/SummitSponsorshipAddOnTypeSerializer.php
- app/Services/ModelServicesProvider.php
- tests/oauth2/OAuth2SummitSponsorshipApiControllerTest.php
- database/migrations/model/Version20260615000000.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorshipsApiController.php
- tests/oauth2/OAuth2SummitSponsorshipAddOnTypesApiControllerTest.php
- app/Services/Model/Imp/SummitSponsorshipAddOnTypeService.php
- app/Repositories/Summit/DoctrineSummitSponsorshipAddOnRepository.php
- database/seeders/ApiEndpointsSeeder.php
- routes/api_v1.php
- app/Services/Model/Imp/SummitSponsorshipService.php
- app/Models/Foundation/Summit/SummitSponsorshipAddOn.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorshipAddOnTypesApiController.php
- tests/InsertSummitTestData.php
- app/Models/Foundation/Summit/SummitSponsorshipAddOnType.php
- database/migrations/model/Version20260615000001.php
smarcet
reviewed
Jun 24, 2026
smarcet
requested changes
Jun 24, 2026
smarcet
left a comment
smarcet
left a comment
Collaborator
There was a problem hiding this comment.
@romanetar please review
Signed-off-by: romanetar <roman_ag@hotmail.com>
Signed-off-by: romanetar <roman_ag@hotmail.com>
romanetar
force-pushed
the
feature/add-on-types-crud
branch
from
69ddb58 to
591a7a1
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-558/ This page is automatically updated on each push to this PR. |
smarcet
reviewed
Jun 25, 2026
smarcet
requested changes
Jun 25, 2026
smarcet
left a comment
smarcet
left a comment
Collaborator
There was a problem hiding this comment.
@romanetar please review
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-558/ This page is automatically updated on each push to this PR. |
Signed-off-by: romanetar <roman_ag@hotmail.com>
romanetar
force-pushed
the
feature/add-on-types-crud
branch
from
ddff204 to
58cacc2
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-558/ This page is automatically updated on each push to this PR. |
smarcet
pushed a commit
that referenced
this pull request
Jun 26, 2026
* feat: add-on types crud Signed-off-by: romanetar <roman_ag@hotmail.com> * fix: review feedback Signed-off-by: romanetar <roman_ag@hotmail.com> * fix: open api fix Signed-off-by: romanetar <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref https://app.clickup.com/t/86b9rdth6
Summary by CodeRabbit
Release Notes
/api/v1/summits/all/add-on-types(OAuth2-protected: list, create, retrieve by id, update, delete).type_id(with optionaltypeexpansion); add-on listings return onlyidandname, with updated filter/sort behavior.type_idandtypetogether.