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

Bump sabre #1792

Merged
merged 8 commits into from
Nov 9, 2016
Merged

Bump sabre #1792

merged 8 commits into from
Nov 9, 2016

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Oct 18, 2016

Uses: owncloud/core#26115

Lets see what CI thinks of this!

@mention-bot
Copy link

@rullzer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @schiessle and @LukasReschke to be potential reviewers.

@phsc84
Copy link

phsc84 commented Oct 19, 2016

#1427

@rullzer rullzer force-pushed the bump_sabre branch 2 times, most recently from a91b241 to 595384b Compare October 19, 2016 08:51
@rullzer rullzer added the 2. developing Work in progress label Oct 19, 2016
@MorrisJobke
Copy link
Member

Lets see what CI thinks of this!

It says 🚫 ❗️

1) OCA\DAV\Tests\unit\CalDAV\CalendarTest::testPrivateClassification with data set #0 (3, false)
Undefined index: {http://owncloud.org/ns}owner-principal
/drone/src/github.com/nextcloud/server/apps/dav/lib/CalDAV/Calendar.php:286
/drone/src/github.com/nextcloud/server/apps/dav/lib/CalDAV/Calendar.php:208
/drone/src/github.com/nextcloud/server/apps/dav/tests/unit/CalDAV/CalendarTest.php:230
2) OCA\DAV\Tests\unit\CalDAV\CalendarTest::testConfidentialClassification with data set #0 (3, false)
Undefined index: {http://owncloud.org/ns}owner-principal
/drone/src/github.com/nextcloud/server/apps/dav/lib/CalDAV/Calendar.php:286
/drone/src/github.com/nextcloud/server/apps/dav/lib/CalDAV/Calendar.php:193
/drone/src/github.com/nextcloud/server/apps/dav/tests/unit/CalDAV/CalendarTest.php:319

@MorrisJobke MorrisJobke removed the 3. to review Waiting for reviews label Oct 20, 2016
@rullzer
Copy link
Member Author

rullzer commented Oct 20, 2016

Yeah I know. Have been messing with it to get the caldav intergration tests happy. But no real luck so far

DeepDiver1975 and others added 3 commits November 4, 2016 13:35
* Update sabre/dav to 3.2.0

* Adjust code to work with sabre/dav 3.2.0 and it's dependencies

* Adding own CalDAV plugin to fix calendar home property

* Test if there is a user logged in when listing files home

* Update sabre version used by integration tests

* Disable unauthenticated DAV access

This is needed to make Sabre 3.2 behave like we did before.
Eventually we should integrate better with the ACL plugin which itself
should implement an auth failure when appropriate.

=====

* Fixed so cherry-pick was succesfull

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
* Made sure delete from self works again (and is tested)

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@codecov-io
Copy link

codecov-io commented Nov 4, 2016

Current coverage is 57.87% (diff: 93.75%)

Merging #1792 into master will increase coverage by 0.08%

@@             master      #1792   diff @@
==========================================
  Files          1094       1099     +5   
  Lines         62793      63539   +746   
  Methods        6994       7147   +153   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          36289      36776   +487   
- Misses        26504      26763   +259   
  Partials          0          0          
Diff Coverage File Path
0% apps/dav/lib/Files/RootCollection.php
•••••• 66% apps/dav/lib/CalDAV/CalDavBackend.php
•••••••••• 100% new apps/dav/lib/CalDAV/Plugin.php
•••••••••• 100% apps/dav/lib/CalDAV/CalendarObject.php
•••••••••• 100% apps/dav/lib/CalDAV/Calendar.php
•••••••••• 100% apps/dav/lib/Connector/Sabre/DavAclPlugin.php
•••••••••• 100% apps/dav/lib/CardDAV/Converter.php
•••••••••• 100% new apps/dav/lib/CalDAV/Schedule/Plugin.php
•••••••••• 100% apps/dav/lib/CardDAV/AddressBookImpl.php
•••••••••• 100% apps/dav/lib/CardDAV/CardDavBackend.php

Review all 11 files changed

Powered by Codecov. Last update f42d5b6...da55dd1

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 4, 2016
@rullzer
Copy link
Member Author

rullzer commented Nov 4, 2016

Whee CI is happy. Review time!

CC: @nickvergessen @MorrisJobke @LukasReschke @icewind1991

@MorrisJobke
Copy link
Member

Works here fine 👍 I did some smoke testing with WebDAV and some file operations.

@MorrisJobke
Copy link
Member

@nickvergessen could we get smashbox for this?

@nickvergessen
Copy link
Member

Smashbox passed ✅

Copy link
Member

@LukasReschke LukasReschke left a comment

Choose a reason for hiding this comment

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

Looks good. Two new files are not really being tested. Will add myself quickly :)

* @param string $principal
* @return array
*/
protected function getAddressesForPrincipal($principal) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing tests. Will quickly add myself.

/**
* @inheritdoc
*/
function getCalendarHomeForPrincipal($principalUrl) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing tests. Will quickly add myself.

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
@LukasReschke
Copy link
Member

LukasReschke commented Nov 9, 2016

Tests added and third-party rebased. LGTM.

@rullzer rullzer merged commit 6a75296 into master Nov 9, 2016
@rullzer rullzer deleted the bump_sabre branch November 9, 2016 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants