Show dial in number for running meetings - #1810
Conversation
WalkthroughChangesThe change captures dial-in details from meeting creation, stores and exposes them for active meetings, and adds a Vue phone-join overlay with call-link and QR-code support. Configuration, translations, dependencies, migrations, backend tests, frontend E2E coverage, and changelog entries are updated. Room dial-in persistence and API
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
01fe74c to
a79e92b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1810 +/- ##
=============================================
- Coverage 97.10% 93.41% -3.70%
- Complexity 1949 1950 +1
=============================================
Files 483 484 +1
Lines 16656 16722 +66
Branches 2408 2411 +3
=============================================
- Hits 16174 15621 -553
- Misses 482 1101 +619 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PILOS
 |
||||||||||||||||||||||||||||||||||||||||
| Project |
PILOS
|
| Branch Review |
1143-show-dial-in-number-for-running-meetings
|
| Run status |
|
| Run duration | 08m 30s |
| Commit |
|
| Committer | Samuel Weirich |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
2
|
 |
0
|
 |
0
|
 |
0
|
 |
634
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
| Test | Artifacts | |
|---|---|---|
| Rooms view meetings > join running meeting by phone |
Test Replay
Screenshots
|
|
| Test | Artifacts | |
|---|---|---|
| Room Join with lobby settings > Lobby enabled |
Test Replay
Screenshots
|
|
a79e92b to
d7195ec
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
database/migrations/2024_06_12_171150_add_dial_in_to_meetings_table.php (1)
14-17: Consider adding an index for the dial_number column.Since the
dial_numberwill likely be used in queries to check for invalid numbers and duplicates, adding an index could improve query performance.Schema::table('meetings', function (Blueprint $table) { $table->string('dial_number')->nullable(); $table->integer('voice_bridge')->nullable(); + $table->index('dial_number'); });resources/js/components/RoomJoinByPhoneButton.vue (1)
85-89: Improve phone number validation and formatting.The current validation only removes non-digits and '+'. Consider:
- Using a proper phone number validation library
- Formatting the number consistently for display
const link = computed(() => { - // Remove all chars that are not digits or '+' - const cleanNumber = props.number.replace(/[^0-9+]/g, ""); + // Use libphonenumber-js for validation and formatting + const phoneNumber = parsePhoneNumber(props.number); + if (!phoneNumber?.isValid()) return ''; + const cleanNumber = phoneNumber.format('E.164'); return `tel:${cleanNumber},${props.pin}#`; });app/Http/Resources/Room.php (1)
85-88: Optimize configuration access.Consider caching the invalid dial numbers configuration to avoid repeated file reads.
+ private $invalidDialNumbers; + + public function __construct($resource) + { + parent::__construct($resource); + $this->invalidDialNumbers = config('bigbluebutton.invalid_dial_numbers'); + // ... existing code ... + } 'dial_in' => $this->when( - $latestMeeting->end == null && !in_array($latestMeeting->dial_number, config('bigbluebutton.invalid_dial_numbers')), + $latestMeeting->end == null && !in_array($latestMeeting->dial_number, $this->invalidDialNumbers), [ 'number' => $latestMeeting->dial_number, 'pin' => $latestMeeting->voice_bridge, ] ),app/Services/RoomService.php (1)
87-90: Consider adding error handling for dial-in properties.The changes look good! The code correctly captures the meeting creation response and stores the dial-in information. However, consider adding null checks before accessing the dial-in properties to handle cases where the meeting service doesn't provide this information.
- $meeting->dial_number = $createMeetingResponse->getDialNumber(); - $meeting->voice_bridge = $createMeetingResponse->getVoiceBridge(); + $meeting->dial_number = $createMeetingResponse->getDialNumber() ?? null; + $meeting->voice_bridge = $createMeetingResponse->getVoiceBridge() ?? null;Also applies to: 105-106
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
app/Http/Resources/Room.php(1 hunks)app/Services/RoomService.php(2 hunks)config/bigbluebutton.php(1 hunks)database/migrations/2024_06_12_171150_add_dial_in_to_meetings_table.php(1 hunks)lang/en/rooms.php(1 hunks)package.json(2 hunks)resources/js/components/RoomJoinByPhoneButton.vue(1 hunks)resources/js/views/RoomsView.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Frontend Tests (5)
- GitHub Check: Frontend Tests (4)
- GitHub Check: Frontend Tests (3)
- GitHub Check: Frontend Tests (2)
- GitHub Check: Frontend Tests (1)
- GitHub Check: Visual Tests
- GitHub Check: System Tests
🔇 Additional comments (5)
resources/js/components/RoomJoinByPhoneButton.vue (1)
3-9: Great implementation of accessible UI elements!The component makes good use of ARIA labels, tooltips, and semantic HTML elements.
app/Http/Resources/Room.php (1)
85-88: Well-implemented security check for dial-in information.The code properly gates the dial-in information behind both meeting status and valid number checks.
resources/js/views/RoomsView.vue (1)
205-209: LGTM! Well-structured component integration.The
RoomJoinByPhoneButtoncomponent is properly integrated with appropriate conditional rendering and prop passing. The placement alongside other room control buttons maintains UI consistency.lang/en/rooms.php (1)
268-275: LGTM! Complete translation coverage for phone features.The translation strings are well-organized and provide comprehensive coverage for all phone-related UI elements. The structure aligns with the existing translation patterns.
package.json (1)
61-61: Verify security of new dependencies.The new dependencies support the phone join feature. Let's verify their security status and latest versions.
Also applies to: 77-77
✅ Verification successful
Dependencies verified as secure and up-to-date
Both
@vueuse/integrations(12.5.0) andqrcode(1.5.4) are actively maintained packages with no reported security advisories. The packages show recent updates and have established track records in the ecosystem.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories and latest versions of new dependencies # Check npm for latest versions and security advisories echo "Checking @vueuse/integrations..." npm view @vueuse/integrations version npm audit @vueuse/integrations echo "Checking qrcode..." npm view qrcode version npm audit qrcodeLength of output: 950
Script:
#!/bin/bash # Check GitHub security advisories and package stats echo "Checking @vueuse/integrations..." gh api /repos/vueuse/vueuse/security-advisories --jq 'length' npm view @vueuse/integrations downloads last-month npm view @vueuse/integrations time --json | jq -r '.modified,.created' echo "Checking qrcode..." gh api /repos/soldair/node-qrcode/security-advisories --jq 'length' npm view qrcode downloads last-month npm view qrcode time --json | jq -r '.modified,.created'Length of output: 575
| 'server_online_threshold' => (int) env('BBB_SERVER_ONLINE_THRESHOLD', 3), | ||
| 'server_offline_threshold' => (int) env('BBB_SERVER_OFFLINE_THRESHOLD', 3), | ||
|
|
||
| 'invalid_dial_numbers' => ['0', '613-555-1212', '613-555-1234', '0000'], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move invalid dial numbers to environment configuration.
The invalid dial numbers should be configurable via environment variables for better security and flexibility across different environments.
- 'invalid_dial_numbers' => ['0', '613-555-1212', '613-555-1234', '0000'],
+ 'invalid_dial_numbers' => array_filter(explode(',', env('BBB_INVALID_DIAL_NUMBERS', '0,0000'))),Also, consider removing specific area codes from the default configuration to avoid potential security implications of exposing geographic information.
📝 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.
| 'invalid_dial_numbers' => ['0', '613-555-1212', '613-555-1234', '0000'], | |
| 'invalid_dial_numbers' => array_filter(explode(',', env('BBB_INVALID_DIAL_NUMBERS', '0,0000'))), |
811f4b2 to
3953f08
Compare
88ee280 to
bc9287a
Compare
# Conflicts: # app/Http/Resources/RoomResource.php # package-lock.json # package.json
b2a0adf to
6b86e82
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Http/Resources/LastMeetingResource.php (1)
24-31: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse strict equality and the null-safe operator.
Consider using strict equality (
=== null) and the null-safe operator (?->) when accessing$this->server->error_countto prevent potential null pointer errors if a meeting's server relationship is unexpectedly null.♻️ Proposed refactor
- 'usage' => $this->when($this->end == null, [ + 'usage' => $this->when($this->end === null, [ 'participant_count' => $this->room->participant_count, ]), - 'dial_in' => $this->when($this->end == null, [ + 'dial_in' => $this->when($this->end === null, [ 'number' => $this->dial_number, 'pin' => $this->voice_bridge, ]), - 'server_connection_issues' => $this->end == null && $this->server->error_count > 0, + 'server_connection_issues' => $this->end === null && $this->server?->error_count > 0,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Http/Resources/LastMeetingResource.php` around lines 24 - 31, Update LastMeetingResource’s server_connection_issues expression to use strict null comparison for end and null-safe access when reading the server error count, preserving the existing condition that issues are reported only when the meeting has not ended and the count is greater than zero.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/Http/Resources/LastMeetingResource.php`:
- Around line 24-31: Update LastMeetingResource’s server_connection_issues
expression to use strict null comparison for end and null-safe access when
reading the server error count, preserving the existing condition that issues
are reported only when the meeting has not ended and the count is greater than
zero.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 044b6eed-0206-4d3c-ac0a-31ea222b1dd6
📒 Files selected for processing (2)
CHANGELOG.mdapp/Http/Resources/LastMeetingResource.php
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/Backend/Unit/RoomServiceTest.php (2)
22-28: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove redundant inline comments.
As per coding guidelines, prefer PHPDoc blocks over inline comments, using inline comments only for exceptionally complex logic. The code here is self-explanatory.
♻️ Proposed fix
protected function setUp(): void { parent::setUp(); - // Create room $this->room = Room::factory()->create(['access_code' => '123456789']); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Backend/Unit/RoomServiceTest.php` around lines 22 - 28, Remove the redundant inline “Create room” comment from setUp while leaving the Room factory initialization unchanged.Source: Coding guidelines
30-72: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd missing return type and remove inline comments.
As per coding guidelines, all methods must have explicit return type declarations, and inline comments should only be used for exceptionally complex logic. The inline comments here are redundant as the test code is self-documenting.
♻️ Proposed fix
- public function test_start_dial_in() + public function test_start_dial_in(): void { config(['bigbluebutton.invalid_dial_numbers' => ['0', '0000']]); $server = Server::factory()->create(); $bbbFaker = new BigBlueButtonServerFaker($server->base_url, $server->secret); $bbbFaker->addCreateMeetingRequest(); $bbbFaker->addCreateMeetingRequest(); $this->room->roomType->serverPool->servers()->attach($server); - // Start new meeting, result has a valid dial-in number $roomService = new RoomService($this->room); $roomService->start(); - // Check meeting was created $this->room->refresh(); $this->assertCount(1, $this->room->meetings); - // Check dial-in number and voice-bridge (pin) are set $meeting = $this->room->latestMeeting; $this->assertEquals('613-555-1234', $meeting->dial_number); $this->assertEquals('02443', $meeting->voice_bridge); - // Set meeting as ended $meetingService = new MeetingService($meeting); $meetingService->setEnd(); - // Change list of invalid dial-in numbers config(['bigbluebutton.invalid_dial_numbers' => ['0', '0000', '613-555-1234']]); - // Start new meeting, result has an invalid dial-in number $roomService->start(); - // Check another meeting was created $this->room->refresh(); $this->assertCount(2, $this->room->meetings); - // Check dial-in number and voice-bridge (pin) are not set $meeting = $this->room->latestMeeting; $this->assertNull($meeting->dial_number); $this->assertNull($meeting->voice_bridge); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Backend/Unit/RoomServiceTest.php` around lines 30 - 72, Update test_start_dial_in in RoomServiceTest to declare its explicit return type, using the test suite’s standard void return type. Remove the redundant inline comments from the method while preserving the existing test setup, assertions, and execution flow.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/Backend/Unit/RoomServiceTest.php`:
- Around line 22-28: Remove the redundant inline “Create room” comment from
setUp while leaving the Room factory initialization unchanged.
- Around line 30-72: Update test_start_dial_in in RoomServiceTest to declare its
explicit return type, using the test suite’s standard void return type. Remove
the redundant inline comments from the method while preserving the existing test
setup, assertions, and execution flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: de114ce1-96c2-4ada-b0d5-bec378b8c8ff
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
composer.jsondatabase/migrations/2026_07_20_171150_add_dial_in_to_meetings_table.phpresources/js/components/RoomJoinByPhoneButton.vuetests/Backend/Feature/api/v1/Room/RoomTest.phptests/Backend/Unit/RoomServiceTest.phptests/Backend/Utils/BigBlueButtonServerFaker.phptests/Frontend/e2e/RoomsViewMeetings.cy.js
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/js/components/RoomJoinByPhoneButton.vue
1 participant
Fixes #1143
Type
Checklist
Changes
Other information
Summary by CodeRabbit
New Features
Bug Fixes
Tests