Conversation
Contributor
WalkthroughAdds a weekly periodicity option (-1) across controllers, views, and translations; expands control history logic to generate weekly repetitions; refactors some controller queries and authorization checks to helper methods; and bumps project version metadata. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Web as Web Controller
participant DB as Database
participant Renderer
User->>Web: Request controls listing (GET /controls)
Web->>DB: Fetch controls (with periodicity, realisation_date)
DB-->>Web: Return controls
alt control.realisation_date is null and periodicity == -1
Web->>Web: Expand into weekly clones (add 7 days per clone)
Web->>DB: (optional) materialize/prepare cloned entries
else control.realisation_date is null and periodicity > 0
Web->>Web: Expand into monthly clones (add periodicity months)
end
Web->>Renderer: Render controls list (including clones)
Renderer-->>User: HTML response with weekly/monthly items
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Http/Controllers/HomeController.php`:
- Around line 422-444: The weekly expansion loop in HomeController (the for loop
using for ($i = 1; $i <= 32; $i++) that clones $control and sets plan_date via
Carbon::parse(...)->addDays($i * 7)) is inconsistent with the similar loop in
ControlController (which uses $i <= 52); update HomeController to use the same
iteration count as ControlController (change 32 to 52) or, if the shorter
horizon is intentional, add a concise inline comment next to the for (...) in
HomeController explaining why 32 weeks is used (e.g., "dashboard shows shorter
projection than history view") so the difference is explicit and maintainable.
In `@resources/views/actions/index.blade.php`:
- Around line 105-117: The HTML closing tags are out of order around the cell
that renders icons for $action->type; move the </center> so it closes before the
</td> (i.e., ensure the <center> wrapper encloses the `@if/`@elseif/@endif span
block and is closed, then close the table cell), and verify the indentation
around the `@if` block and spans (mif-warning/mif-eye/mif-thumbs-up) remains
correct so the HTML structure is valid.
In `@resources/views/controls/edit.blade.php`:
- Line 192: The select option in edit.blade.php for weekly recurrence has
value="2" but the code expects periodicity == -1; update the option to
value="-1" and keep the selection check using $control->periodicity == -1 so the
form submits the correct -1 value that ControlController (methods handling
periodicity checks and next-date logic) expects; verify consistency with
create.blade.php which already uses value="-1".
In `@resources/views/controls/plan.blade.php`:
- Around line 79-80: You changed the UI option for “once” from periodicity=1 to
0 but did not include a data migration or controller checks, which will cause
legacy rows with periodicity=1 to be misinterpreted as recurring; add a
reversible DB migration/backfill that updates existing records with
periodicity=1 to periodicity=0, update any controller logic in ControlController
and HomeController that currently checks periodicity (>0 or !==0) to explicitly
treat 0 as “once” (e.g., periodicity === 0) and 1 no longer special-case, and
add a small migration test and/or logging to ensure the backfill runs safely
during rollout.
In `@resources/views/measures/plan.blade.php`:
- Around line 121-124: The periodicity input is currently only marked required
in MeasureController and raw values are saved; add server-side allow-list
validation for the periodicity field in the MeasureController method that
handles the form submission (e.g., the store/update action) by adding an "in"
rule (or Rule::in([...])) allowing only the supported values -1, 0, 1, validate
before persisting, and ensure you use the validated value (or cast to int) when
assigning to $measure->periodicity so unallowed values cannot be saved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9a92e9bc-85fe-4137-9e67-dbe4005411ff
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
app/Http/Controllers/ActionController.phpapp/Http/Controllers/ControlController.phpapp/Http/Controllers/HomeController.phppackage.jsonresources/lang/de/common.phpresources/lang/en/common.phpresources/lang/fr/common.phpresources/views/actions/index.blade.phpresources/views/controls/create.blade.phpresources/views/controls/edit.blade.phpresources/views/controls/make.blade.phpresources/views/controls/plan.blade.phpresources/views/controls/show.blade.phpresources/views/measures/plan.blade.phpversion.txt
Comment on lines
+422
to
444
| if ($control->realisation_date === null) { | ||
| if ($control->periodicity === -1) { | ||
| for ($i = 1; $i <= 32; $i++) { | ||
| $repeatedControl = clone $control; | ||
| $repeatedControl->id = null; | ||
| $repeatedControl->score = null; | ||
| $repeatedControl->observations = null; | ||
| $repeatedControl->realisation_date = null; | ||
| $repeatedControl->plan_date = Carbon::parse($control->plan_date)->addDays($i * 7); | ||
| $expanded->push($repeatedControl); | ||
| } | ||
| } | ||
| else if ($control->periodicity > 0 && $control->periodicity <= 12) { | ||
| for ($i = 1; $i <= 12 / $control->periodicity; $i++) { | ||
| $repeatedControl = clone $control; | ||
| $repeatedControl->id = null; | ||
| $repeatedControl->score = null; | ||
| $repeatedControl->observations = null; | ||
| $repeatedControl->realisation_date = null; | ||
| $repeatedControl->plan_date = Carbon::parse($control->plan_date)->addMonthsNoOverflow($i * $control->periodicity); | ||
| $expanded->push($repeatedControl); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the weekly expansion iteration counts across both controllers
echo "=== HomeController weekly iterations ==="
rg -n "periodicity === -1" -A 5 app/Http/Controllers/HomeController.php
echo ""
echo "=== ControlController weekly iterations ==="
rg -n "periodicity === -1" -A 5 app/Http/Controllers/ControlController.phpRepository: dbarzin/deming
Length of output: 1439
Inconsistency: Weekly expansion uses 32 iterations (HomeController) vs 52 (ControlController).
Line 424 uses $i <= 32 (≈8 months of weekly projections), while ControlController line 797 uses $i <= 52 (52 weeks). Both expand weekly controls identically but project different time horizons.
Either align both to the same iteration count, or add an inline comment in both locations documenting why the difference is intentional (e.g., dashboard needs shorter future projection than history view).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Http/Controllers/HomeController.php` around lines 422 - 444, The weekly
expansion loop in HomeController (the for loop using for ($i = 1; $i <= 32;
$i++) that clones $control and sets plan_date via Carbon::parse(...)->addDays($i
* 7)) is inconsistent with the similar loop in ControlController (which uses $i
<= 52); update HomeController to use the same iteration count as
ControlController (change 32 to 52) or, if the shorter horizon is intentional,
add a concise inline comment next to the for (...) in HomeController explaining
why 32 weeks is used (e.g., "dashboard shows shorter projection than history
view") so the difference is explicit and maintainable.
Comment on lines
+79
to
+80
| <option value="0" {{ $control->periodicity==0 ? "selected" : ""}}>{{ trans('common.once') }}</option> | ||
| <option value="-1" {{ $control->periodicity==-1 ? "selected" : ""}}>{{ trans('common.weekly') }}</option> |
Contributor
There was a problem hiding this comment.
Changing “once” from 1 to 0 needs a data migration/backfill plan.
With this UI remap, legacy rows using periodicity=1 can be treated as recurring monthly by controller logic (periodicity > 0 / periodicity !== 0 in app/Http/Controllers/ControlController.php and app/Http/Controllers/HomeController.php). That can create unintended future controls.
📦 Suggested rollout safeguard
+ // Add a one-time data migration/reconciliation step before enabling this mapping in production:
+ // 1) Identify legacy records that used `1` as "once"
+ // 2) Convert them to `0`
+ // 3) Keep true monthly records as `1`
+ // 4) Document the cutoff strategy in release notes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/views/controls/plan.blade.php` around lines 79 - 80, You changed
the UI option for “once” from periodicity=1 to 0 but did not include a data
migration or controller checks, which will cause legacy rows with periodicity=1
to be misinterpreted as recurring; add a reversible DB migration/backfill that
updates existing records with periodicity=1 to periodicity=0, update any
controller logic in ControlController and HomeController that currently checks
periodicity (>0 or !==0) to explicitly treat 0 as “once” (e.g., periodicity ===
0) and 1 no longer special-case, and add a small migration test and/or logging
to ensure the backfill runs safely during rollout.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Http/Controllers/ControlController.php (1)
794-805:history()expansion may become heavy with many weekly controls.Lines 797-805 add 52 synthetic rows per pending weekly control. Consider bounding expansion horizon (e.g., configurable months/weeks or pagination window) to keep memory/render time predictable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/ControlController.php` around lines 794 - 805, The history() expansion currently generates 52 weekly synthetic controls (inside the block that checks $control->realisation_date === null and $control->periodicity === -1), which can blow up memory; change this to compute a bounded expansion horizon (e.g., derive $maxWeeks from a configurable setting or request parameter like config('controls.expansion_weeks', 12) converted to weeks or from a pagination window) and loop only for $i = 1..$maxWeeks, or skip expansion when outside the current pagination/time window; update the creation of $repeatedControl and $repeatedControl->plan_date (still using Carbon::parse($control->plan_date)->addDays($i * 7)) but iterate only up to the calculated $maxWeeks so expansion is predictable and configurable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Http/Controllers/ControlController.php`:
- Around line 1490-1499: The scheduling branch that sets $new_control->plan_date
for monthly periodicity uses
Carbon::parse($control->plan_date)->addMonths($control->periodicity)->toDateString(),
which is inconsistent with other scheduling paths; replace addMonths(...) with
addMonthsNoOverflow(...) so the code uses
Carbon::parse($control->plan_date)->addMonthsNoOverflow($control->periodicity)->toDateString()
to avoid end-of-month drift and match the other occurrences that use
addMonthsNoOverflow().
---
Nitpick comments:
In `@app/Http/Controllers/ControlController.php`:
- Around line 794-805: The history() expansion currently generates 52 weekly
synthetic controls (inside the block that checks $control->realisation_date ===
null and $control->periodicity === -1), which can blow up memory; change this to
compute a bounded expansion horizon (e.g., derive $maxWeeks from a configurable
setting or request parameter like config('controls.expansion_weeks', 12)
converted to weeks or from a pagination window) and loop only for $i =
1..$maxWeeks, or skip expansion when outside the current pagination/time window;
update the creation of $repeatedControl and $repeatedControl->plan_date (still
using Carbon::parse($control->plan_date)->addDays($i * 7)) but iterate only up
to the calculated $maxWeeks so expansion is predictable and configurable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4d16d804-4b86-4048-bcef-a8db3d4c9042
📒 Files selected for processing (4)
app/Http/Controllers/ControlController.phpapp/Http/Controllers/MeasureController.phpresources/views/actions/index.blade.phpresources/views/controls/edit.blade.php
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/views/actions/index.blade.php
Comment on lines
+1490
to
+1499
| if ($control->periodicity === -1) | ||
| // One week | ||
| $new_control->plan_date = Carbon::parse($control->plan_date) | ||
| ->addDays(7) | ||
| ->toDateString(); | ||
| else | ||
| // Months | ||
| $new_control->plan_date = Carbon::parse($control->plan_date) | ||
| ->addMonths($control->periodicity) | ||
| ->toDateString(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify monthly increment method consistency in ControlController scheduling paths.
rg -n "addMonthsNoOverflow|addMonths\(" app/Http/Controllers/ControlController.phpRepository: dbarzin/deming
Length of output: 440
🏁 Script executed:
#!/bin/bash
# Get context around all addMonths/addMonthsNoOverflow calls to understand usage patterns
echo "=== Context around line 194 ==="
sed -n '185,210p' app/Http/Controllers/ControlController.php
echo -e "\n=== Context around line 815 ==="
sed -n '810,820p' app/Http/Controllers/ControlController.php
echo -e "\n=== Context around line 1385 ==="
sed -n '1380,1390p' app/Http/Controllers/ControlController.php
echo -e "\n=== Context around line 1498 ==="
sed -n '1490,1505p' app/Http/Controllers/ControlController.phpRepository: dbarzin/deming
Length of output: 2622
Use addMonthsNoOverflow() here to maintain consistency with other scheduling paths.
Line 1498 currently uses addMonths(), while the same scheduling operation at line 1385 and line 815 uses addMonthsNoOverflow(). This inconsistency can produce different next dates (particularly for end-of-month dates), causing schedule drift across different code paths.
🐛 Proposed fix
- $new_control->plan_date = Carbon::parse($control->plan_date)
- ->addMonths($control->periodicity)
- ->toDateString();
+ $new_control->plan_date = Carbon::parse($control->plan_date)
+ ->addMonthsNoOverflow($control->periodicity)
+ ->toDateString();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Http/Controllers/ControlController.php` around lines 1490 - 1499, The
scheduling branch that sets $new_control->plan_date for monthly periodicity uses
Carbon::parse($control->plan_date)->addMonths($control->periodicity)->toDateString(),
which is inconsistent with other scheduling paths; replace addMonths(...) with
addMonthsNoOverflow(...) so the code uses
Carbon::parse($control->plan_date)->addMonthsNoOverflow($control->periodicity)->toDateString()
to avoid end-of-month drift and match the other occurrences that use
addMonthsNoOverflow().
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
1 participant
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.
Summary by CodeRabbit
New Features
Improvements
Chores