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

Include accept / decline links in CalDAV invitation emails #9942

Merged
merged 2 commits into from
Jul 11, 2018

Conversation

georgehrke
Copy link
Member

@georgehrke georgehrke commented Jun 20, 2018

fixes #2338

Todo:

  • provide screenshots for UI reviews
  • implement "more options" page
  • unit tests
  • garbage collection for tokens

To test this:

  • install thunderbird and setup a Nextcloud calendar in thunderbird
  • create an event and invite an attendee (be sure to create an event in the future, otherwise Nextcloud won't send out an email)
  • recipient will receive an email:
    accept-decline

Click Accept and wait till thunderbird refreshed the event from the server:
5bff1e6d-44ac-44f8-ae34-da0a04cadaa9

Click Decline and wait till thunderbird refreshed the event from the server:
fe3acd4f-1595-4e5b-8986-61e589fdfabc

Use more options to select Tentative:
5dcdba82-9b21-49cc-b386-cfb2b339c82b
c2be14e0-9ec4-40b4-a830-8d98c07c29d2

We use common X- parameters to store number of guests and comment in event, sadly thunderbird doesn't support that, but we will build that into the calendar app.

(If Thunderbird takes too long to refresh the event, you can try to move the event to a different way. Thunderbird will show you a warning about a sync conflict. Select to discard local changes and refresh event)

@warnerbryce
Copy link

I could help you for testing this feature.

I have a lot of mail clients - mail servers - and scenario to test

@georgehrke georgehrke force-pushed the feature/2338/invitation_response_in_email branch from 5f53787 to 346e205 Compare June 26, 2018 11:06
@codecov
Copy link

codecov bot commented Jun 26, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@326f07e). Click here to learn what that means.
The diff coverage is 43.5%.

@@            Coverage Diff            @@
##             master    #9942   +/-   ##
=========================================
  Coverage          ?   51.94%           
  Complexity        ?    26052           
=========================================
  Files             ?     1668           
  Lines             ?    96455           
  Branches          ?     1290           
=========================================
  Hits              ?    50103           
  Misses            ?    46352           
  Partials          ?        0
Impacted Files Coverage Δ Complexity Δ
apps/dav/composer/composer/autoload_static.php 0% <ø> (ø) 1 <0> (?)
apps/dav/templates/schedule-response-success.php 0% <0%> (ø) 0 <0> (?)
apps/dav/appinfo/routes.php 0% <0%> (ø) 0 <0> (?)
apps/dav/templates/schedule-response-options.php 0% <0%> (ø) 0 <0> (?)
apps/dav/composer/composer/autoload_classmap.php 0% <0%> (ø) 0 <0> (?)
apps/dav/templates/schedule-response-error.php 0% <0%> (ø) 0 <0> (?)
...av/lib/Migration/Version1006Date20180619154313.php 0% <0%> (ø) 2 <2> (?)
...AV/InvitationResponse/InvitationResponseServer.php 0% <0%> (ø) 8 <8> (?)
...av/lib/BackgroundJob/CleanupInvitationTokenJob.php 100% <100%> (ø) 2 <2> (?)
...av/lib/Controller/InvitationResponseController.php 71.13% <71.13%> (ø) 18 <18> (?)
... and 1 more

@warnerbryce
Copy link

warnerbryce commented Jun 26, 2018

Oh no continuous-integration failed, does is it bad for the feature freeze ?

@georgehrke georgehrke force-pushed the feature/2338/invitation_response_in_email branch from 346e205 to 9283c7b Compare June 26, 2018 22:49
@georgehrke georgehrke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 26, 2018
@MorrisJobke
Copy link
Member

Oh no continuous-integration failed, does is it bad for the feature freeze ?

We are struggling with 1 of the multiple thousand of tests we run on CI. This one is an acceptance test for the web UI where the started browser has some delays and thus the element was not rendered. A pretty hard to fix race condition. So it's fine, because it is a known limitation of this test suite. The remaining tests all succeeded so this is fine to be reviewed.

@warnerbryce
Copy link

warnerbryce commented Jun 27, 2018

I tried to test thoses changes but i got buged,
This is what i did :
Downloaded the last git from Master branch,
Changed the files from this pull
Tried to make an invitation and get a bug, something to do with data base :
SQLSTATE[42S02]: Base table or view not found: 1146 Table 'testcloud.oc_calendar_invitation_tokens' doesn't exist

Do i have to create a table myself ? Could you give us the syntaxe for adding oc_calendar_invitation_tokens ?

