Conversation
|
Caution Review failedThe pull request is closed. WalkthroughSwitches SMTP/DKIM sourcing to config(), updates Control API to sync actions/documents/users/groups (removing owners), adds/clarified type hints and return types, tightens Socialite role/lang handling and signatures, fixes migration typos, switches seeders to getenv(), converts route controllers to strings, removes user-group export, and bumps versions. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Provider as OAuth Provider
participant App as SocialiteController
participant DB as Database
User->>App: OAuth callback
App->>Provider: Retrieve profile
Provider-->>App: Profile (email, claims)
App->>App: Resolve role via role_claim/default_role (config)
App->>DB: Find or create user (create_user returns User)
DB-->>App: User record
App->>User: Login or redirect
sequenceDiagram
actor Client
participant API as ControlController (API)
participant DB as Database
Client->>API: POST/PUT control payload
API->>DB: Create/Update control
API->>DB: Sync measures
alt payload contains relations
API->>DB: Conditionally sync actions/documents/users/groups
end
DB-->>API: Relations persisted
API-->>Client: JSON response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (13)
app/Http/Controllers/SocialiteController.php (3)
126-150: create_user() declares User return type but returns null on failure; also store a hashed placeholder password.
- Returning null violates the declared return type and will raise a TypeError. Bubble the exception instead (it’s already caught in callback()).
- Passwords should be hashed even if not used for auth; storing a random plaintext token is a security smell.
- // TODO allow null password - $user->password = bin2hex(random_bytes(32)); + // TODO allow null password + // Store a non-guessable, hashed placeholder since the account is OIDC-managed + $user->password = Hash::make(bin2hex(random_bytes(32))); @@ - } catch(QueryException $exception) { - Log::debug($exception->getMessage()); - Log::error('Unable to create user'); - return null; - } + } catch (QueryException $exception) { + Log::error('Unable to create user', ['exception' => $exception]); + throw $exception; + }Additionally required import (outside this hunk):
use Illuminate\Support\Facades\Hash;If you want, I can follow up to implement the “allow null password” TODO end-to-end (DB default, model casts/validation).
196-203: Guarantee string return type in get_user_login().Cast the values to string to avoid potential TypeError if the provider returns a non-string id/claim.
- if ($socialite_user->offsetExists('preferred_username')) { - return $socialite_user->offsetGet('preferred_username'); - } - return $socialite_user->id; + if ($socialite_user->offsetExists('preferred_username')) { + return (string) $socialite_user->offsetGet('preferred_username'); + } + return (string) $socialite_user->id;
210-226: get_user_role() declares string but may return null; also no validation of default_role. Normalize and ensure a valid mapping is always returned.Current code returns null when no valid role/default is provided, which contradicts the string return type and will explode at runtime. It also doesn’t validate that default_role exists in ROLES_MAP.
Refactor to:
- Normalize role values (trim/lowercase).
- Validate both the claim-derived role and default_role.
- Fall back to a safe default ('user') when needed.
- Keep the string return type contract.
- private function get_user_role(SocialiteUser $socialite_user, string $role_claim, string $default_role): string - { - $role_name = ''; - if (! empty($role_claim)) { - $role_name = $this->get_claim_value($socialite_user, $role_claim); - Log::debug("Provided claim '{$role_claim}'='{$role_name}'"); - } - if (! array_key_exists($role_name, self::ROLES_MAP)) { - if (! empty($default_role)) { - $role_name = $default_role; - } else { - Log::error("No default role set! A valid role must be provided. role='{$role_name}'"); - return null; - } - } - return self::ROLES_MAP[$role_name]; - } + private function get_user_role(SocialiteUser $socialite_user, string $role_claim, string $default_role): string + { + $role_name = ''; + if (!empty($role_claim)) { + $value = $this->get_claim_value($socialite_user, $role_claim); + $role_name = is_string($value) ? strtolower(trim($value)) : ''; + Log::debug("Provided claim '{$role_claim}'='{$role_name}'"); + } + + $candidate = $role_name; + if (!array_key_exists($candidate, self::ROLES_MAP)) { + $candidate = !empty($default_role) ? strtolower(trim($default_role)) : 'user'; + if (!array_key_exists($candidate, self::ROLES_MAP)) { + Log::warning("Invalid default role '{$default_role}', falling back to 'user'"); + $candidate = 'user'; + } + } + + return self::ROLES_MAP[$candidate]; + }Also, the docblock above still mentions returning null; please update it to match the non-null contract.
app/Http/Controllers/MeasureController.php (2)
482-507: Avoid sorting/unique inside the loop; compute once after building the array.Sorting and uniquing per attribute iteration is O(n^2) and unnecessary. Do it once after the loop to reduce complexity and CPU.
Apply this diff:
- foreach ($attributes as $attribute) { - foreach (explode(' ', $attribute->values) as $value) { - if (strlen($value) > 0) { - array_push($values, $value); - } - } - sort($values); - $values = array_unique($values); - } + foreach ($attributes as $attribute) { + foreach (explode(' ', $attribute->values) as $value) { + if (strlen($value) > 0) { + $values[] = $value; + } + } + } + $values = array_values(array_unique($values)); + sort($values);
596-602: Fix: incorrect status filter, possible null dereference, and outdated relation usage.
- where('status', [0,1]) is incorrect; use whereIn.
- ->first()->id will throw on null; use value(...) or safe navigation.
- Query filters by controls.measure_id, which is inconsistent with the many-to-many pivot used elsewhere (control_measure). Aligning to the pivot avoids silently returning null.
Apply this diff:
- $control_id = DB::table('controls') - ->select('id') - ->where('measure_id', '=', $request->id) - ->where('status', [0,1]) - ->first() - ->id; + $control_id = Control::query() + ->join('control_measure', 'controls.id', '=', 'control_measure.control_id') + ->where('control_measure.measure_id', $request->id) + ->whereIn('controls.status', [0, 1]) + ->value('controls.id');Optional: if multiple active controls are possible for a measure, consider handling the full set (pluck('controls.id')).
database/seeders/AttributeSeeder.php (1)
21-27: Harden language detection: handle unset LANG, locale variants (fr_FR), and avoid TypeError in strtolower.getenv('LANG') may be false/null; strtolower expects string. Also LANG commonly includes region/encoding (e.g., fr_FR.UTF-8), so the current strict 'fr' comparison will pick EN incorrectly.
Apply this diff:
- // get language - $lang = getenv('LANG'); - - // get filename - if (strtolower($lang)==="fr") - $filename="database/data/attributes.fr.csv"; - else - $filename="database/data/attributes.en.csv"; + // get language (fallback to app.locale, default to 'en'); accept locale variants like 'fr_FR' + $lang = strtolower((string) (getenv('LANG') ?: config('app.locale') ?: 'en')); + // get filename + if (str_starts_with($lang, 'fr')) { + $filename = "database/data/attributes.fr.csv"; + } else { + $filename = "database/data/attributes.en.csv"; + }Optional: add an existence check and fail early if the CSV is missing:
if (! is_file(base_path($filename))) { throw new \RuntimeException("Seed file not found: {$filename}"); }app/Models/Measure.php (2)
52-59: Use relationship for existence checks (clearer and consistent with Eloquent)Avoid raw
DB::tablehere; leverage thecontrols()relationship for readability and consistency.- public function isActive(): bool - { - return DB::table('controls') - ->where('measure_id', $this->id) - ->whereNull('realisation_date') - ->exists(); - } + public function isActive(): bool + { + return $this->controls() + ->whereNull('realisation_date') + ->exists(); + }
61-72: Clarify intent and use relationship for the disabled stateThe comment doesn’t match the implementation, and using the relation reads better. This preserves behavior: true if there are controls and none are “empty” (i.e., pending).
- // check if there is an empty control associated with this measure + // Check if the measure is disabled (has controls but none pending/empty) public function isDisabled(): bool { - return DB::table('controls') - ->where('measure_id', $this->id) - ->exists() - && - ! DB::table('controls') - ->where('measure_id', $this->id) - ->whereNull('realisation_date') - ->exists(); + return $this->controls()->exists() + && ! $this->controls() + ->whereNull('realisation_date') + ->exists(); }database/seeders/MeasureSeeder.php (1)
22-28: LANG detection: add fallback and normalize locale codesSame concern as in DomainSeeder: handle values like
fr_BE.UTF-8and provide a fallback when LANG is unset.- $lang = getenv('LANG'); + $lang = getenv('LANG') ?: config('app.locale', 'en'); + $lang = strtolower(substr($lang, 0, 2));app/Exports/MeasuresExport.php (1)
73-90: Flatten the array returned by map()The
WithMappingconcern requiresmap()to return a single-dimensional array for each row. Currently, you’re returning an extra nested array, which will introduce unwanted structure in the exported sheet.Please update the method in app/Exports/MeasuresExport.php to remove the outer array:
• File: app/Exports/MeasuresExport.php
• Method: map()Proposed change:
public function map($measure): array { - return [ - [ - $measure->domain->framework, - $measure->domain->title, - $measure->domain->description, - $measure->clause, - $measure->name, - $measure->objective, - $measure->attributes, - $measure->input, - $measure->model, - $measure->indicator, - $measure->action_plan, - ], - ]; + return [ + $measure->domain->framework, + $measure->domain->title, + $measure->domain->description, + $measure->clause, + $measure->name, + $measure->objective, + $measure->attributes, + $measure->input, + $measure->model, + $measure->indicator, + $measure->action_plan, + ]; }app/Http/Controllers/ActionController.php (3)
94-98: Critical: Wrong join/alias when filtering actions for auditees.You’re joining action_user but referencing control_user in the join condition, and joining via controls.id. This will either error or silently mis-filter results. You likely want to filter actions assigned to the user via the action_user pivot.
Apply this diff:
- if (Auth::User()->role === 5) { - $actions = $actions - ->leftjoin('action_user', 'controls.id', '=', 'control_user.control_id') - ->where('action_user.user_id', '=', Auth::User()->id); - } + if (Auth::user()->role === 5) { + $actions = $actions + ->join('action_user', 'actions.id', '=', 'action_user.action_id') + ->where('action_user.user_id', '=', Auth::id()); + }Optionally, also qualify the earlier join as
->leftJoin('controls', 'actions.control_id', '=', 'controls.id')for clarity.
289-293: Likely wrong source for measures in edit(): using action id as a control_id.This query fetches measures by
control_id = $id, but $id is the action id. Use the existing relation on the Action model to get measure IDs.Apply this diff:
- $measures = DB::table('control_measure') - ->select('measure_id') - ->where('control_id', $id) - ->pluck('measure_id') - ->toArray(); + $measures = $action->measures()->pluck('measures.id')->toArray();If you truly need control-based measures, first resolve
$controlId = $action->control_idand use that explicitly.
534-537: Bug: reading 'end' date from 'start' request key.The end date should come from the 'end' parameter.
Apply this diff:
- $end = $request->get('start'); + $end = $request->get('end');
🧹 Nitpick comments (18)
app/Http/Controllers/SocialiteController.php (2)
76-79: Initialize role config vars as empty strings to align with typed signatures.These are later passed to methods typed as string. Initializing to '' instead of null avoids accidental type mismatches if control flow changes in the future.
- $role_claim = null; - $default_role = null; + $role_claim = ''; + $default_role = '';
232-241: Language selection looks good; minor hardening and typos.
- Use strict in_array and cast to string to avoid surprises from provider data.
- Nit: “langage” (method name) and “dertermine” in the doc comment are typos; consider renaming to get_user_language and fixing comments/logs (non-blocking).
- if ($socialite_user->offsetExists('locale')) { - $locale = explode('-', $socialite_user->offsetGet('locale'))[0]; - if (in_array($locale, self::LOCALES)) { + if ($socialite_user->offsetExists('locale')) { + $locale = (string) explode('-', (string) $socialite_user->offsetGet('locale'))[0]; + if (in_array($locale, self::LOCALES, true)) { return $locale; } } return self::LOCALES[0];If you opt to rename the method, remember to update its two call sites in create_user() and update_user().
app/Http/Controllers/ControlController.php (1)
1359-1361: Minor readability improvement: use Carbon’s toDateString().Functionally identical, slightly clearer API usage.
- ->addMonthsNoOverflow($control->periodicity) - ->format('Y-m-d'); + ->addMonthsNoOverflow($control->periodicity) + ->toDateString();app/Http/Controllers/MeasureController.php (1)
298-301: Formatting-only change is fine; consider more robust tokenization of attributes.Current split on a single space will leave empty tokens if there are multiple spaces. Minor, but easy to harden.
Apply this diff for resilient splitting and filtering:
- $selectedAttributes = array_filter( - explode(' ', $measure->attributes ?? ''), - fn ($val) => strlen($val) > 0 - ); + $selectedAttributes = array_values(array_filter( + preg_split('/\s+/', trim((string) ($measure->attributes ?? ''))), + 'strlen' + ));app/Providers/AuthServiceProvider.php (1)
12-14: Type the $policies property for stricter static analysis.Adding a native type improves clarity and tooling support.
Apply this diff:
- protected $policies = [ + protected array $policies = [ // 'App\Model' => 'App\Policies\ModelPolicy', ];resources/views/actions/close.blade.php (1)
33-49: Simplify with Blade @switch and add a safe default for unknown types.Current if/elseif chain is fine but verbose and lacks a default fallback. A switch improves readability and ensures something is rendered for unexpected values.
Apply this diff:
- @if ($action->type==1) - <p class="fg-red text-bold"> - {{ trans('cruds.action.types.major') }} - </p> - @elseif ($action->type==2) - <p class="fg-orange text-bold"> - {{ trans('cruds.action.types.minor') }} - </p> - @elseif ($action->type==3) - <p class="fg-yellow text-bold"> - {{ trans('cruds.action.types.observation') }} - </p> - @elseif ($action->type==4) - <p class="fg-green text-bold"> - {{ trans('cruds.action.types.opportunity') }} - </p> - @endif + @switch($action->type) + @case(1) + <p class="fg-red text-bold">{{ trans('cruds.action.types.major') }}</p> + @break + @case(2) + <p class="fg-orange text-bold">{{ trans('cruds.action.types.minor') }}</p> + @break + @case(3) + <p class="fg-yellow text-bold">{{ trans('cruds.action.types.observation') }}</p> + @break + @case(4) + <p class="fg-green text-bold">{{ trans('cruds.action.types.opportunity') }}</p> + @break + @default + <p class="text-bold">{{ $action->type }}</p> + @endswitchOptional: verify that translation keys cruds.action.types.* exist for all cases. If not, add them to avoid rendering the raw type.
app/Providers/EventServiceProvider.php (1)
14-14: Type the $listen property for better static checks.Native typing enhances maintainability without behavior change.
Apply this diff:
- protected $listen = [ + protected array $listen = [app/Http/Middleware/Authenticate.php (1)
12-17: Confirm parent signature compatibility; prefer short nullable syntaxOverriding the signature is fine in recent Laravel/PHP versions, but please ensure it remains compatible with the parent
Illuminate\Auth\Middleware\Authenticate::redirectTo. Prefer the short nullable syntax for readability.- protected function redirectTo(\Illuminate\Http\Request $request): string|null + protected function redirectTo(\Illuminate\Http\Request $request): ?stringIf your project is on an older Laravel/PHP version, verify that adding a parameter type and return type doesn't trigger a signature compatibility error with the parent.
database/seeders/MeasureSeeder.php (1)
35-37: Remove noisy debug logging in seeding
Log::Debug($data);will spam logs during seeding and may impact performance. Remove or guard under a verbosity flag.app/Http/Controllers/ActionController.php (3)
6-9: Nit: Misgrouped imports (models vs exports).The import comments don’t match the actual imports, which is confusing during maintenance. Group Action under Models and ActionsExport under Export.
Apply this diff:
-// Models -use App\Exports\ActionsExport; -// Export -use App\Models\Action; +// Models +use App\Models\Action; +// Export +use App\Exports\ActionsExport;
30-34: Simplify role check with in_array and strict comparison.Current OR-chain is verbose. Using in_array with strict mode improves readability and avoids subtle type coercions.
Apply this diff:
- ! ((Auth::User()->role === 1) || - (Auth::User()->role === 2) || - (Auth::User()->role === 3) || - (Auth::User()->role === 5)), + ! in_array(Auth::user()->role, [1, 2, 3, 5], true),If you like this style, consider applying the same pattern across this controller for consistency.
600-601: Nit: Typo in comment."scrope" → "scope".
Apply this diff:
- // Get Actions in scrope + // Get Actions in scopeapp/Http/Controllers/DocumentController.php (1)
115-121: Stream or download the file instead of loading it fully into memory.For large files,
file_get_contents+strlenloads the entire file in memory. Preferresponse()->download(...)or a streamed response to reduce memory footprint and improve performance.Example (outside the shown lines):
$headers = [ 'Cache-Control' => 'no-cache private', 'Content-Description' => 'File Transfer', 'Content-Type' => $document->mimetype, 'Content-Disposition' => 'attachment; filename="'.$document->filename.'"', 'Content-Transfer-Encoding'=> 'binary', ]; return response()->download($path, $document->filename, $headers);app/Http/Controllers/DomainController.php (2)
170-175: Prefer PHPDoc-style docblock (/) for export() to aid tooling.**Switching to a plain block comment may cause some IDEs/static analyzers to ignore it.
Apply this diff:
- /* + /** * Export the Domains in Excel * * @return \Illuminate\Http\RedirectResponse */
22-23: Redundant assignment to $domains (immediately overwritten).The first query result is discarded by the second assignment. Remove the unused query for clarity.
Apply this diff:
- $domains = DB::table('domains')->orderBy('id')->get(); -app/Http/Controllers/API/ControlController.php (2)
31-42: Consider wrapping create + relation syncs in a transaction.If any of the syncs fail, you can end up with a partially-created entity with inconsistent relations. Wrap the create + sync block in a DB transaction to ensure atomicity.
Example (outside this hunk):
use Illuminate\Support\Facades\DB; DB::transaction(function () use ($request) { $control = Control::create($request->all()); // ... sync measures/actions/users/groups/documents as needed });
63-74: Deduplicate relation-sync logic across store() and update().The four new sync blocks are duplicated in both methods. Extract into a private helper method to keep it DRY and less error-prone.
Example (outside this hunk):
private function syncRelations(Control $control, Request $request): void { foreach (['measures','actions','users','groups'] as $rel) { if ($request->has($rel)) { $control->{$rel}()->sync($request->input($rel, [])); } } if ($request->has('documents')) { $docIds = $request->input('documents', []); if (empty($docIds)) { $control->documents()->update(['control_id' => null]); } else { $control->documents()->whereNotIn('id', $docIds)->update(['control_id' => null]); \App\Models\Document::whereIn('id', $docIds)->update(['control_id' => $control->id]); } } }Then call
syncRelations($control, $request)from both methods.config/mail.php (1)
45-48: New keys (auth, secure, auto_tls): ensure alignment with consumers.Your consumers should read these under
mail.mailers.smtp.*. Current code in two places usedmail.smtp.authandmail.mailer.smtp.*(singular), which won’t resolve.Either:
- Keep these keys and ensure all code uses
mail.mailers.smtp.*(what I suggested in diffs), or- Drop
secureand rely on the existingencryptionkey to avoid two sources of truth.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
app/Console/Commands/SendNotifications.php(3 hunks)app/Exceptions/Handler.php(0 hunks)app/Exports/DomainsExport.php(0 hunks)app/Exports/MeasuresExport.php(2 hunks)app/Http/Controllers/API/ControlController.php(2 hunks)app/Http/Controllers/ActionController.php(2 hunks)app/Http/Controllers/ConfigurationController.php(2 hunks)app/Http/Controllers/ControlController.php(1 hunks)app/Http/Controllers/DocumentController.php(1 hunks)app/Http/Controllers/DomainController.php(1 hunks)app/Http/Controllers/MeasureController.php(1 hunks)app/Http/Controllers/MeasureImportController.php(1 hunks)app/Http/Controllers/SocialiteController.php(6 hunks)app/Http/Controllers/UserGroupController.php(0 hunks)app/Http/Kernel.php(0 hunks)app/Http/Middleware/Authenticate.php(1 hunks)app/Http/Middleware/CheckForMaintenanceMode.php(0 hunks)app/Http/Middleware/EncryptCookies.php(0 hunks)app/Http/Middleware/TrimStrings.php(0 hunks)app/Http/Middleware/TrustProxies.php(0 hunks)app/Http/Middleware/VerifyCsrfToken.php(0 hunks)app/Models/Action.php(1 hunks)app/Models/Document.php(1 hunks)app/Models/Measure.php(4 hunks)app/Providers/AuthServiceProvider.php(1 hunks)app/Providers/EventServiceProvider.php(1 hunks)config/mail.php(2 hunks)database/migrations/2024_11_06_123808_add_actions.php(1 hunks)database/seeders/AttributeSeeder.php(2 hunks)database/seeders/DomainSeeder.php(1 hunks)database/seeders/MeasureSeeder.php(2 hunks)package.json(1 hunks)resources/views/actions/close.blade.php(1 hunks)routes/api.php(1 hunks)version.txt(1 hunks)
💤 Files with no reviewable changes (9)
- app/Http/Middleware/CheckForMaintenanceMode.php
- app/Http/Middleware/VerifyCsrfToken.php
- app/Http/Middleware/EncryptCookies.php
- app/Http/Middleware/TrustProxies.php
- app/Http/Middleware/TrimStrings.php
- app/Exports/DomainsExport.php
- app/Http/Kernel.php
- app/Exceptions/Handler.php
- app/Http/Controllers/UserGroupController.php
🧰 Additional context used
🧬 Code Graph Analysis (8)
app/Providers/EventServiceProvider.php (3)
app/Providers/AuthServiceProvider.php (1)
boot(19-22)app/Providers/AppServiceProvider.php (1)
boot(29-63)app/Providers/RouteServiceProvider.php (1)
boot(20-34)
app/Http/Controllers/API/ControlController.php (1)
app/Models/Document.php (1)
control(9-12)
app/Providers/AuthServiceProvider.php (3)
app/Providers/EventServiceProvider.php (1)
boot(23-26)app/Providers/AppServiceProvider.php (1)
boot(29-63)app/Providers/RouteServiceProvider.php (1)
boot(20-34)
app/Console/Commands/SendNotifications.php (1)
app/Models/Control.php (1)
measures(54-58)
app/Http/Controllers/ActionController.php (1)
app/Models/User.php (1)
User(11-80)
app/Models/Action.php (1)
app/Http/Controllers/AuditLogsController.php (1)
history(58-108)
database/seeders/DomainSeeder.php (1)
app/Models/Domain.php (1)
Domain(7-26)
app/Http/Controllers/SocialiteController.php (1)
app/Models/User.php (1)
User(11-80)
🔇 Additional comments (17)
app/Http/Controllers/SocialiteController.php (1)
6-6: Importing Exception is appropriate for top-level callback error handling.No issues with this addition.
version.txt (1)
1-1: Version bump aligns with release metadata. LGTM.The version is updated to 2025.08.13 and matches package.json. No issues.
package.json (1)
2-2: Package version updated. LGTM.Version set to 2025.08.13, consistent with version.txt.
app/Http/Controllers/MeasureController.php (1)
303-306: Readable multi-line view return — looks good.app/Providers/AuthServiceProvider.php (1)
19-21: Explicit void return on boot() — good alignment with other providers.database/seeders/AttributeSeeder.php (1)
37-38: LGTM on numeric CSV indexing.Using numeric indices with fgetcsv is idiomatic and clear.
app/Providers/EventServiceProvider.php (1)
23-26: Explicit void return on boot() — consistent with other providers.app/Models/Measure.php (1)
7-7: Good move: explicit relation type importImporting and using
BelongsToimproves IDE support and static analysis accuracy. LGTM.database/seeders/MeasureSeeder.php (1)
22-28: Cross-seeder ordering and integrity checkEnsure
DomainSeederruns beforeMeasureSeederand that domain IDs in the CSV match actual IDs in the DB after seeding (especially if you switch totruncate()).app/Exports/MeasuresExport.php (2)
6-6: Good: explicit Builder import for accurate return typingThis aligns with the move to explicit return types and improves static analysis.
92-95: LGTM: typed query() returning Eloquent Builder with eager-loaded relationThe explicit
Builderreturn type andwith('domain')avoid N+1s inmap. All good.app/Models/Action.php (1)
66-69: LGTM: Query builder chaining/readabilityThe where/orderBy/get chain is clear and idiomatic. No issues spotted here.
app/Models/Document.php (1)
7-7: LGTM: Formatting only.No behavioral change; relation remains correct within the same namespace.
app/Http/Controllers/ConfigurationController.php (1)
117-122: Ignore the DKIM path change; the config usesmail.dkim.*at top level
Theconfig/mail.phpfile defines the DKIM settings under the top-level'dkim'key (line 76), not undermailers. Your current calls toconfig('mail.dkim.*')are correct—no updates needed.Likely an incorrect or invalid review comment.
config/mail.php (2)
39-39: Changing default SMTP host to localhost may break existing deployments.Defaulting to
localhostinstead of a real SMTP host (e.g., Mailgun) can silently misroute mail on environments without a local MTA.Confirm this is intentional for your deployment profile. If not, keep the default to a well-known provider or require explicit MAIL_HOST.
76-81: DKIM config is under mail.mailers.dkim; make sure all code references the same path.Both ConfigurationController and SendNotifications read
mail.dkim.*. I proposed diffs to switch them tomail.mailers.dkim.*to match this structure.If you’d rather keep
mail.dkim.*, move this block to the top level.routes/api.php (1)
19-24: LGTM: Controller strings under a namespaced group are valid and consistent.Given the explicit Route::namespace('App\Http\Controllers\API'), using string controller names is fine and keeps routes concise.
If you use route caching in production, run
php artisan route:cachelocally to ensure no namespace resolution issues.
| if ($request->has('actions')) { | ||
| $control->actions()->sync($request->input('actions', [])); | ||
| } | ||
| if ($request->has('documents')) { | ||
| $control->documents()->sync($request->input('documents', [])); | ||
| } |
There was a problem hiding this comment.
documents()->sync() is invalid for hasMany; will throw. Replace with explicit reassignment logic.
Document.php defines Document::control() as belongsTo(Control::class), which implies Control::documents() is a hasMany. The sync() method is only available on many-to-many relations and will error here.
Replace the documents()->sync(...) with explicit updates to set/unset control_id:
- if ($request->has('documents')) {
- $control->documents()->sync($request->input('documents', []));
- }
+ if ($request->has('documents')) {
+ $docIds = $request->input('documents', []);
+ if (empty($docIds)) {
+ // Detach all existing documents
+ $control->documents()->update(['control_id' => null]);
+ } else {
+ // Detach documents no longer linked
+ $control->documents()->whereNotIn('id', $docIds)->update(['control_id' => null]);
+ // Attach selected documents
+ \App\Models\Document::whereIn('id', $docIds)->update(['control_id' => $control->id]);
+ }
+ }Do you want me to follow up with a patch that also updates destroy() to detach documents (set control_id to null) and parity across update()?
📝 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.
| if ($request->has('actions')) { | |
| $control->actions()->sync($request->input('actions', [])); | |
| } | |
| if ($request->has('documents')) { | |
| $control->documents()->sync($request->input('documents', [])); | |
| } | |
| if ($request->has('actions')) { | |
| $control->actions()->sync($request->input('actions', [])); | |
| } | |
| if ($request->has('documents')) { | |
| $docIds = $request->input('documents', []); | |
| if (empty($docIds)) { | |
| // Detach all existing documents | |
| $control->documents()->update(['control_id' => null]); | |
| } else { | |
| // Detach documents no longer linked | |
| $control->documents()->whereNotIn('id', $docIds)->update(['control_id' => null]); | |
| // Attach selected documents | |
| \App\Models\Document::whereIn('id', $docIds)->update(['control_id' => $control->id]); | |
| } | |
| } |
| if ($request->has('actions')) { | ||
| $control->actions()->sync($request->input('actions', [])); | ||
| } | ||
| if ($request->has('documents')) { | ||
| $control->documents()->sync($request->input('documents', [])); | ||
| } | ||
| if ($request->has('users')) { | ||
| $control->users()->sync($request->input('users', [])); | ||
| } | ||
| if ($request->has('groups')) { | ||
| $control->groups()->sync($request->input('groups', [])); | ||
| } |
There was a problem hiding this comment.
Repeat of documents()->sync() bug in update(); use the same reassignment pattern.
Same reasoning as in store(): sync() on hasMany will fail. Use explicit updates to manage control_id.
- if ($request->has('documents')) {
- $control->documents()->sync($request->input('documents', []));
- }
+ if ($request->has('documents')) {
+ $docIds = $request->input('documents', []);
+ if (empty($docIds)) {
+ $control->documents()->update(['control_id' => null]);
+ } else {
+ $control->documents()->whereNotIn('id', $docIds)->update(['control_id' => null]);
+ \App\Models\Document::whereIn('id', $docIds)->update(['control_id' => $control->id]);
+ }
+ }📝 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.
| if ($request->has('actions')) { | |
| $control->actions()->sync($request->input('actions', [])); | |
| } | |
| if ($request->has('documents')) { | |
| $control->documents()->sync($request->input('documents', [])); | |
| } | |
| if ($request->has('users')) { | |
| $control->users()->sync($request->input('users', [])); | |
| } | |
| if ($request->has('groups')) { | |
| $control->groups()->sync($request->input('groups', [])); | |
| } | |
| if ($request->has('actions')) { | |
| $control->actions()->sync($request->input('actions', [])); | |
| } | |
| if ($request->has('documents')) { | |
| $docIds = $request->input('documents', []); | |
| if (empty($docIds)) { | |
| $control->documents()->update(['control_id' => null]); | |
| } else { | |
| $control->documents() | |
| ->whereNotIn('id', $docIds) | |
| ->update(['control_id' => null]); | |
| \App\Models\Document::whereIn('id', $docIds) | |
| ->update(['control_id' => $control->id]); | |
| } | |
| } | |
| if ($request->has('users')) { | |
| $control->users()->sync($request->input('users', [])); | |
| } | |
| if ($request->has('groups')) { | |
| $control->groups()->sync($request->input('groups', [])); | |
| } |
🤖 Prompt for AI Agents
In app/Http/Controllers/API/ControlController.php around lines 63-74, the
update() method incorrectly calls $control->documents()->sync(...), but
documents is a hasMany so sync will fail; replace the sync call with the same
reassignment pattern used in store(): when $request->has('documents') fetch the
incoming document IDs, update any existing documents not in that list to set
control_id = null (or remove association), and update the documents in the
incoming list to set control_id = $control->id (use a single query for each
update set rather than sync). Keep the conditional ($request->has('documents'))
and ensure you operate on IDs from $request->input('documents', []).
| $table->integer('criticity')->default(0); | ||
| $table->integer('status')->default(0); |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Fixed typo in migration defaults. LGTM, but verify rollout strategy.
The switch from dafault(0) to default(0) fixes a migration breakage. Note: editing an existing migration will not affect databases where this migration has already run. If this has been applied anywhere, create a new migration to alter the columns’ defaults instead.
Would you like me to generate an additional migration (e.g., alter table to set default 0 on actions.criticity and actions.status) to safely apply this in already-migrated environments?
Ensure safe rollout of migration default fixes
Editing the original migration only applies to new deployments. For databases that have already run 2024_11_06_123808_add_actions.php, you must provide a follow-up migration to update the column defaults without data loss.
• Location to review:
– database/migrations/2024_11_06_123808_add_actions.php (lines 27–28)
• Suggested follow-up migration snippet:
Schema::table('actions', function (Blueprint $table) {
$table->integer('criticity')->default(0)->change();
$table->integer('status')->default(0)->change();
});• Confirm and document your deployment strategy to apply this change in already-migrated environments.
🤖 Prompt for AI Agents
In database/migrations/2024_11_06_123808_add_actions.php around lines 27 to 28,
you only changed defaults in the original migration which affects new installs
only; create a new follow-up migration that uses Schema::table to change() the
existing columns to default(0) for both criticity and status so existing
databases are updated without data loss, and include in the PR a short
deployment plan (run migrations during maintenance window, backup DB, and verify
changes) so already-migrated environments receive the default-change safely.
| "domain_id" => $data[0], | ||
| "clause" => $data[1], | ||
| "name" => $data[2], | ||
| "objective" => str_replace("\\n","\n",$data[3]), | ||
| "attributes" => str_replace("\\n","\n",$data[4]), | ||
| "input" => str_replace("\\n","\n",$data[5]), | ||
| "model" => str_replace("\\n","\n",$data[6]), | ||
| "indicator" => str_replace("\\n","\n",$data[7]), | ||
| "action_plan" => str_replace("\\n","\n",$data[8]), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
indicator/action_plan likely dropped due to mass-assignment constraints
App\Models\Measure fillable doesn't include indicator or action_plan, so these attributes won’t be persisted via Measure::create([...]). You’ll silently lose data.
Recommended fixes (pick one):
- Preferred: Add missing attributes to
$fillableinapp/Models/Measure.php:
// app/Models/Measure.php
protected $fillable = [
'name',
'clause',
'objective',
'input',
'attributes',
'model',
'indicator',
'action_plan',
];- Or: Use
DB::table('measures')->insert([...])for seeding to bypass fillable. - Or: Temporarily
Model::unguard()during seeding.
Also verify FK integrity: since domain_id is taken from CSV (column 0), it assumes DomainSeeder preserved IDs. If Domain IDs are not preserved (e.g., due to mass-assignment issues or not truncating), this will point to non-existent domains.
🏁 Script executed:
#!/bin/bash
grep -R "protected \$fillable" -n app/Models/Measure.php -A10Length of output: 334
🏁 Script executed:
#!/bin/bash
grep -R -n -C3 "Measure::create" database/seeders/MeasureSeeder.phpLength of output: 379
🏁 Script executed:
#!/bin/bash
grep -R -n -C5 "class DomainSeeder" -n database/seeders/DomainSeeder.phpLength of output: 296
🏁 Script executed:
#!/bin/bash
# Print the contents of DomainSeeder to inspect how domains are inserted
sed -n '1,200p' database/seeders/DomainSeeder.phpLength of output: 1145
Ensure domain_id, indicator, and action_plan are mass-assignable in Measure
The MeasureSeeder uses Measure::create([...]) but the model’s $fillable only lists name, clause, objective, input, attributes, and model. As a result, domain_id, indicator, and action_plan will be ignored and never persisted.
Please update your model (app/Models/Measure.php) as follows:
// app/Models/Measure.php
protected $fillable = [
- 'name',
- 'clause',
- 'objective',
- 'input',
- 'attributes',
- 'model',
+ 'domain_id',
+ 'clause',
+ 'name',
+ 'objective',
+ 'input',
+ 'attributes',
+ 'model',
+ 'indicator',
+ 'action_plan',
];Locations to review:
- app/Models/Measure.php (lines 30–37)
- database/seeders/MeasureSeeder.php (lines 39–47)
Alternative approaches:
- Use
DB::table('measures')->insert([...])in the seeder to bypass$fillable. - Wrap your seeding in
Model::unguard()/Model::reguard(). - Verify that your
DomainSeederactually preserves IDs (consider usingDB::table('domains')->truncate()or ensuringApp\Models\Domainallows mass-assignment ofid) so thatdomain_idvalues in the CSV resolve correctly.
🤖 Prompt for AI Agents
In database/seeders/MeasureSeeder.php around lines 39–47, the seeder writes
domain_id, indicator, and action_plan but app/Models/Measure.php (lines 30–37)
currently omits them from $fillable so those fields are ignored; update
Measure.php to include "domain_id", "indicator", and "action_plan" in the
model’s $fillable array (or alternatively change the seeder to
DB::table('measures')->insert([...]) or wrap the seeding with
Model::unguard()/Model::reguard()), and also verify your DomainSeeder preserves
expected IDs (or allow mass-assignment of Domain id) so domain_id values from
the CSV resolve correctly.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/Console/Commands/SendNotifications.php (2)
93-97: Eliminate N+1 queries; iterate the already-loaded relation.Calling $control->measures()->get() inside the loop causes an N+1. Iterate the relation collection instead, and eager load it in the controls query to fetch all measures in one go.
Apply this diff to the loop:
- foreach ($control->measures()->get() as $measure) { + foreach ($control->measures as $measure) {Additionally (outside this hunk), update the $controls query to select distinct controls and eager load measures to avoid duplicates caused by joins:
$controls = Control::where('status', 0) ->leftJoin('control_user', 'controls.id', '=', 'control_user.control_id') ->leftJoin('control_user_group', 'controls.id', '=', 'control_user_group.control_id') ->leftJoin('user_user_group', 'control_user_group.user_group_id', '=', 'user_user_group.user_group_id') ->where(function ($query) use ($user) { $query->where('control_user.user_id', '=', $user->id) ->orWhere('user_user_group.user_id', '=', $user->id); }) ->where('plan_date', '<=', Carbon::today() ->addDays((int) config('deming.notification.expire-delay'))->toDateString()) ->select('controls.*') ->distinct() ->orderBy('plan_date') ->with('measures') // eager load to avoid N+1 ->get();
144-147: DKIM config keys point to mail.dkim.; switch to mail.mailers.dkim. (and gate on presence).These keys won’t resolve if config has been moved under mail.mailers.dkim.* as in the rest of the PR; result: DKIM silently not applied. Also set these only if all required DKIM values are present to avoid partial config.
Apply this diff:
- $mail->DKIM_domain = config('mail.dkim.domain'); - $mail->DKIM_private = config('mail.dkim.private'); - $mail->DKIM_selector = config('mail.dkim.selector'); - $mail->DKIM_passphrase = config('mail.dkim.passphrase'); - $mail->DKIM_identity = $mail->From; + $dkimDomain = config('mail.mailers.dkim.domain'); + $dkimPrivate = config('mail.mailers.dkim.private'); + $dkimSelector = config('mail.mailers.dkim.selector'); + $dkimPassphrase = config('mail.mailers.dkim.passphrase'); + if ($dkimDomain && $dkimPrivate && $dkimSelector) { + $mail->DKIM_domain = $dkimDomain; + $mail->DKIM_private = $dkimPrivate; + $mail->DKIM_selector = $dkimSelector; + $mail->DKIM_passphrase = $dkimPassphrase; + $mail->DKIM_identity = $mail->From; + }Run this to verify DKIM keys used across the repo and ensure the config path matches:
#!/bin/bash # Check occurrences of DKIM config keys and where they're defined echo "== DKIM config usages ==" rg -n -A2 -i $'config\\([\'"]mail\\.(mailers\\.)?dkim\\.' echo -e "\n== mail config DKIM definitions ==" rg -n -A4 -i 'dkim' config
🧹 Nitpick comments (1)
app/Console/Commands/SendNotifications.php (1)
119-120: Map encryption to PHPMailer constants; avoid overriding SMTPAutoTLS when unset.Small hardening: use PHPMailer’s ENCRYPTION_* constants for clarity, and only override SMTPAutoTLS if the config key is present—leaving PHPMailer’s default (true) otherwise.
- $mail->SMTPSecure = config('mail.mailers.smtp.encryption'); // 'tls', 'ssl', null/false - $mail->SMTPAutoTLS = (bool) config('mail.mailers.smtp.auto_tls', false); + $encryption = config('mail.mailers.smtp.encryption'); // 'tls' | 'ssl' | null/false + $mail->SMTPSecure = $encryption === 'tls' + ? PHPMailer::ENCRYPTION_STARTTLS + : ($encryption === 'ssl' ? PHPMailer::ENCRYPTION_SMTPS : false); + $autoTls = config('mail.mailers.smtp.auto_tls'); + if ($autoTls !== null) { + $mail->SMTPAutoTLS = (bool) $autoTls; + }
📜 Review details
Configuration used: CodeRabbit 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)
app/Console/Commands/SendNotifications.php(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/Console/Commands/SendNotifications.php (2)
app/Models/Document.php (1)
control(9-12)app/Models/Control.php (1)
measures(54-58)
🔇 Additional comments (1)
app/Console/Commands/SendNotifications.php (1)
109-120: SMTP config read via config() with correct keys and types looks good.Using config('mail.mailers.smtp.*') with explicit int/bool casts aligns with the PR’s move away from env() and PHPMailer’s expectations.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores