Open
Conversation
bjerrecs
added
back-end
enhancement
Contributor
Reviewer's GuideImplements an admin-manageable Training Report Template system (with per-area scoping and draft/published states) and integrates it into the training report creation flow so instructors can prefill report fields from selected templates. Sequence diagram for training report creation with template prefillsequenceDiagram
actor Instructor
participant Browser
participant TrainingReportController
participant TrainingReportTemplate
participant Training
Instructor->>Browser: Open training report create URL
Browser->>TrainingReportController: GET create(training_id)
TrainingReportController->>Training: find(training_id)
TrainingReportController->>TrainingReportTemplate: query published templates scoped to training.area_id
TrainingReportTemplate-->>TrainingReportController: templates collection
TrainingReportController-->>Browser: Render training.report.create with templates
Instructor->>Browser: Choose template from templateSelect
Browser->>Browser: JS reads selectedOption.dataset.content and contentimprove
alt current content empty or user confirms replacement
Browser->>Browser: simplemde1.value(template.content)
Browser->>Browser: simplemde2.value(template.contentimprove)
else user cancels
Browser-->>Browser: Keep existing editor content
end
ER diagram for training report templates and area mappingerDiagram
AREAS {
int id PK
string name
string other_columns
}
TRAINING_REPORT_TEMPLATES {
int id PK
string name
text content
text contentimprove
boolean draft
timestamp created_at
timestamp updated_at
}
TRAINING_REPORT_TEMPLATE_AREA {
int id PK
int training_report_template_id FK
int area_id FK
timestamp created_at
timestamp updated_at
string unique_training_report_template_id_area_id
}
TRAININGS {
int id PK
int area_id FK
string other_columns
}
AREAS ||--o{ TRAININGS : has
AREAS ||--o{ TRAINING_REPORT_TEMPLATE_AREA : maps
TRAINING_REPORT_TEMPLATES ||--o{ TRAINING_REPORT_TEMPLATE_AREA : maps
Class diagram for TrainingReportTemplate management and area scopingclassDiagram
class Area {
+int id
+string name
+positions()
+trainingReportTemplates()
}
class TrainingReportTemplate {
+int id
+string name
+text content
+text contentimprove
+bool draft
+timestamps
+areas()
}
class TrainingReportController {
+create(Request request, Training training)
}
class TrainingReportTemplateController {
+index()
+create()
+store(Request request)
+edit(TrainingReportTemplate template)
+update(Request request, TrainingReportTemplate template)
+destroy(TrainingReportTemplate template)
}
class TrainingReportTemplatePolicy {
+viewAny(User user) bool
+view(User user, TrainingReportTemplate template) bool
+create(User user) bool
+update(User user, TrainingReportTemplate template) bool
+delete(User user, TrainingReportTemplate template) bool
}
class User {
+bool isModeratorOrAbove()
}
Area "1" --> "*" Position
Area "*" -- "*" TrainingReportTemplate : training_report_template_area
TrainingReportController ..> TrainingReportTemplate : uses
TrainingReportController ..> Training : uses
TrainingReportTemplateController ..> TrainingReportTemplate : manages
TrainingReportTemplateController ..> Area : reads
TrainingReportTemplatePolicy ..> TrainingReportTemplate : authorizes
TrainingReportTemplatePolicy ..> User : checks_role
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The new
trainingReportTemplates()relation onAreareferencesTrainingReportTemplatewithout auseimport inArea.php, which will cause a class resolution error when the relation is used; consider addinguse App\Models\TrainingReportTemplate;at the top of the model. - In the
2026_01_04_223424_create_training_report_template_area_tablemigration, callingSchema::dropIfExistsin theup()method will silently drop any existing data on re-run; it would be safer to remove that call and instead handle cleanup in a dedicated rollback or manual migration if needed. - In
admin/reporttemplates/edit.blade.php, the selected logic for the areas multi-select ($template->areas->contains($area->id) || in_array($area->id, old('areas', []))) means original assignments can override user changes after a validation error; you may want to preferold('areas')when present (e.g. usingin_array($area->id, old('areas', $template->areas->pluck('id')->toArray()))).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `trainingReportTemplates()` relation on `Area` references `TrainingReportTemplate` without a `use` import in `Area.php`, which will cause a class resolution error when the relation is used; consider adding `use App\Models\TrainingReportTemplate;` at the top of the model.
- In the `2026_01_04_223424_create_training_report_template_area_table` migration, calling `Schema::dropIfExists` in the `up()` method will silently drop any existing data on re-run; it would be safer to remove that call and instead handle cleanup in a dedicated rollback or manual migration if needed.
- In `admin/reporttemplates/edit.blade.php`, the selected logic for the areas multi-select (`$template->areas->contains($area->id) || in_array($area->id, old('areas', []))`) means original assignments can override user changes after a validation error; you may want to prefer `old('areas')` when present (e.g. using `in_array($area->id, old('areas', $template->areas->pluck('id')->toArray()))`).
## Individual Comments
### Comment 1
<location> `resources/views/training/report/create.blade.php:65` </location>
<code_context>
+ <select class="form-select @error('template_id') is-invalid @enderror" name="template_id" id="templateSelect">
+ <option value="">-- Select a template --</option>
+ @foreach($templates as $template)
+ <option value="{{ $template->id }}" data-content="{{ htmlspecialchars($template->content, ENT_QUOTES, 'UTF-8') }}" data-contentimprove="{{ htmlspecialchars($template->contentimprove ?? '', ENT_QUOTES, 'UTF-8') }}">{{ $template->name }}</option>
+ @endforeach
+ </select>
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid double-escaping template content in data attributes, which will surface as HTML entities in the editor.
`{{ ... }}` already escapes output, so adding `htmlspecialchars` escapes it a second time. When you read `dataset.content` into the editor, users will see `&` instead of `&`. Please adjust the attribute so the value is escaped exactly once, for example:
- `data-content="{!! e($template->content) !!}"`, or
- `data-content="{{ $template->content }}"` with `@php echo e(...); @endphp`
This keeps the content safe in the attribute without corrupting what appears in the editor.
</issue_to_address>
### Comment 2
<location> `database/migrations/2026_01_04_223424_create_training_report_template_area_table.php:14-15` </location>
<code_context>
+ */
+ public function up(): void
+ {
+ // Drop the table if it exists (from a previous failed migration)
+ Schema::dropIfExists('training_report_template_area');
+
+ Schema::create('training_report_template_area', function (Blueprint $table) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Dropping the pivot table in `up()` can cause unexpected data loss if migrations are re-run.
Using `Schema::dropIfExists` in `up()` means any time this migration is re-run (e.g. in non-fresh environments or some CI flows), existing pivot data will be dropped before the table is recreated. If this was only to recover from a one-off failed migration, it’s safer to remove the drop and instead do a manual cleanup or a separate corrective migration.
</issue_to_address>
### Comment 3
<location> `app/Http/Controllers/TrainingReportTemplateController.php:25-28` </location>
<code_context>
+ {
+ $this->authorize('viewAny', TrainingReportTemplate::class);
+
+ $templates = TrainingReportTemplate::with('areas')->orderBy('created_at', 'desc')->get();
+ $areas = Area::all();
+
+ return view('admin.reporttemplates', compact('templates', 'areas'));
</code_context>
<issue_to_address>
**suggestion (performance):** The `$areas` variable loaded in `index()` is not used in the corresponding view.
Since the view only references `$templates`, consider removing the `$areas = Area::all();` query and omitting `$areas` from the `view()` call to avoid an unnecessary database query on each request.
```suggestion
$templates = TrainingReportTemplate::with('areas')->orderBy('created_at', 'desc')->get();
return view('admin.reporttemplates', compact('templates'));
```
</issue_to_address>
### Comment 4
<location> `resources/views/admin/reporttemplates/edit.blade.php:49-54` </location>
<code_context>
+ <label class="form-label" for="areas">Assign to Areas</label>
+ <select class="form-select @error('areas') is-invalid @enderror" name="areas[]" id="areas" multiple size="5">
+ @foreach($areas as $area)
+ <option value="{{ $area->id }}" {{ $template->areas->contains($area->id) || in_array($area->id, old('areas', [])) ? 'selected' : '' }}>{{ $area->name }}</option>
+ @endforeach
+ </select>
</code_context>
<issue_to_address>
**suggestion:** Let `old('areas')` take precedence over the existing assignment so the form reflects failed-validation input correctly.
As written, a previously-associated area can still appear selected after a validation failure even if the user tried to unselect it, because `$template->areas->contains($area->id)` is always considered alongside `old('areas')`. To preserve the user’s last submission, branch on `old('areas')` first, e.g. `($old = old('areas')) !== null ? in_array($area->id, $old) : $template->areas->contains($area->id)`.
```suggestion
<label class="form-label" for="areas">Assign to Areas</label>
<select class="form-select @error('areas') is-invalid @enderror" name="areas[]" id="areas" multiple size="5">
@php($oldAreas = old('areas'))
@foreach($areas as $area)
<option value="{{ $area->id }}" {{
!is_null($oldAreas)
? (in_array($area->id, $oldAreas ?? []) ? 'selected' : '')
: ($template->areas->contains($area->id) ? 'selected' : '')
}}>{{ $area->name }}</option>
@endforeach
</select>
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <select class="form-select @error('template_id') is-invalid @enderror" name="template_id" id="templateSelect"> | ||
| <option value="">-- Select a template --</option> | ||
| @foreach($templates as $template) | ||
| <option value="{{ $template->id }}" data-content="{{ htmlspecialchars($template->content, ENT_QUOTES, 'UTF-8') }}" data-contentimprove="{{ htmlspecialchars($template->contentimprove ?? '', ENT_QUOTES, 'UTF-8') }}">{{ $template->name }}</option> |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Avoid double-escaping template content in data attributes, which will surface as HTML entities in the editor.
{{ ... }} already escapes output, so adding htmlspecialchars escapes it a second time. When you read dataset.content into the editor, users will see & instead of &. Please adjust the attribute so the value is escaped exactly once, for example:
data-content="{!! e($template->content) !!}", ordata-content="{{ $template->content }}"with@php echo e(...); @endphp
This keeps the content safe in the attribute without corrupting what appears in the editor.
Comment on lines
+14
to
+15
| // Drop the table if it exists (from a previous failed migration) | ||
| Schema::dropIfExists('training_report_template_area'); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Dropping the pivot table in up() can cause unexpected data loss if migrations are re-run.
Using Schema::dropIfExists in up() means any time this migration is re-run (e.g. in non-fresh environments or some CI flows), existing pivot data will be dropped before the table is recreated. If this was only to recover from a one-off failed migration, it’s safer to remove the drop and instead do a manual cleanup or a separate corrective migration.
Comment on lines
+25
to
+28
| $templates = TrainingReportTemplate::with('areas')->orderBy('created_at', 'desc')->get(); | ||
| $areas = Area::all(); | ||
|
|
||
| return view('admin.reporttemplates', compact('templates', 'areas')); |
Contributor
There was a problem hiding this comment.
suggestion (performance): The $areas variable loaded in index() is not used in the corresponding view.
Since the view only references $templates, consider removing the $areas = Area::all(); query and omitting $areas from the view() call to avoid an unnecessary database query on each request.
Suggested change
| $templates = TrainingReportTemplate::with('areas')->orderBy('created_at', 'desc')->get(); | |
| $areas = Area::all(); | |
| return view('admin.reporttemplates', compact('templates', 'areas')); | |
| $templates = TrainingReportTemplate::with('areas')->orderBy('created_at', 'desc')->get(); | |
| return view('admin.reporttemplates', compact('templates')); |
Comment on lines
+49
to
+54
| <label class="form-label" for="areas">Assign to Areas</label> | ||
| <select class="form-select @error('areas') is-invalid @enderror" name="areas[]" id="areas" multiple size="5"> | ||
| @foreach($areas as $area) | ||
| <option value="{{ $area->id }}" {{ $template->areas->contains($area->id) || in_array($area->id, old('areas', [])) ? 'selected' : '' }}>{{ $area->name }}</option> | ||
| @endforeach | ||
| </select> |
Contributor
There was a problem hiding this comment.
suggestion: Let old('areas') take precedence over the existing assignment so the form reflects failed-validation input correctly.
As written, a previously-associated area can still appear selected after a validation failure even if the user tried to unselect it, because $template->areas->contains($area->id) is always considered alongside old('areas'). To preserve the user’s last submission, branch on old('areas') first, e.g. ($old = old('areas')) !== null ? in_array($area->id, $old) : $template->areas->contains($area->id).
Suggested change
| <label class="form-label" for="areas">Assign to Areas</label> | |
| <select class="form-select @error('areas') is-invalid @enderror" name="areas[]" id="areas" multiple size="5"> | |
| @foreach($areas as $area) | |
| <option value="{{ $area->id }}" {{ $template->areas->contains($area->id) || in_array($area->id, old('areas', [])) ? 'selected' : '' }}>{{ $area->name }}</option> | |
| @endforeach | |
| </select> | |
| <label class="form-label" for="areas">Assign to Areas</label> | |
| <select class="form-select @error('areas') is-invalid @enderror" name="areas[]" id="areas" multiple size="5"> | |
| @php($oldAreas = old('areas')) | |
| @foreach($areas as $area) | |
| <option value="{{ $area->id }}" {{ | |
| !is_null($oldAreas) | |
| ? (in_array($area->id, $oldAreas ?? []) ? 'selected' : '') | |
| : ($template->areas->contains($area->id) ? 'selected' : '') | |
| }}>{{ $area->name }}</option> | |
| @endforeach | |
| </select> |
thor
requested changes
Jan 5, 2026
Collaborator
thor
left a comment
thor
left a comment
There was a problem hiding this comment.
Whoop!
I haven't looked particularly close at any one thing yet, but visually it looks interesting. Given the failing checks, I've only done a very cursory glance, and want to give it a closer review if it's fully ready.
Seeing as this is a complete feature PR, instead of bit by bit:
- Tests! Simple, yet with good coverage. Broad and useful. Occasionally specific for the edge cases.
- Style. Define return types with PHPs type annotations where applicable.
- Documentation. How do you use it?
During use:
-
Would all report-related templates and, if they exist, be available for training department members (and above) only?
-
Should we keep putting things under Administration as one app or view the different parts of CC as multiple apps with their own settings?
Contributor
|
Will training staff for each FIR be allowed to manage their own templates? Looking at the examples above it might look like this is something that would be available to Administrator only with assignment to different FIRs. |
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
3 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.
Implementation for issue #1203
Summary by Sourcery
Introduce reusable training report templates and integrate them into the training report workflow.
New Features:
Enhancements:
Chores: