-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix: aliases and capitalization of emails #52622
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
base: master
Are you sure you want to change the base?
Conversation
6402015
to
8759f57
Compare
❌ We are unable to process any of the uploaded JUnit XML files. Please ensure your files are in the right format. |
4290551
to
88e5c74
Compare
Signed-off-by: SebastianKrupinski <[email protected]>
e94efdf
to
7a614d1
Compare
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.
High level feedback
Did not test.
@@ -25,6 +27,8 @@ interface IHandleImipMessage extends ICalendar { | |||
* @since 26.0.0 | |||
* | |||
* @throws CalendarException on validation failure or calendar write error | |||
* | |||
* @deprecated 32.0.0 Use IHandleImip instead | |||
*/ | |||
public function handleIMipMessage(string $name, string $calendarData): void; |
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.
This method uses calendar data as string intentionally. It was chosen over a Sabre type to not leak Sabre into the public API.
Having Sabre in the public API means we might be forced to break our own API if Sabre changes.
With Symfony and Doctrine we have seen that. Our own layer of abstraction helps us to work around some of the 3rdparty changes. In rare cases API changes are necessary.
I will leave the decision with you.
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.
I see your point but I don't think this is an issue.
I think the main entry point for iMip (and most other functions) should be the calendar manager (string), and the manager then finds the correct calendar and forwards the request to the calendar it self.
External App -> CalendarManager::handleImip(string) -> Calendar::handleImip(VCALENDAR)
Any external app does not need to know anything about calendar searching or the sabre interface, the only to components that do are the manager and the calendar it self. But if any app does want to work directly with the calendar then it would need to know how to work with Sabre.
I think the benefits of not serializing and de-serializing every time we exit and enter a function, out weight the possibility of having to change our API in the future. I could also wrap the VCALENDAR object in a DTO, but that might be over kill.
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.
This is not the point. The issue is that you make consumers of the Nextcloud OCP API also dependent on Sabre code.
This will be tricky in terms of tooling (Psalm, IDEs) and impose potential breakage should Sabre change.
I can't imagine the serialization to be a real world problem, as we are passing one event at a time. IMO the ics is still the most flexible and framework-agnostic type here.
Signed-off-by: SebastianKrupinski <[email protected]>
5562499
to
584a214
Compare
Signed-off-by: Sebastian Krupinski <[email protected]>
Signed-off-by: SebastianKrupinski <[email protected]>
Design Consideration. The current logic replicates the current imip handling behaviour, that only handles changes to existing events. But taking in to consideration functionality from other apps, GH and client requests it might be a good idea to modify the CalendarManager::handleImip with a extra flag to create a new event if one does not already exist. aka Automatically adding tentative ivites @ChristophWurst i'll leave this decision to you. |
Let's take baby steps. Fix the alias mismatch in this PR and we look into importing of tentative events in another one. |
Thanks 👍 The change to loop over the attendees in handleImip also fixes #51115. Therefore, I have sunsetted #53046. I will check, when you are done here, if it makes sense to port some of the test cases, but most of them were relevant for creating the extra cancellation message, which is not required here. |
Summary
Testing
(Make sure "Restrict users to only share with users in their groups" (Administration Settings -> Sharing) is off) (There is a unrelated bug that will cause imip processing to fail)
Checklist