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

don't send invitation emails for past events #5304

Merged

Conversation

georgehrke
Copy link
Member

@georgehrke georgehrke commented Jun 8, 2017

fixes #2855

@georgehrke georgehrke added the 2. developing Work in progress label Jun 8, 2017
@georgehrke georgehrke added this to the Nextcloud 13 milestone Jun 8, 2017
@georgehrke georgehrke force-pushed the bugfix/2855/dont_send_invitations_for_past_events branch from 6d74321 to 677267b Compare June 9, 2017 10:13
@codecov
Copy link

codecov bot commented Jun 9, 2017

Codecov Report

Merging #5304 into master will decrease coverage by 0.62%.
The diff coverage is 71.79%.

@@             Coverage Diff              @@
##             master    #5304      +/-   ##
============================================
- Coverage     54.49%   53.86%   -0.63%     
- Complexity    21220    22777    +1557     
============================================
  Files          1313     1405      +92     
  Lines         82440    86748    +4308     
  Branches       1329     1328       -1     
============================================
+ Hits          44922    46727    +1805     
- Misses        37518    40021    +2503
Impacted Files Coverage Δ Complexity Δ
apps/dav/appinfo/v1/caldav.php 0% <0%> (ø) 0 <0> (ø) ⬇️
config/config.sample.php 0% <0%> (ø) 0 <0> (ø) ⬇️
apps/dav/lib/Server.php 47.82% <100%> (+0.38%) 13 <0> (+1) ⬆️
apps/dav/lib/CalDAV/Schedule/IMipPlugin.php 75.36% <80.64%> (+4.3%) 22 <9> (+21) ⬆️
lib/private/Files/ObjectStore/S3.php 0% <0%> (-73.69%) 5% <0%> (ø)
apps/files/appinfo/app.php 43.47% <0%> (-56.53%) 0% <0%> (ø)
...ib/private/Files/ObjectStore/S3ConnectionTrait.php 0% <0%> (-54.17%) 22% <0%> (ø)
apps/files_sharing/appinfo/app.php 43.9% <0%> (-46.1%) 0% <0%> (ø)
apps/user_ldap/templates/part.settingcontrols.php 0% <0%> (-42.86%) 0% <0%> (ø)
apps/files_trashbin/appinfo/app.php 60% <0%> (-40%) 0% <0%> (ø)
... and 447 more

@@ -52,6 +52,7 @@
$calDavBackend = new CalDavBackend($db, $principalBackend, $userManager, $random, $dispatcher, true);

$debugging = \OC::$server->getConfig()->getSystemValue('debug', false);
$sendInvitations = \OC::$server->getConfig()->getSystemValue('caldav_send_invitations', true);
Copy link
Member

Choose a reason for hiding this comment

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

I'm usually not really a fan of configuration values. Is there a reason we do really need to have that one? 🙈

Also stuff in the app config is usually easier to deploy on clusters :)

Copy link
Member Author

Choose a reason for hiding this comment

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

see #2800, #3830
some people run into issues with our server side scheduling and just want to keep sending their invitations via thunderbird.
or they just don't like that invitations are sent out by the server's email address, not their own one.

Copy link
Member Author

Choose a reason for hiding this comment

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

also #2345

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure we will ever change that value on clusters.

Copy link

Choose a reason for hiding this comment

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

Besides this configuration value, it would be more intuitive to have a checkbox when importing a calendar. The user itself, who has no access to the config.php should be able to decide whether to send invitaions for future events or not while importing a calendar.

Copy link
Member Author

@georgehrke georgehrke Aug 24, 2017

Choose a reason for hiding this comment

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

it would be more intuitive to have a checkbox when importing a calendar

Or maybe never send invitations on import?
Anyway, that's out of scope for this pr.
Let's continue the discussion here: nextcloud/calendar#576

Copy link
Member

Choose a reason for hiding this comment

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

I would also not make it a server wide setting. Just remove the added stuff (compared to the backport) and then we can get this in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I will remove that commit for now.
I would anyway still like to add this feature in the future.

Should I just implement it via the appconfig table?

@kosli
Copy link

kosli commented Jun 27, 2017

any update on merging this pull request?

@georgehrke
Copy link
Member Author

@kosli There is still something missing, see the first post. Won't have time to look into this before the weekend.

@kosli
Copy link

kosli commented Jun 29, 2017

@georgehrke ups, I didn't see the ToDo on top. Thanks!

@georgehrke georgehrke force-pushed the bugfix/2855/dont_send_invitations_for_past_events branch from 677267b to 7e24c8c Compare July 17, 2017 10:40
@georgehrke
Copy link
Member Author

rebased

@georgehrke
Copy link
Member Author

One thing I wasn't sure about:

Should it not send out invitations for any events in the past or only for events that are older than 2 weeks / month ?

@jospoortvliet
Copy link
Member

jospoortvliet commented Jul 18, 2017

I wouldn't sent out any invitations for past events, after all those either have been sent already or the user decided not to sent them...

Or are you afraid some events would end up being missed?

@georgehrke
Copy link
Member Author

Backport to review once this is merged: #5841

@ggeorgg
Copy link

ggeorgg commented Aug 22, 2017

Import of a calendar in a fresh nextcloud 12.0.1 with these changes applied is working. The option to turn off resending invitation mails via the config setting is also working.

@jospoortvliet
Copy link
Member

@LukasReschke happy with the changes/feedback from Georg? It'd be nice to get this in so 12.0.3 will not resent all these invitations 😢

@@ -52,6 +52,7 @@
$calDavBackend = new CalDavBackend($db, $principalBackend, $userManager, $random, $dispatcher, true);

$debugging = \OC::$server->getConfig()->getSystemValue('debug', false);
$sendInvitations = \OC::$server->getConfig()->getSystemValue('caldav_send_invitations', true);
Copy link
Member

Choose a reason for hiding this comment

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

I would also not make it a server wide setting. Just remove the added stuff (compared to the backport) and then we can get this in.

Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@georgehrke georgehrke force-pushed the bugfix/2855/dont_send_invitations_for_past_events branch from 7e24c8c to a1df91d Compare September 5, 2017 11:13
@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed backport-request 3. to review Waiting for reviews labels Sep 5, 2017
@MorrisJobke MorrisJobke merged commit 8e6d86a into master Sep 6, 2017
@MorrisJobke MorrisJobke deleted the bugfix/2855/dont_send_invitations_for_past_events branch September 6, 2017 20:48
@jospoortvliet
Copy link
Member

AWESOME!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invitation send to attendees upon calendar import (also for past events)
7 participants