Greenlight 2/3 Import: Preparations for import of recordings#3034
Open
pizkaz wants to merge 4 commits intoTHM-Health:developfrom
Open
Greenlight 2/3 Import: Preparations for import of recordings#3034pizkaz wants to merge 4 commits intoTHM-Health:developfrom
pizkaz wants to merge 4 commits intoTHM-Health:developfrom
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:
WalkthroughAdds reading of external meeting IDs from Greenlight v2/v3 room imports ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3034 +/- ##
=============================================
+ Coverage 96.73% 96.77% +0.03%
+ Complexity 1924 1923 -1
=============================================
Files 457 457
Lines 13002 13137 +135
Branches 2088 2133 +45
=============================================
+ Hits 12578 12713 +135
Misses 424 424 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pizkaz
force-pushed
the
feat/import-greenlight-recordings
branch
from
7289bae to
9372868
Compare
![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: 2
🤖 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/ImportGreenlight2Command.php`:
- Around line 391-395: Before creating a Meeting record, check that
$room->bbb_id is not null; if it is null, do not set $dbMeeting->id or call
$dbMeeting->save()—instead skip creating the Meeting (and optionally log a
warning) to avoid violating the non-null primary key constraint; update the
block that creates the Meeting (where $dbMeeting = new Meeting, $dbMeeting->id =
$room->bbb_id, $dbMeeting->room()->associate($dbRoom), $dbMeeting->save()) to
guard on $room->bbb_id and only run those lines when it is non-null.
In `@app/Console/Commands/ImportGreenlight3Command.php`:
- Around line 305-309: Add a defensive null/empty check for $room->meeting_id
before instantiating Meeting: in ImportGreenlight3Command (around the block
creating $dbMeeting) verify that $room->meeting_id is not null/empty (and
optionally numeric) and only then create new Meeting, set $dbMeeting->id and
associate room()->associate($dbRoom); if it is null/empty, skip creation and
optionally log or record the missing meeting_id for later; mirror the same
validation used in the GL2 importer for consistency.
🪄 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: e1e4a130-cba1-474f-8d04-10d34acb2b47
📒 Files selected for processing (7)
app/Console/Commands/ImportGreenlight2Command.phpapp/Console/Commands/ImportGreenlight3Command.phpapp/Models/RecordingFormat.phptests/Backend/Unit/Console/ImportGreenlight2Test.phptests/Backend/Unit/Console/ImportGreenlight3Test.phptests/Backend/Unit/Console/helper/Greenlight2Room.phptests/Backend/Unit/Console/helper/Greenlight3Room.php
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/Backend/Unit/Console/ImportGreenlight3Test.php
- app/Models/RecordingFormat.php
- tests/Backend/Unit/Console/helper/Greenlight2Room.php
- tests/Backend/Unit/Console/helper/Greenlight3Room.php
pizkaz
force-pushed
the
feat/import-greenlight-recordings
branch
from
9372868 to
6b8df6e
Compare
pizkaz
force-pushed
the
feat/import-greenlight-recordings
branch
from
6b8df6e to
927c831
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/docs/administration/08-advanced/06-migrate-greenlight.md (1)
6-6: Tighten the updated prose.A few changed sentences have small grammar/readability issues.
📝 Proposed wording cleanup
-PILOS provides an easy to use command to import all Greenlight users (incl. ldap), rooms (incl. default presentation) and shared accesses. +PILOS provides an easy-to-use command to import all Greenlight users (incl. LDAP), rooms (incl. default presentation) and shared accesses.-The command will output the process of the import and informs about failed user, room, room presentation and shared access import. +The command will show the import progress and report failed user, room, room presentation and shared access imports.-This meeting does not have a start- or end timestamp, so it not visible in the frontend. Associated recordings *will* be listed, however. +This meeting does not have a start or end timestamp, so it is not visible in the frontend. Associated recordings *will* be listed, however.Also applies to: 34-34, 140-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/administration/08-advanced/06-migrate-greenlight.md` at line 6, The sentence "PILOS provides an easy to use command to import all Greenlight users (incl. ldap), rooms (incl. default presentation) and shared accesses." has grammar and clarity issues—replace it with a tighter, more readable version such as: "PILOS provides an easy-to-use command to import all Greenlight users (including LDAP), rooms (including the default presentation), and shared accesses." Update the same style in the other occurrences referenced (lines containing similar phrasing at 34 and 140–141) to ensure consistent hyphenation, expanded acronyms, and parallel list structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/administration/08-advanced/06-migrate-greenlight.md`:
- Around line 138-147: Update the paragraph describing the import command to
clarify that the importer creates a Meeting row only when it creates a new room
(i.e., it skips room-id collisions and does not create a Meeting for
pre-existing rooms); mention that users importing recordings for existing rooms
must first create or verify the Meeting-to-room association themselves (check
the rooms table `bbb_id`/`meeting_id`) before packing/moving tar files to PILOS'
`recordings-spool`.
---
Nitpick comments:
In `@docs/docs/administration/08-advanced/06-migrate-greenlight.md`:
- Line 6: The sentence "PILOS provides an easy to use command to import all
Greenlight users (incl. ldap), rooms (incl. default presentation) and shared
accesses." has grammar and clarity issues—replace it with a tighter, more
readable version such as: "PILOS provides an easy-to-use command to import all
Greenlight users (including LDAP), rooms (including the default presentation),
and shared accesses." Update the same style in the other occurrences referenced
(lines containing similar phrasing at 34 and 140–141) to ensure consistent
hyphenation, expanded acronyms, and parallel list structure.
🪄 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: e80324e5-e5c0-4985-b11c-2fb450b3328d
📒 Files selected for processing (8)
app/Console/Commands/ImportGreenlight2Command.phpapp/Console/Commands/ImportGreenlight3Command.phpapp/Models/RecordingFormat.phpdocs/docs/administration/08-advanced/06-migrate-greenlight.mdtests/Backend/Unit/Console/ImportGreenlight2Test.phptests/Backend/Unit/Console/ImportGreenlight3Test.phptests/Backend/Unit/Console/helper/Greenlight2Room.phptests/Backend/Unit/Console/helper/Greenlight3Room.php
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/Backend/Unit/Console/helper/Greenlight3Room.php
- tests/Backend/Unit/Console/ImportGreenlight3Test.php
- tests/Backend/Unit/Console/helper/Greenlight2Room.php
- tests/Backend/Unit/Console/ImportGreenlight2Test.php
pizkaz
force-pushed
the
feat/import-greenlight-recordings
branch
from
d3c3f13 to
9cc2188
Compare
pizkaz
force-pushed
the
feat/import-greenlight-recordings
branch
from
9cc2188 to
fe44731
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/docs/administration/08-advanced/06-migrate-greenlight.md (1)
138-141:⚠️ Potential issue | 🟡 MinorClarify that Meetings are created only for newly imported rooms.
Line 140 states the command creates a Meeting "for every imported room", but the importer skips room-id collisions and creates
Meetingrecords only when creating new rooms. Users importing recordings for pre-existing/skipped rooms must create or verify the Meeting association themselves before importing recordings.Additionally, "existing rooms" in line 140 is ambiguous—it could mean rooms that pre-existed in PILOS or rooms from Greenlight.
📝 Proposed documentation fix
-You can also import recordings for existing rooms. To make this possible the import command creates a meeting with the BBB meeting ID for every imported room. +You can also import recordings for rooms imported by this command. For each newly imported room, the import command creates a meeting with the BBB meeting ID. +If a room already existed in PILOS and was skipped by the import, create or verify the corresponding meeting association before importing recordings. This meeting does not have a start- or end timestamp, so it not visible in the frontend. Associated recordings _will_ be listed, however.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/administration/08-advanced/06-migrate-greenlight.md` around lines 138 - 141, Update the paragraph about importing recordings to clarify that the importer creates a Meeting record only when it creates a new Room (it skips on room-id collisions), so it does not create Meetings for pre-existing rooms; replace the ambiguous phrase "existing rooms" with explicit wording distinguishing "rooms that already exist in your PILOS database" versus "rooms from Greenlight", and add a short note that users importing recordings for pre-existing/skipped rooms must manually create or verify the Meeting association before importing recordings (refer to the importer/Meeting behavior and room-id collision handling).
🧹 Nitpick comments (1)
docs/docs/administration/08-advanced/06-migrate-greenlight.md (1)
145-145: Clarify thatroomstable refers to the Greenlight database.Step 1 instructs users to find the meeting ID in the
roomstable, but doesn't specify whether this is the Greenlight or PILOS database. Users should query the Greenlight database.📝 Proposed clarification
-1. find the meeting ID (column `bbb_id` (GL2) or `meeting_id` (GL3) in the `rooms` table) +1. find the meeting ID (column `bbb_id` (GL2) or `meeting_id` (GL3) in Greenlight's `rooms` table)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/administration/08-advanced/06-migrate-greenlight.md` at line 145, Clarify that the "rooms" table referenced in step 1 is in the Greenlight database by updating the sentence to read something like: "find the meeting ID in the Greenlight app database (the rooms table — column `bbb_id` for GL2 or `meeting_id` for GL3)"; ensure the mention of `rooms`, `bbb_id`, `meeting_id`, and GL2/GL3 stays intact so readers know which database and columns to query.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/administration/08-advanced/06-migrate-greenlight.md`:
- Line 6: Replace the unhyphenated compound adjective "easy to use" with the
hyphenated form "easy-to-use" in the sentence that reads "PILOS provides an easy
to use command to import all Greenlight users (incl. ldap), rooms (incl. default
presentation) and shared accesses." to correctly form the compound adjective
before the noun; update only that phrase ("easy to use" → "easy-to-use")
preserving the rest of the sentence and punctuation.
---
Duplicate comments:
In `@docs/docs/administration/08-advanced/06-migrate-greenlight.md`:
- Around line 138-141: Update the paragraph about importing recordings to
clarify that the importer creates a Meeting record only when it creates a new
Room (it skips on room-id collisions), so it does not create Meetings for
pre-existing rooms; replace the ambiguous phrase "existing rooms" with explicit
wording distinguishing "rooms that already exist in your PILOS database" versus
"rooms from Greenlight", and add a short note that users importing recordings
for pre-existing/skipped rooms must manually create or verify the Meeting
association before importing recordings (refer to the importer/Meeting behavior
and room-id collision handling).
---
Nitpick comments:
In `@docs/docs/administration/08-advanced/06-migrate-greenlight.md`:
- Line 145: Clarify that the "rooms" table referenced in step 1 is in the
Greenlight database by updating the sentence to read something like: "find the
meeting ID in the Greenlight app database (the rooms table — column `bbb_id` for
GL2 or `meeting_id` for GL3)"; ensure the mention of `rooms`, `bbb_id`,
`meeting_id`, and GL2/GL3 stays intact so readers know which database and
columns to query.
🪄 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: 1cde5c2a-127b-4df1-b5b0-71edffcf4c57
📒 Files selected for processing (2)
CHANGELOG.mddocs/docs/administration/08-advanced/06-migrate-greenlight.md
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
| --- | ||
|
|
||
| PILOS provides an easy to use command to import all Greenlight users (incl. ldap), rooms and shared accesses. | ||
| PILOS provides an easy to use command to import all Greenlight users (incl. ldap), rooms (incl. default presentation) and shared accesses. |
Contributor
There was a problem hiding this comment.
Fix compound adjective hyphenation.
The phrase "easy to use" should be hyphenated when used as a compound adjective before a noun.
📝 Proposed grammar fix
-PILOS provides an easy to use command to import all Greenlight users (incl. ldap), rooms (incl. default presentation) and shared accesses.
+PILOS provides an easy-to-use command to import all Greenlight users (incl. ldap), rooms (incl. default presentation) and shared accesses.📝 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.
Suggested change
| PILOS provides an easy to use command to import all Greenlight users (incl. ldap), rooms (incl. default presentation) and shared accesses. | |
| PILOS provides an easy-to-use command to import all Greenlight users (incl. ldap), rooms (incl. default presentation) and shared accesses. |
🧰 Tools
🪛 LanguageTool
[grammar] ~6-~6: Use a hyphen to join words.
Context: ...ght to PILOS --- PILOS provides an easy to use command to import all Greenlight use...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/docs/administration/08-advanced/06-migrate-greenlight.md` at line 6,
Replace the unhyphenated compound adjective "easy to use" with the hyphenated
form "easy-to-use" in the sentence that reads "PILOS provides an easy to use
command to import all Greenlight users (incl. ldap), rooms (incl. default
presentation) and shared accesses." to correctly form the compound adjective
before the noun; update only that phrase ("easy to use" → "easy-to-use")
preserving the rest of the sentence and punctuation.
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 #
Type
Checklist
Changes
Summary by CodeRabbit