Skip to content

Do not load calendar/addressbook plugins if not needed #9268

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.

Sign up for GitHub

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

Jump to bottom
Merged
MorrisJobke merged 6 commits into from
May 28, 2018
Merged

Do not load calendar/addressbook plugins if not needed #9268

MorrisJobke merged 6 commits into from
May 28, 2018

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented Apr 23, 2018

Fixes #6711
Based on owncloud/core@f0fb21c

Signed-off-by: Roeland Jago Douma roeland@famdouma.nl

@rullzer rullzer added this to the Nextcloud 14 milestone Apr 23, 2018
@rullzer rullzer mentioned this pull request Apr 23, 2018
Closed
@MorrisJobke
Copy link
Member

MorrisJobke commented Apr 23, 2018 •
edited
Loading

Does this also fix stuff like #8141? And #8827?

@georgehrke
Copy link
Member

georgehrke commented Apr 23, 2018

@MorrisJobke It should be as far as i can tell.

@georgehrke
Copy link
Member

georgehrke commented Apr 23, 2018

I will check when testing this pr.

@rullzer
Copy link
Member Author

rullzer commented Apr 23, 2018

Yes it should

MorrisJobke
MorrisJobke approved these changes Apr 23, 2018
View reviewed changes
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, works and code makes sense 👍

@MorrisJobke
Copy link
Member

MorrisJobke commented Apr 23, 2018

CI fails :/

@codecov
Copy link

codecov bot commented Apr 23, 2018 •
edited
Loading

Codecov Report

Merging #9268 into master will decrease coverage by 22.57%.
The diff coverage is 3.07%.

@@              Coverage Diff              @@
##             master    #9268       +/-   ##
=============================================
- Coverage     51.68%   29.11%   -22.58%     
- Complexity    25691    25707       +16     
=============================================
  Files          1634     1569       -65     
  Lines         95914    88082     -7832     
  Branches       1384        0     -1384     
=============================================
- Hits          49571    25642    -23929     
- Misses        46343    62440    +16097
Impacted Files Coverage Δ Complexity Δ
apps/dav/composer/composer/autoload_static.php 0% <ø> (ø) 1 <0> (ø) ⬇️
apps/dav/lib/Command/RemoveInvalidShares.php 0% <0%> (ø) 6 <6> (?)
apps/dav/lib/Connector/Sabre/Principal.php 84.34% <0%> (-5.66%) 49 <0> (+2)
apps/dav/composer/composer/autoload_classmap.php 0% <0%> (ø) 0 <0> (ø) ⬇️
apps/dav/lib/DAV/Sharing/Backend.php 0% <0%> (-86.82%) 26 <5> (+2)
lib/private/Repair.php 30.88% <0%> (ø) 19 <0> (ø) ⬇️
apps/dav/lib/Server.php 0% <0%> (-44.67%) 22 <3> (+5)
apps/dav/lib/CardDAV/AddressBook.php 57.5% <100%> (ø) 28 <0> (ø) ⬇️
apps/dav/lib/CalDAV/Calendar.php 67.14% <100%> (-3.58%) 50 <0> (ø)
apps/user_ldap/lib/Migration/UUIDFixUser.php 0% <0%> (-100%) 1% <0%> (ø)
... and 452 more

@MorrisJobke
Copy link
Member

MorrisJobke commented Apr 23, 2018

CI failure is due to a timeout :)

@rullzer
Copy link
Member Author

rullzer commented Apr 25, 2018

One more review please :)

@MorrisJobke MorrisJobke requested a review from blizzz April 26, 2018 07:45
@rullzer
Copy link
Member Author

rullzer commented May 1, 2018

Conflicts fixed and rebased.

@MorrisJobke MorrisJobke requested a review from schiessle May 2, 2018 07:34
@MorrisJobke
Copy link
Member

MorrisJobke commented May 7, 2018

@georgehrke @blizzz @nickvergessen @ChristophWurst @skjnldsv Reviews would be nice :)

blizzz
blizzz requested changes May 7, 2018
View reviewed changes
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

the repair step might lose valid shares when

  1. Nextcloud is in maintenance mode
  2. AND external user backends are used

since apps are not loaded in this context.

@rullzer
Copy link
Member Author

rullzer commented May 8, 2018

@blizzz I can drop the repair job... but then we might have invalid shares....

@blizzz
Copy link
Member

blizzz commented May 9, 2018

@blizzz I can drop the repair job... but then we might have invalid shares....

Or add a switch that prevents running in maintenance mode.

@rullzer
Copy link
Member Author

rullzer commented May 24, 2018

@blizzz now they are only run if you run the expensive repair steps in case you run into it.

All our other repair steps are executed in maintenance mode so then it indeed would do 💥 in certain situations

@blizzz
Copy link
Member

blizzz commented May 25, 2018

All our other repair steps are executed in maintenance mode so then it indeed would do 💥 in certain situations

Then it is better run as a seperate occ command, bound to apps, as i am afraid it would do more harm than good.

@rullzer
Copy link
Member Author

rullzer commented May 25, 2018

Nah during upgrade.

Now it is. occ maintenance:repair - - expensive

rullzer added 5 commits May 27, 2018 20:51
@rullzer
Do not load calendar/addressbook plugins if not needed
c5d5c7d 
Fixes #6711
Based on owncloud/core@f0fb21c

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
When sharing calendars and addressbooks the principal has to be verif…
240006b 
…ied to be valid

owncloud/core@d3fb8fc

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
CalDAV and CardDAV plugins need to be registered for the principals c…
69e650d 
…ollection as well

owncloud/core@9f2e643

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
Adding repair step which cleans shares of invalid principals
8bf8e92 
owncloud/core@edacf22

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
Fix tests
f9a64d3 
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
Copy link
Member Author

rullzer commented May 27, 2018

@blizzz ok one more time ;)

@blizzz
Copy link
Member

blizzz commented May 28, 2018

looks good now, apart from tests

@rullzer
Move repairstep to a custom command
26c1e33 
People that have issues can run it manually

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@MorrisJobke MorrisJobke merged commit 0947ddb into master May 28, 2018
@MorrisJobke MorrisJobke deleted the bugfix/6711 branch May 28, 2018 11:53
@rullzer rullzer mentioned this pull request May 29, 2018
Merged
MorrisJobke
MorrisJobke reviewed May 31, 2018
View reviewed changes
lib/private/Repair.php
use OC\Template\JSCombiner;
use OC\Template\SCSSCacher;
use OCA\DAV\Connector\Sabre\Principal;
use OCA\DAV\Repair\RemoveInvalidShares;
Copy link
Member

@MorrisJobke MorrisJobke May 31, 2018

Choose a reason for hiding this comment

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

bildschirmfoto 2018-05-31 um 17 22 21

🙈

@MorrisJobke MorrisJobke mentioned this pull request May 31, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@blizzz blizzz blizzz approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@georgehrke georgehrke Awaiting requested review from georgehrke

@icewind1991 icewind1991 Awaiting requested review from icewind1991

@schiessle schiessle Awaiting requested review from schiessle

Assignees

No one assigned

Labels

3. to review Waiting for reviews bug enhancement

Projects

None yet

Milestone

Nextcloud 14

Development

Successfully merging this pull request may close these issues.

5 participants

@rullzer @MorrisJobke @georgehrke @blizzz