-
Notifications
You must be signed in to change notification settings - Fork 3
Nachtragshaushaltsplan (NHHP): change-set overlay, apply/revert, editor & view #329
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
Open
lukas-staab
wants to merge
37
commits into
release/4.5.0
Choose a base branch
from
feature/op-581-nhhp
base: release/4.5.0
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
37 commits
Select commit
Hold shift + click to select a range
067b535
refactor(budget-plan): rename state Published -> Active
lukas-staab 8120093
feat(budget-plan): add Nachtragshaushaltsplan schema + model layer
lukas-staab 2ece216
feat(budget-plan): wire the amendment apply/revert engine into the st…
lukas-staab 658ec8e
feat(budget-plan): schedule automatic activation of due amendments
lukas-staab ddf280b
feat(budget-plan): add Nachtragshaushaltsplan creation entry point an…
lukas-staab e29844a
feat(budget-plan): show the amendment diff on plan-view; note history…
lukas-staab 84663ca
fix(budget-plan): fix updatedReasonInputs' property-path parsing in t…
lukas-staab 3dda8f1
test(budget-plan): cover the Published->Active state rename and amend…
lukas-staab b6b6470
test(budget-plan): cover the amendment editor's change-set recording
lukas-staab 982a862
test(budget-plan): cover the amendment apply/revert engine
lukas-staab d426ef4
test(budget-plan): cover the stufis:apply-due-amendments schedule com…
lukas-staab ed726ab
test(budget-plan): cover amendment-awareness in the legacy views and …
lukas-staab 212f2d9
fix(budget-plan): stop BudgetItemChange::fieldChange() reading Eloque…
lukas-staab a15ef25
fix(budget-plan): give amendment-editor rows a real wire:key so morph…
lukas-staab 780ebf5
feat(budget-plan): let budget officers set an amendment's approval/ef…
lukas-staab b9860b4
feat(budget-plan): make a base item's Titelnummer (short_name) immuta…
lukas-staab a8944c0
feat(budget-plan): highlight the specific changed field in the amendm…
lukas-staab 28ecb29
feat(budget-plan): let a Nachtrag carry its own optional name
lukas-staab a4cf3c0
feat(budget-plan): add a back button and fix breadcrumb nesting for t…
lukas-staab a7a9f7d
feat(budget-plan): show the amendment's aggregated income/expense del…
lukas-staab 1722094
refactor(budget-plan): move "Nachtrag erstellen" into plan-view's act…
lukas-staab cbc13ee
feat(budget-plan): freeze "Bearbeiten" for normal plans from Approved…
lukas-staab 5c3f7ee
fix(budget-plan): rename the `delete` Livewire actions so CSP-safe ev…
lukas-staab 2116de8
refactor(budget-plan): confirm plan deletion in a flux modal instead …
lukas-staab 78cedfc
refactor(budget-plan): rename budget_item_change.changes to diff to s…
lukas-staab 08f39dd
feat(budget-plan): add DATEV export button and remove placeholder pri…
lukas-staab db60bdc
fix(budget-plan): stop a plan's item tree pulling in another plan's i…
lukas-staab cb667fd
feat(budget-plan): show on a title which Nachträge change it
lukas-staab 747c98c
feat(budget-plan): tighten HHP/NHHP status rules (create window, dele…
lukas-staab 6506cbb
feat(budget-plan): validate a plan's titles when it advances through …
lukas-staab 1cc8c7e
feat(budget-plan): capture the meta dates in the state-change dialog
lukas-staab b34d250
docs(changelog): note the NHHP fixes, status rules, title validation …
lukas-staab 97be360
fix(budget-plan,konto): repair the PHPStan and translations CI checks
lukas-staab e0c48a8
test(accounting): stop the CSV import redirect test drifting with the…
lukas-staab c96f709
refactor(budget-plan): rename the amendment effectiveness date to act…
lukas-staab a397d10
refactor(budget-plan): work through the NHHP review feedback
lukas-staab e8be09e
refactor(budget-plan): give editability one home and delete through t…
lukas-staab File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| <?php | ||
|
|
||
| namespace App\Console\Commands\stufis; | ||
|
|
||
| use App\Models\BudgetPlan; | ||
| use App\States\BudgetPlan\Active; | ||
| use App\Support\Budget\AmendmentConflictException; | ||
| use Illuminate\Console\Command; | ||
| use Illuminate\Support\Facades\Log; | ||
| use Spatie\ModelStates\Exceptions\CouldNotPerformTransition; | ||
| use Throwable; | ||
|
|
||
| /** | ||
| * Scheduled effectiveness for amendments (OP#581): an approved amendment with an | ||
| * `activation_date` in the past should go live on its own, without someone manually clicking | ||
| * "aktivieren" on the day. Runs daily (see routes/console.php). | ||
| * | ||
| * Every due amendment is transitioned independently, so one amendment's conflict (e.g. a stale | ||
| * item, or its parent plan no longer being Active) doesn't block the others. Failures are logged | ||
| * and reported on stderr; the command exits non-zero when any amendment failed, so the run stays | ||
| * visible and the schedule's failure hooks can act on it — the run is safely re-triggerable, since | ||
| * only successfully-applied amendments leave Approved. | ||
| */ | ||
| class ApplyDueAmendments extends Command | ||
| { | ||
| /** | ||
| * The name and signature of the console command. | ||
| * | ||
| * @var string | ||
| */ | ||
| protected $signature = 'stufis:apply-due-amendments'; | ||
|
|
||
| /** | ||
| * The console command description. | ||
| * | ||
| * @var string | ||
| */ | ||
| protected $description = 'Activate approved amendments whose activation_date has arrived'; | ||
|
|
||
| public function handle(): int | ||
| { | ||
| $due = BudgetPlan::query()->dueForActivation()->get(); | ||
|
|
||
| if ($due->isEmpty()) { | ||
| $this->info('No due amendments to activate.'); | ||
|
|
||
| return self::SUCCESS; | ||
| } | ||
|
|
||
| $failed = 0; | ||
| foreach ($due as $amendment) { | ||
| try { | ||
| $amendment->state->transitionTo(Active::class); | ||
| $this->info("Activated amendment #{$amendment->id} ({$amendment->label()})."); | ||
| } catch (AmendmentConflictException|CouldNotPerformTransition $e) { | ||
| $failed++; | ||
| $this->error("Amendment #{$amendment->id} could not be activated: {$e->getMessage()}"); | ||
| Log::warning('stufis:apply-due-amendments failed for amendment', [ | ||
| 'amendment_id' => $amendment->id, | ||
| 'message' => $e->getMessage(), | ||
| ]); | ||
| } catch (Throwable $e) { | ||
| $failed++; | ||
| $this->error("Amendment #{$amendment->id} could not be activated: {$e->getMessage()}"); | ||
| Log::error('stufis:apply-due-amendments unexpected failure for amendment', [ | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here |
||
| 'amendment_id' => $amendment->id, | ||
| 'exception' => $e, | ||
| ]); | ||
| } | ||
| } | ||
|
|
||
| return $failed > 0 ? self::FAILURE : self::SUCCESS; | ||
| } | ||
| } | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,6 +136,16 @@ public function budgetPlan(): BelongsTo | |
| return $this->belongsTo(BudgetPlan::class, 'budget_plan_id'); | ||
| } | ||
|
|
||
| /** | ||
| * The change rows amendments have drafted against this item — several amendments may touch | ||
| * the same item at once. An `add` row only surfaces here once its amendment was applied and | ||
| * the new item got rehomed onto this plan. | ||
| */ | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make this comment more relevant for readers in the future. It explains very verbose why its HasMany, it explains the design very complicated |
||
| public function amendmentChanges(): HasMany | ||
| { | ||
| return $this->hasMany(BudgetItemChange::class, 'budget_item_id'); | ||
| } | ||
|
|
||
| /** The plan this item "mounts" (only set for mount items). */ | ||
| public function referencedPlan(): BelongsTo | ||
| { | ||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| <?php | ||
|
|
||
| namespace App\Models; | ||
|
|
||
| use App\Models\Enums\BudgetItemChangeAction; | ||
| use Illuminate\Database\Eloquent\Model; | ||
| use Illuminate\Database\Eloquent\Relations\BelongsTo; | ||
| use Illuminate\Support\Carbon; | ||
|
|
||
| /** | ||
| * App\Models\BudgetItemChange | ||
| * | ||
| * One delta row of an amendment against a single budget_item, keyed by (budget_plan_id, | ||
| * budget_item_id) — see the Architecture section of OP#581 for the full change-set design. | ||
| * `action` is one of: | ||
| * | ||
| * - modify: the item already existed on the parent plan; `diff` holds | ||
| * {field: {"from": ..., "to": ...}} for every touched field. | ||
| * - add: `budget_item_id` points at a real BudgetItem row created under the amendment plan; | ||
| * `diff` is typically empty (the item itself carries the new data). | ||
| * - delete: `budget_item_id` points at a live item slated for removal (only allowed when it has | ||
| * no bookings); `diff` is typically empty. | ||
| * | ||
| * The column is named `diff`, not `changes`: Eloquent's own HasAttributes trait already declares a | ||
| * `protected $changes` property for its dirty-tracking bookkeeping, and a `changes` column silently | ||
| * shadows it when read from INSIDE the model (magic `__get()` only kicks in for external access, so | ||
| * `$this->changes` there would read Eloquent's internal array instead of the cast attribute). This | ||
| * already caused a production bug once; renaming the column removes the trap entirely instead of | ||
| * routing around it. | ||
| * | ||
| * @property int $id | ||
| * @property int $budget_plan_id | ||
| * @property int $budget_item_id | ||
| * @property BudgetItemChangeAction $action | ||
| * @property array<string, array{from: mixed, to: mixed}>|null $diff | ||
| * @property string|null $reason | ||
| * @property Carbon $created_at | ||
| * @property Carbon $updated_at | ||
| * @property-read BudgetPlan $amendmentPlan | ||
| * @property-read BudgetItem $budgetItem | ||
| */ | ||
| class BudgetItemChange extends Model | ||
| { | ||
| protected $table = 'budget_item_change'; | ||
|
|
||
| protected $fillable = ['budget_plan_id', 'budget_item_id', 'action', 'diff', 'reason']; | ||
|
|
||
| #[\Override] | ||
| protected function casts(): array | ||
| { | ||
| return [ | ||
| 'action' => BudgetItemChangeAction::class, | ||
| 'diff' => 'array', | ||
| ]; | ||
| } | ||
|
|
||
| public function amendmentPlan(): BelongsTo | ||
| { | ||
| return $this->belongsTo(BudgetPlan::class, 'budget_plan_id'); | ||
| } | ||
|
|
||
| public function budgetItem(): BelongsTo | ||
| { | ||
| return $this->belongsTo(BudgetItem::class, 'budget_item_id'); | ||
| } | ||
|
|
||
| /** | ||
| * The {from, to} pair recorded for a single field, or null when this change row doesn't | ||
| * (or no longer) touches that field. | ||
| * | ||
| * @return array{from: mixed, to: mixed}|null | ||
| */ | ||
| public function fieldChange(string $field): ?array | ||
| { | ||
| return $this->diff[$field] ?? null; | ||
| } | ||
|
|
||
| /** Whether this row currently touches any field at all (an empty `diff` should be pruned). */ | ||
| public function isEmpty(): bool | ||
| { | ||
| return $this->action === BudgetItemChangeAction::Modify && blank($this->diff); | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
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.
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.
back to OP WP: how should a failed apply be notified. To whom, and when, and how often?