Fix provisioning BBB servers#3014
Merged
samuelwei merged 12 commits intoTHM-Health:developfrom Apr 8, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMake provisioning JSON sections optional with null-safe guards; ensure BBB server endpoints are normalized to end with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🤖 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/Console/Commands/ProvisionCommand.php`:
- Around line 87-93: The role provisioning path fails for empty permission
groups because $permissions is only created inside the nested grant loop; update
the role-handling logic in ProvisioningService (around lines 210-223) to
initialize $permissions = [] before iterating grants, then populate it if grants
exist (e.g., foreach ($role->permissions as $namespace => $grants) { foreach
((array)$grants as $g) { $permissions[] = ... } }), and ensure you cast empty
grant containers to arrays so a role with permissions: {} or {"rooms": []}
results in an empty $permissions array and still calls the role creation
routine.
- Around line 64-110: The code calls count() and get_object_vars() on untrusted
$data properties (e.g. $data->servers->add, $data->server_pools->add,
$data?->room_types->add, $data->roles->add, $data->users->add and
$data->settings) which can throw TypeError for non-countable/non-object values;
before each count() or foreach, check and normalize the value (use
is_countable() or is_array() and cast to array (or skip) for *.add lists, and
ensure $data->settings is an object before calling get_object_vars()—convert or
initialize it to an object or array as needed—and make the null-safe access
consistent (remove the stray ? in $data?->room_types->add); then proceed to call
provision methods like $this->provision->server->create, serverPool->create,
roomType->create, role->create, user->create and settings->set only with
validated/cast data.
In `@CHANGELOG.md`:
- Line 16: The changelog entry "Fix provisioning BBB servers ([`#3011`])"
references an undefined link label; update that line to use the actual, declared
changelog reference or a full issue/PR URL so the link resolves (e.g., replace
"([`#3011`])" with the existing reference label used in the file or the canonical
issue/PR link), ensuring the label you use matches one declared in the reference
list at the bottom of the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 85f9ed5a-41c6-4302-bd49-72338fb080f0
📒 Files selected for processing (4)
CHANGELOG.mdapp/Console/Commands/ProvisionCommand.phpapp/Services/ProvisioningService.phpdocs/docs/administration/08-advanced/07-provisioning.md
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3014 +/- ##
=============================================
+ Coverage 96.70% 96.71% +0.01%
- Complexity 1905 1911 +6
=============================================
Files 445 445
Lines 12885 12892 +7
Branches 2077 2077
=============================================
+ Hits 12460 12469 +9
+ Misses 425 423 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pizkaz
force-pushed
the
fix/provisioning-bbb-servers
branch
3 times, most recently
from
fd896a6 to
02f4d5f
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/Console/Commands/ProvisionCommand.php (2)
62-68: Minor:$data->servers->add ?? []is evaluated twice.The
addarray is evaluated once forcount()and again forforeach. This is negligible for performance but could be consolidated for clarity.♻️ Optional: assign once and reuse
if (isset($data->servers)) { - $n = count($data->servers->add ?? []); - info("Provisioning $n servers"); - foreach ($data->servers->add ?? [] as $item) { + $add = $data->servers->add ?? []; + info('Provisioning ' . count($add) . ' servers'); + foreach ($add as $item) { $this->provision->server->create($item); } }This pattern could be applied to all similar blocks (server_pools, room_types, roles, users).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Console/Commands/ProvisionCommand.php` around lines 62 - 68, The code evaluates $data->servers->add twice; assign the add array to a local variable (e.g., $serversToAdd = $data->servers->add ?? []) then use count($serversToAdd) and foreach ($serversToAdd as $item) to avoid duplicate evaluation and improve clarity; apply the same refactor pattern to the other similar blocks (server_pools, room_types, roles, users) and keep calls to $this->provision->server->create($item) unchanged.
44-57: Wipe conditions are correct but could be more readable.The nested ternary
isset($data->X) ? $data->X->wipe ?? false : falseworks correctly but is verbose. A simpler pattern using logical AND short-circuits identically:♻️ Optional simplification
- if (isset($data->room_types) ? $data->room_types->wipe ?? false : false) { + if (isset($data->room_types) && ($data->room_types->wipe ?? false)) { $this->provision->roomType->destroy(); } - if (isset($data->server_pools) ? $data->server_pools->wipe ?? false : false) { + if (isset($data->server_pools) && ($data->server_pools->wipe ?? false)) { $this->provision->serverPool->destroy(); } - if (isset($data->servers) ? $data->servers->wipe ?? false : false) { + if (isset($data->servers) && ($data->servers->wipe ?? false)) { $this->provision->server->destroy(); } - if (isset($data->roles) ? $data->roles->wipe ?? false : false) { + if (isset($data->roles) && ($data->roles->wipe ?? false)) { $this->provision->role->destroy(); } - if (isset($data->users) ? $data->users->wipe ?? false : false) { + if (isset($data->users) && ($data->users->wipe ?? false)) { $this->provision->user->destroy(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Console/Commands/ProvisionCommand.php` around lines 44 - 57, The repeated nested ternary checks for wipe flags are correct but verbose; replace each pattern like isset($data->room_types) ? $data->room_types->wipe ?? false : false with a short-circuit expression using logical AND and null coalescing (e.g., isset($data->room_types) && ($data->room_types->wipe ?? false)) for the roomType, serverPool, server, role, and user wipe checks that call $this->provision->roomType->destroy(), $this->provision->serverPool->destroy(), $this->provision->server->destroy(), $this->provision->role->destroy(), and $this->provision->user->destroy() respectively so the semantics remain identical but the code is more readable.tests/Backend/Unit/ProvisioningServiceTest.php (1)
152-162: Consider adding a test case for endpoints already ending with a slash.While
Str::finish()correctly handles this (it won't double the slash), an explicit test would document the expected behavior and guard against regressions if the implementation changes.🧪 Optional additional test
/** * Test server creation with endpoint already having trailing slash */ public function test_server_create_endpoint_with_trailing_slash() { $this->testServer->endpoint = 'https://bbb.testdoma.in/'; $this->svc->server->create($this->testServer); $server = Server::firstWhere('name', $this->testServer->name); $this->assertNotNull($server); $this->assertEquals('https://bbb.testdoma.in/', $server->base_url); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Backend/Unit/ProvisioningServiceTest.php` around lines 152 - 162, Add a new unit test that verifies create() preserves a trailing slash on endpoints so it doesn't double or strip it: extend the tests near test_server_create by adding a test (e.g., test_server_create_endpoint_with_trailing_slash) that sets $this->testServer->endpoint to a URL that already ends with '/' before calling $this->svc->server->create($this->testServer), then fetch the record with Server::firstWhere('name', $this->testServer->name) and assert the saved base_url equals the original URL with the trailing slash; this will document and protect the behavior handled by Str::finish() in the create() implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/Console/Commands/ProvisionCommand.php`:
- Around line 62-68: The code evaluates $data->servers->add twice; assign the
add array to a local variable (e.g., $serversToAdd = $data->servers->add ?? [])
then use count($serversToAdd) and foreach ($serversToAdd as $item) to avoid
duplicate evaluation and improve clarity; apply the same refactor pattern to the
other similar blocks (server_pools, room_types, roles, users) and keep calls to
$this->provision->server->create($item) unchanged.
- Around line 44-57: The repeated nested ternary checks for wipe flags are
correct but verbose; replace each pattern like isset($data->room_types) ?
$data->room_types->wipe ?? false : false with a short-circuit expression using
logical AND and null coalescing (e.g., isset($data->room_types) &&
($data->room_types->wipe ?? false)) for the roomType, serverPool, server, role,
and user wipe checks that call $this->provision->roomType->destroy(),
$this->provision->serverPool->destroy(), $this->provision->server->destroy(),
$this->provision->role->destroy(), and $this->provision->user->destroy()
respectively so the semantics remain identical but the code is more readable.
In `@tests/Backend/Unit/ProvisioningServiceTest.php`:
- Around line 152-162: Add a new unit test that verifies create() preserves a
trailing slash on endpoints so it doesn't double or strip it: extend the tests
near test_server_create by adding a test (e.g.,
test_server_create_endpoint_with_trailing_slash) that sets
$this->testServer->endpoint to a URL that already ends with '/' before calling
$this->svc->server->create($this->testServer), then fetch the record with
Server::firstWhere('name', $this->testServer->name) and assert the saved
base_url equals the original URL with the trailing slash; this will document and
protect the behavior handled by Str::finish() in the create() implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fc5201b6-92d9-4c48-8763-b34feb02afb4
📒 Files selected for processing (5)
CHANGELOG.mdapp/Console/Commands/ProvisionCommand.phpapp/Services/ProvisioningService.phpdocs/docs/administration/08-advanced/07-provisioning.mdtests/Backend/Unit/ProvisioningServiceTest.php
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/docs/administration/08-advanced/07-provisioning.md
samuelwei
requested changes
Apr 7, 2026
pizkaz
commented
Apr 7, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Backend/Unit/Console/ProvisionCommandTest.php`:
- Around line 39-55: The fixture provisioning_data-partial.json doesn't include
the data the test_provisioning_partial expects; update the fixture so the
ProvisionCommand (which only reads the fixture's "add" arrays) will create the
expected records: add a server_pools.add entry with name "default"; add a
room_types object containing an add array with four room type objects (include
one with name "Meeting") following the structure used in provisioning_data.json;
and set settings.recording.recording_retention_period to the string "365" to
match the fixture format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 402b7088-7066-4bd1-8154-7bbfb44ccb38
📒 Files selected for processing (3)
app/Console/Commands/ProvisionCommand.phptests/Backend/Fixtures/provisioning_data-partial.jsontests/Backend/Unit/Console/ProvisionCommandTest.php
✅ Files skipped from review due to trivial changes (1)
- tests/Backend/Fixtures/provisioning_data-partial.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Console/Commands/ProvisionCommand.php`:
- Around line 44-57: Move the payload file reading and json_decode call into the
existing try block inside the ProvisionCommand handler, replace json_decode with
json_decode(..., flags: JSON_THROW_ON_ERROR) so invalid JSON throws, and
explicitly check the return of file_get_contents() before decoding (throw an
exception if it returns false or empty). Ensure the catch block handles the
thrown exceptions so the command fails instead of silently proceeding; update
references where $data is used (the null-safe checks around $data?->room_types
etc. and the transaction block that calls $this->provision->roomType->destroy(),
serverPool->destroy(), server->destroy(), role->destroy(), user->destroy()) to
rely on a validated $data object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a0184dd0-4bf3-40b6-beff-b039df5d8877
📒 Files selected for processing (1)
app/Console/Commands/ProvisionCommand.php
Comment on lines
+44
to
57
| if ($data?->room_types?->wipe ?? false) { | ||
| $this->provision->roomType->destroy(); | ||
| } | ||
| if ($data->server_pools->wipe) { | ||
| if ($data?->server_pools?->wipe ?? false) { | ||
| $this->provision->serverPool->destroy(); | ||
| } | ||
| if ($data->servers->wipe) { | ||
| if ($data?->servers?->wipe ?? false) { | ||
| $this->provision->server->destroy(); | ||
| } | ||
| if ($data->roles->wipe) { | ||
| if ($data?->roles?->wipe ?? false) { | ||
| $this->provision->role->destroy(); | ||
| } | ||
| if ($data->users->wipe) { | ||
| if ($data?->users?->wipe ?? false) { | ||
| $this->provision->user->destroy(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the ProvisionCommand.php file to see the full context
find . -type f -name "ProvisionCommand.php" | head -5Repository: THM-Health/PILOS
Length of output: 103
🏁 Script executed:
# Get the file size and read it
wc -l app/Console/Commands/ProvisionCommand.phpRepository: THM-Health/PILOS
Length of output: 105
🏁 Script executed:
# Read the entire ProvisionCommand.php to understand the payload handling
cat -n app/Console/Commands/ProvisionCommand.phpRepository: THM-Health/PILOS
Length of output: 4795
Move payload decoding into the try block and enforce a valid JSON object root.
Lines 38-40 currently decode outside the try-catch, so json_decode() and file_get_contents() errors are unhandled. If the JSON file is invalid or missing, both return falsy values that silently pass through all the null-safe and isset guards (lines 44-110), causing the command to succeed with an empty transaction. Add explicit validation by moving the decode into the try block, using JSON_THROW_ON_ERROR to catch invalid JSON, and checking file_get_contents() explicitly.
Suggested approach
- $data = json_decode(file_get_contents($this->argument('path')));
-
- try {
+ try {
+ $json = file_get_contents($this->argument('path'));
+ if ($json === false) {
+ throw new \RuntimeException("Could not read provisioning file '{$this->argument('path')}'");
+ }
+
+ $data = json_decode($json, false, 512, JSON_THROW_ON_ERROR);
+ if (! is_object($data)) {
+ throw new \UnexpectedValueException('Provisioning data must be a JSON object.');
+ }
+
DB::beginTransaction();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Console/Commands/ProvisionCommand.php` around lines 44 - 57, Move the
payload file reading and json_decode call into the existing try block inside the
ProvisionCommand handler, replace json_decode with json_decode(..., flags:
JSON_THROW_ON_ERROR) so invalid JSON throws, and explicitly check the return of
file_get_contents() before decoding (throw an exception if it returns false or
empty). Ensure the catch block handles the thrown exceptions so the command
fails instead of silently proceeding; update references where $data is used (the
null-safe checks around $data?->room_types etc. and the transaction block that
calls $this->provision->roomType->destroy(), serverPool->destroy(),
server->destroy(), role->destroy(), user->destroy()) to rely on a validated
$data object.
# Conflicts: # CHANGELOG.md
samuelwei
force-pushed
the
fix/provisioning-bbb-servers
branch
from
2f1b6c0 to
5c65908
Compare
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
2 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.
Fixes #3011
Type
Checklist
Changes
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Chores