This repository was archived by the owner on May 29, 2026. It is now read-only.
fix(factory): accept permissionLevel arg on DashboardFactory::create (fixes broken unit test)#77
Merged
Conversation
Contributor
Quality Report — ConductionNL/mydash @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 342/342 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-01 07:27 UTC
Download the full PDF report from the workflow artifacts.
4 tasks
rubenvdlinde
force-pushed
the
fix/factory-permission-level
branch
from
d0b5b3a to
ea2c83c
Compare
…reate DashboardFactoryTest::testCreateRespectsExplicitPermissionLevel was failing with "Unknown named parameter \$permissionLevel" because the factory hard-coded \$dashboard->setPermissionLevel(PERMISSION_FULL) and exposed no way for callers (or share / fork flows) to override it. Add the param, default to PERMISSION_FULL (preserving existing behaviour for the 9 other tests + every production caller, which are all positional), and pass it through to setPermissionLevel(). Tests: 354 / 354 passing.
rubenvdlinde
force-pushed
the
fix/factory-permission-level
branch
from
ea2c83c to
3f3f90c
Compare
Contributor
Quality Report — ConductionNL/mydash @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 342/342 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-01 08:02 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/mydash @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 342/342 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-01 08:03 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
that referenced
this pull request
May 1, 2026
PR #78 dropped phpstan from 47 → 26 by fixing the IL10N namespace typo. This batch clears the rest in four contained groups and removes the `|| echo` swallow from the composer script so future regressions actually fail CI. DashboardShareApiController (21): swap `DataResponse` → `JSONResponse` throughout the controller, matching the pattern used by every other passing controller in the app (DashboardApiController, ResponseHelper). The two classes are wire-compatible (both extend Response, same constructor signature, both serialise to JSON), but phpstan can resolve the JSONResponse generics from the existing docblocks while the bare DataResponse type was forcing it to infer T = `array<string, string>` and reject the call. AdminSettingMapper (2): the entity's setUpdatedAt() expects `?string`, but setSetting() was passing a `DateTime` object. Format to `Y-m-d H:i:s` to match the convention from DashboardFactory::create(). AdminTemplateService (1): replace `Ramsey\Uuid\Uuid::uuid4()` with the same `random_bytes`-based generator used by DashboardFactory. Ramsey UUID is not a declared composer dep, so the previous code would have crashed at runtime — this surfaced because phpstan correctly couldn't resolve the class. FileService (2): two `Node::nodeExists()` / `Node::newFile()` / `Node::putContent()` calls were made on results of `Folder::get()`, which returns the `Node` interface. Type-narrow with explicit `instanceof Folder` / `instanceof File` checks (throwing `RuntimeException` on the impossible-but-typed branch) so phpstan can verify the methods exist. Side benefit: removes a stale `@phpstan-ignore-next-line` that was silencing a real downstream error. composer.json: drop the `|| echo 'PHPStan not installed, skipping...'` from the `phpstan` script. The `||` made every phpstan run exit 0, so the CI quality job was reporting green even when phpstan failed — exactly how the IL10N typo and the Ramsey-UUID call slipped through review. Now `composer phpstan` propagates the real exit code. Verified locally: phpstan `[OK] No errors`, phpcs clean across 49 files, phpunit 354/354 (the one pre-existing DashboardFactoryTest::testCreateRespectsExplicitPermissionLevel failure is fixed by PR #77 and out of scope here).
rubenvdlinde
added a commit
that referenced
this pull request
May 1, 2026
PR #78 dropped phpstan from 47 → 26 by fixing the IL10N namespace typo. This batch clears the rest in four contained groups and removes the `|| echo` swallow from the composer script so future regressions actually fail CI. DashboardShareApiController (21): swap `DataResponse` → `JSONResponse` throughout the controller, matching the pattern used by every other passing controller in the app (DashboardApiController, ResponseHelper). The two classes are wire-compatible (both extend Response, same constructor signature, both serialise to JSON), but phpstan can resolve the JSONResponse generics from the existing docblocks while the bare DataResponse type was forcing it to infer T = `array<string, string>` and reject the call. AdminSettingMapper (2): the entity's setUpdatedAt() expects `?string`, but setSetting() was passing a `DateTime` object. Format to `Y-m-d H:i:s` to match the convention from DashboardFactory::create(). AdminTemplateService (1): replace `Ramsey\Uuid\Uuid::uuid4()` with the same `random_bytes`-based generator used by DashboardFactory. Ramsey UUID is not a declared composer dep, so the previous code would have crashed at runtime — this surfaced because phpstan correctly couldn't resolve the class. FileService (2): two `Node::nodeExists()` / `Node::newFile()` / `Node::putContent()` calls were made on results of `Folder::get()`, which returns the `Node` interface. Type-narrow with explicit `instanceof Folder` / `instanceof File` checks (throwing `RuntimeException` on the impossible-but-typed branch) so phpstan can verify the methods exist. Side benefit: removes a stale `@phpstan-ignore-next-line` that was silencing a real downstream error. composer.json: drop the `|| echo 'PHPStan not installed, skipping...'` from the `phpstan` script. The `||` made every phpstan run exit 0, so the CI quality job was reporting green even when phpstan failed — exactly how the IL10N typo and the Ramsey-UUID call slipped through review. Now `composer phpstan` propagates the real exit code. Verified locally: phpstan `[OK] No errors`, phpcs clean across 49 files, phpunit 354/354 (the one pre-existing DashboardFactoryTest::testCreateRespectsExplicitPermissionLevel failure is fixed by PR #77 and out of scope here).
3 tasks
rubenvdlinde
added a commit
that referenced
this pull request
May 3, 2026
…reate (#77) DashboardFactoryTest::testCreateRespectsExplicitPermissionLevel was failing with "Unknown named parameter \$permissionLevel" because the factory hard-coded \$dashboard->setPermissionLevel(PERMISSION_FULL) and exposed no way for callers (or share / fork flows) to override it. Add the param, default to PERMISSION_FULL (preserving existing behaviour for the 9 other tests + every production caller, which are all positional), and pass it through to setPermissionLevel(). Tests: 354 / 354 passing.
rubenvdlinde
added a commit
that referenced
this pull request
May 3, 2026
PR #78 dropped phpstan from 47 → 26 by fixing the IL10N namespace typo. This batch clears the rest in four contained groups and removes the `|| echo` swallow from the composer script so future regressions actually fail CI. DashboardShareApiController (21): swap `DataResponse` → `JSONResponse` throughout the controller, matching the pattern used by every other passing controller in the app (DashboardApiController, ResponseHelper). The two classes are wire-compatible (both extend Response, same constructor signature, both serialise to JSON), but phpstan can resolve the JSONResponse generics from the existing docblocks while the bare DataResponse type was forcing it to infer T = `array<string, string>` and reject the call. AdminSettingMapper (2): the entity's setUpdatedAt() expects `?string`, but setSetting() was passing a `DateTime` object. Format to `Y-m-d H:i:s` to match the convention from DashboardFactory::create(). AdminTemplateService (1): replace `Ramsey\Uuid\Uuid::uuid4()` with the same `random_bytes`-based generator used by DashboardFactory. Ramsey UUID is not a declared composer dep, so the previous code would have crashed at runtime — this surfaced because phpstan correctly couldn't resolve the class. FileService (2): two `Node::nodeExists()` / `Node::newFile()` / `Node::putContent()` calls were made on results of `Folder::get()`, which returns the `Node` interface. Type-narrow with explicit `instanceof Folder` / `instanceof File` checks (throwing `RuntimeException` on the impossible-but-typed branch) so phpstan can verify the methods exist. Side benefit: removes a stale `@phpstan-ignore-next-line` that was silencing a real downstream error. composer.json: drop the `|| echo 'PHPStan not installed, skipping...'` from the `phpstan` script. The `||` made every phpstan run exit 0, so the CI quality job was reporting green even when phpstan failed — exactly how the IL10N typo and the Ramsey-UUID call slipped through review. Now `composer phpstan` propagates the real exit code. Verified locally: phpstan `[OK] No errors`, phpcs clean across 49 files, phpunit 354/354 (the one pre-existing DashboardFactoryTest::testCreateRespectsExplicitPermissionLevel failure is fixed by PR #77 and out of scope here).
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Repairs `DashboardFactoryTest::testCreateRespectsExplicitPermissionLevel`, which currently fails with:
```
Error: Unknown named parameter $permissionLevel
tests/Unit/Service/DashboardFactoryTest.php:116
```
The test passes `permissionLevel: Dashboard::PERMISSION_ADD_ONLY` as a named argument, but the factory's signature stops at `gridColumns` and the body hard-codes `setPermissionLevel(PERMISSION_FULL)`. The test was written ahead of the parameter — adding the param closes the gap and gives share/fork flows a way to create restricted dashboards without immediately mutating-then-saving.
What changed
`lib/Service/DashboardFactory.php` — append `string $permissionLevel = Dashboard::PERMISSION_FULL` to `create()`, pass it to `setPermissionLevel($permissionLevel)`, document the new param in the docblock.
Default value is `PERMISSION_FULL` so the 9 other unit tests + every production caller (none currently pass the param) are unaffected.
Verified locally