Skip to content
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

feature: Improved Recurrence Invitations Messages #45547

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

SebastianKrupinski
Copy link
Contributor

@SebastianKrupinski SebastianKrupinski commented May 28, 2024

Summary

Improved Recurrence Invitations Messages

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Psalm found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@SebastianKrupinski
Copy link
Contributor Author

TODO: unit tests

Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

apps/dav/lib/CalDAV/EventReader.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReaderRDate.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReaderRRule.php Outdated Show resolved Hide resolved
apps/dav/tests/unit/CalDAV/EventReaderTest.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReaderRRule.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReaderRRule.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReaderRRule.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReader.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReader.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReader.php Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReader.php Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReaderRDate.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/Schedule/IMipService.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/Schedule/IMipService.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/Schedule/IMipService.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/Schedule/IMipService.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/Schedule/IMipService.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/Schedule/IMipService.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/Schedule/IMipService.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/Schedule/IMipService.php Show resolved Hide resolved
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

In general there are a lot of ternary operators. I'm not a big fan because they sacrifice readability for brevity.

I tagged the ones that are a bit problematic from my point of view. I'd also prefer some of the others to be refactored to regular if statements but have no hard opinions.

E.g. there are a lot of return cond ? a : b constructs.


It would also be nice to have some exemplary screenshots of the new messages or concrete steps for testing.

@SebastianKrupinski
Copy link
Contributor Author

It would also be nice to have some exemplary screenshots of the new messages or concrete steps for testing.

For testing just create a new calendar event with at least one attendee that has a valid email address.

Here are some screenshots of the new messages...

Singleton Partial Day Event

Screenshot 2024-06-25 174246

Singleton Complete Day Event

Screenshot 2024-06-25 175756

Recurring Daily Event

Screenshot 2024-06-25 174523

Recurring Weekly Event

Screenshot 2024-06-25 174959

Recurring Bi-Weekly Event

Screenshot 2024-06-25 175235

Recurring Month Event with Absolute Date

Screenshot 2024-06-25 175425

Recurring Monthly Event with Relative Date

Screenshot 2024-06-25 175615

apps/dav/lib/CalDAV/EventReaderRDate.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReaderRRule.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReaderRRule.php Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReaderRRule.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReaderRRule.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReader.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReader.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReader.php Show resolved Hide resolved
apps/dav/lib/CalDAV/EventReader.php Show resolved Hide resolved
apps/dav/lib/CalDAV/Schedule/IMipService.php Show resolved Hide resolved
@SebastianKrupinski SebastianKrupinski added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 9, 2024
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

LGTM! Extensive unit tests are great.

@SebastianKrupinski SebastianKrupinski merged commit 31d051a into master Jul 18, 2024
166 checks passed
@SebastianKrupinski SebastianKrupinski deleted the feature/recurrence-invitations2 branch July 18, 2024 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug feature: caldav Related to CalDAV internals feedback-requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Event update email unclear with recurrence exceptions
7 participants