-
Notifications
You must be signed in to change notification settings - Fork 72
add clone application #1848
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
add clone application #1848
Conversation
WalkthroughAdds a cloning feature for applications: a "Clone" button on an application's page routes to a new controller action that loads reference data, merges the source application's fillable attributes into the request, flashes input, and returns the create form prefilled. Changes
Sequence Diagram(s)sequenceDiagram
%% Styling note: new/changed interactions highlighted via notes
participant User as User (Browser)
participant App as Web App (Router/Controller)
participant DB as Database
participant Icon as IconUploadService
participant View as Blade View
Note over User,App `#DDEBF7`: User clicks "Clone" button
User->>App: GET /applications-clone/{id}
App->>DB: fetch MApplication by id
DB-->>App: MApplication record
App->>App: merge fillable attrs into Request (flash input)
App->>Icon: (constructor available) prepare icons list
Icon-->>App: icons data
App->>View: render create view with prefilled data
View-->>User: HTML form prefilled from source application
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)app/Http/Controllers/Admin/MApplicationController.php (11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
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 |
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.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @app/Http/Controllers/Admin/MApplicationController.php:
- Line 105: Replace the incorrect comment "Workstation not found" with the
correct text "Application not found" in the MApplicationController source (the
comment near the error-handling block around line where the application lookup
fails), ensuring the comment accurately reflects that the Application was not
found.
- Around line 44-46: The clone method currently accepts Request $request but
retrieves the ID from $request['id'], which fails because the route passes the
ID as a route parameter (applications-clone/{id}); change the
MApplicationController::clone signature to accept the route parameter (e.g.,
clone($id) or clone(MApplication $application) for route model binding) and
replace uses of $request['id'] with the route variable ($id or
$application->id), or alternatively update the route to use model binding
(applications-clone/{application}) to match the controller change.
🧹 Nitpick comments (2)
app/Http/Controllers/Admin/MApplicationController.php (1)
108-109: Consider pre-populating relationships when cloning.The clone method merges the application's fillable attributes but doesn't pre-populate the many-to-many relationships (entities, processes, activities, services, databases, logical_servers, security_devices, administrators). Users would likely expect these relationships to carry over when cloning an application.
💡 Suggested enhancement
After merging fillable attributes, also merge the relationship IDs:
$request->merge($application->only($application->getFillable())); +$request->merge([ + 'entities' => $application->entities()->pluck('id')->toArray(), + 'processes' => $application->processes()->pluck('id')->toArray(), + 'activities' => $application->activities()->pluck('id')->toArray(), + 'services' => $application->services()->pluck('id')->toArray(), + 'databases' => $application->databases()->pluck('id')->toArray(), + 'logical_servers' => $application->logicalServers()->pluck('id')->toArray(), + 'security_devices' => $application->securityDevices()->pluck('id')->toArray(), + 'administrators' => $application->administrators()->pluck('id')->toArray(), +]); $request->flash();This pattern is used in VlanController::clone (see relevant code snippets) where subnetworks are merged into the request.
routes/web.php (1)
108-108: Route pattern inconsistency with similar resources.The new clone route uses the pattern
applications-clone/{id}, which is consistent with some physical infrastructure resources (physical-servers, workstations) but inconsistent with other domain resources like sites, buildings, and bays that use route model binding:{resource}/{model}/clone.While the current pattern works, using route model binding would be more idiomatic and consistent with the application's logical domain resources.
♻️ Optional refactoring for consistency
- Route::get('applications-clone/{id}', [Admin\MApplicationController::class, 'clone'])->name('applications.clone'); + Route::get('applications/{application}/clone', [Admin\MApplicationController::class, 'clone'])->name('applications.clone');This would require updating the controller method signature as mentioned in the controller review.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
app/Http/Controllers/Admin/MApplicationController.phpresources/views/admin/applications/show.blade.phproutes/web.php
🧰 Additional context used
🧬 Code graph analysis (2)
app/Http/Controllers/Admin/MApplicationController.php (4)
app/Services/IconUploadService.php (1)
IconUploadService(10-34)app/Http/Controllers/Admin/SiteController.php (1)
clone(38-52)app/Http/Controllers/Admin/PhoneController.php (1)
clone(42-64)app/Http/Controllers/Admin/VlanController.php (1)
clone(27-44)
routes/web.php (1)
app/Http/Controllers/Admin/MApplicationController.php (1)
MApplicationController(26-406)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
resources/views/admin/applications/show.blade.php (1)
17-21: LGTM!The Clone button implementation is correct:
- Properly gated by the
m_application_createpermission- Follows the existing UI pattern for action buttons
- Correctly routes to the new clone endpoint with the application ID
app/Http/Controllers/Admin/MApplicationController.php (1)
28-28: Constructor injection added for IconUploadService.The IconUploadService is now injected via the constructor. This service is used in both
storeandupdatemethods for handling icon uploads during application creation and modification.
| // Get Application to clone | ||
| $application = MApplication::query()->find($request['id']); | ||
|
|
||
| // Workstation not found |
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.
Fix copy-paste error in comment.
The comment says "Workstation not found" but should say "Application not found".
📝 Proposed fix
- // Workstation not found
+ // Application not found
abort_if($application === null, Response::HTTP_NOT_FOUND, '404 Not Found');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Workstation not found | |
| // Application not found | |
| abort_if($application === null, Response::HTTP_NOT_FOUND, '404 Not Found'); |
🤖 Prompt for AI Agents
In @app/Http/Controllers/Admin/MApplicationController.php at line 105, Replace
the incorrect comment "Workstation not found" with the correct text "Application
not found" in the MApplicationController source (the comment near the
error-handling block around line where the application lookup fails), ensuring
the comment accurately reflects that the Application was not found.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.