EDIT : Alright, it was my fault, i dropped my database, deleted config.php, start a new install and i got my table. So now, testing day

@warnerbryce
Copy link

warnerbryce commented Jun 27, 2018

Alright, i don't know if we should talk about this here or on help.nextcloud.com, but i've made some first tests, and i got some litte problems.

On iPhone the html won't display when i get invited, but if the event is canceled, the canceled mail get html read correctly...strange

img_1677
img_1678

When we go in the More Option page, it could be nice to have on the left or right, a recapitulation of the event for not getting troubled by severals invitations. And the answer to give with Guests and Comments, goes nowhere (for the moment ?) how the creator could be notified for this ?
moredetails

On Outlook.com webmail but it's the same on Gmail, we got a little (quite big in fact) banner for Accepting/Refusing/I don't known the event, but clicking on that will make an email to the creator, but will never change the event... I don't know if we can modify that or having a way to hide this... This banner is so big that we can't know that the buttons for answering are lower in the mail... And if we clic on this banner, the mail will go automaticly in the trashbin...
hotmail

It's functionning and it's a good start for following attendees. Bravo

@georgehrke
Copy link
Member Author

Alright, i don't know if we should talk about this here or on help.nextcloud.com, but i've made some first tests, and i got some litte problems.

This is the right place :)

On iPhone the html won't display when i get invited, but if the event is canceled, the canceled mail get html read correctly...strange

That's very odd, because it works for me:
img_6593

Anyway, the only HTML we inject is definitely valid: https://github.com/nextcloud/server/pull/9942/files#diff-d84119a7a4a15a279ced923237bd016bR503

So if anything, this is a bug in the E-Mail-Templating class and should be fixed separately.
Or do you use custom email templates by any chance?

When we go in the More Option page, it could be nice to have on the left or right, a recapitulation of the event for not getting troubled by severals invitations. And the answer to give with Guests and Comments, goes nowhere (for the moment ?) how the creator could be notified for this ?

That would be nice indeed, but also involves more much more work. This pull-request is already a vast enhancement, so I would just suggest to do smaller steps and implement the overview later on for Nextcloud 15.

On Outlook.com webmail but it's the same on Gmail, we got a little (quite big in fact) banner for Accepting/Refusing/I don't known the event, but clicking on that will make an email to the creator, but will never change the event... I don't know if we can modify that or having a way to hide this... This banner is so big that we can't know that the buttons for answering are lower in the mail... And if we clic on this banner, the mail will go automaticly in the trashbin...

This was there before adding those links as well and unfortunately I'm not aware of any way to get rid of those buttons. Your email client simply detects that this email contains an invitation and offers you to reply to that invitation by sending an email to the organizer. That mechanism is related to iMIP: https://tools.ietf.org/html/rfc6047

And yes, it doesn't change the events, because the user would have to manually import the ics files attached into their calendar. I was talking to @ChristophWurst the other day if we could integrate this with the mail app, so if you setup the mail app with your email account, it will automatically poll your email account in the background to check for new invitation responses and update the information in your calendar. Definitely an issue we are interested in solving, but not directly related to this pull-request.

@warnerbryce
Copy link

Some news. I got the email template bug on iphone with my Outlook.com address. With my imap address no graphical bug.
Really strange because on my outlook address my iPhone receive correctly the mails from the NC13 invitation

@MorrisJobke
Copy link
Member

@georgehrke Mind to rebase to get this finally in?

georgehrke and others added 2 commits June 29, 2018 10:44
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the feature/2338/invitation_response_in_email branch from 9283c7b to f12922c Compare June 29, 2018 08:46
@rullzer
Copy link
Member

rullzer commented Jun 29, 2018

I took the libery of rebasing

@georgehrke
Copy link
Member Author

@MorrisJobke @rullzer Can we merge this? :)

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

Only one minor nitpick, that could also be fixed later:

  • accepting/declining from the mail created a second attendee with the mailto: prefix in the Nextcloud calendar app

@georgehrke
Copy link
Member Author

accepting/declining from the mail created a second attendee with the mailto: prefix in the Nextcloud calendar app

I know what you mean. That's actually a bug in the calendar app. It's creating attendees with MAILTO: instead of mailto:. Not related to this PR.

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

The let do this 🎸 🚀 🍺

@rullzer rullzer merged commit d9aa5ed into master Jul 11, 2018
@rullzer rullzer deleted the feature/2338/invitation_response_in_email branch July 11, 2018 17:53
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accept calendar invitation directly in the invitation email
4 participants