-
Notifications
You must be signed in to change notification settings - Fork 241
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
Create Talk rooms for appointments #4025
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #4025 +/- ##
============================================
+ Coverage 22.55% 22.59% +0.04%
- Complexity 353 373 +20
============================================
Files 233 236 +3
Lines 11372 11475 +103
Branches 2144 2149 +5
============================================
+ Hits 2565 2593 +28
- Misses 8807 8882 +75
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 11 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
return; | ||
} | ||
|
||
if (!$event->getConfig()->getCreateTalkRoom()) { |
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.
$event->getConfig()
might as well be a local variable.
$broker->createConversation( | ||
$event->getConfig()->getName(), | ||
[$organizer], | ||
$broker->newConversationOptions()->setPublic(false), |
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.
If the link is to be sent shouldn't the conversation be public if the attendee is not an user?
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.
Yet I missed to make it public: #5516
Right now there is only a verification email, not a confirmation email for the final booking. We'll have to add the latter to propagate the link. The verification email can't have the Talk link as we only create the conversation once the booking is confirmed. |
string $displayName, | ||
string $email, | ||
?string $description = null, | ||
?string $location = null) : string { |
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.
The location is on the $config object (unless you want to refactor that and allow a talk room per booking, not per config?)
$booking->getDisplayName(), | ||
$booking->getEmail(), | ||
$booking->getDescription(), | ||
$config->getCreateTalkRoom() ? $booking->getTalkUrl() : $config->getLocation(), |
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.
@miaulalala 👀 an appointment with a Talk room is implicitly taking place online, so there is no location to enter for the organizer and we will write the Talk URL instead
4669ebc
to
eef6116
Compare
Linter is not happy yet @hamza221 did you have to change anthing except resolveing conflicts? Then approve the PR if it works. I'm not allowed to review my own PR :) |
Edit: info.xml missing so the migration didn't run. |
fe69765
to
ac52bb6
Compare
👍 on Hamza's latest state. The integration works. |
Signed-off-by: Christoph Wurst <[email protected]>
0d86766
to
74fc70d
Compare
Fixes #3480