Skip to content

fix: override iTip Broker to fix several issues - #48583

Merged
SebastianKrupinski merged 1 commit into
masterfrom
fix/overide-itip-broker
Oct 21, 2024
Merged

fix: override iTip Broker to fix several issues#48583
SebastianKrupinski merged 1 commit into
masterfrom
fix/overide-itip-broker

Conversation

@SebastianKrupinski

@SebastianKrupinskiSebastianKrupinski commented Oct 6, 2024

Copy link
Copy Markdown
Contributor

Summary

This over rides the iTip broker and the main iTip message generation routine to fix several issues.

Checklist

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good

Do you have instructions for us for some manual testing? Would it make sense to cover some of this with unit tests? parseEventForOrganizer appears to be free of side effects - event info in, messages out - something that's naturally nice to write tests for.

Comment threadapps/dav/lib/CalDAV/TipBroker.php Outdated
@SebastianKrupinski

Copy link
Copy Markdown
ContributorAuthor

Code looks good

Do you have instructions for us for some manual testing? Would it make sense to cover some of this with unit tests? parseEventForOrganizer appears to be free of side effects - event info in, messages out - something that's naturally nice to write tests for.

Morning, Sure I can write some test, for it, I originally skipped the tests because iTip broker message generation is already covered by most of our unit tests, but I'll write some test specifically for this, this week.

@SebastianKrupinski

SebastianKrupinski commented Oct 14, 2024

Copy link
Copy Markdown
ContributorAuthor

Do you have instructions for us for some manual testing?

For this, basically just run through the common actions a user would do while inviting an attendee,

  1. create an event with attendees
  2. update an event (location, description)
  3. cancel an event using status in the side bar

And basically see if the attendees received and email.

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merci for the test coverage 🙏

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
@SebastianKrupinski

SebastianKrupinski commented Oct 21, 2024

Copy link
Copy Markdown
ContributorAuthor

@ChristophWurst backport to 30 or further? We would need to backport the sabre patch to go further.

@ChristophWurst

Copy link
Copy Markdown
Member

@ChristophWurst backport to 30 or further? We would need to backport the sabre patch to go further.

nextcloud/3rdparty#1939 looks low risk. I'm okay with backporting this further than 30. 28-30 is our current support range: https://github.com/nextcloud/server/wiki/Maintenance-and-Release-Schedule

@SebastianKrupinski

Copy link
Copy Markdown
ContributorAuthor

/backport to stable30

@SebastianKrupinski

Copy link
Copy Markdown
ContributorAuthor

/backport to stable29

@SebastianKrupinski

Copy link
Copy Markdown
ContributorAuthor

/backport to stable28

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewWaiting for reviews

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants

@SebastianKrupinski@ChristophWurst@st3iny