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.

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

add Nextcloud Search extension to CalDAV #4098

Merged
merged 8 commits into from
Apr 29, 2017
Merged

add Nextcloud Search extension to CalDAV #4098

merged 8 commits into from
Apr 29, 2017

Conversation

georgehrke
Copy link
Member

@georgehrke georgehrke commented Mar 27, 2017 •
edited
Loading

Implements custom Search plugin for CalDAV

@georgehrke georgehrke added the 2. developing Work in progress label Mar 27, 2017
@georgehrke georgehrke added this to the Nextcloud 12.0 milestone Mar 27, 2017
@georgehrke georgehrke self-assigned this Mar 27, 2017
@mention-bot
Copy link

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

@codecov-io
Copy link

codecov-io commented Mar 27, 2017 •
edited by codecov bot
Loading

Codecov Report

Merging #4098 into master will increase coverage by 12.31%.
The diff coverage is 68.04%.

@@              Coverage Diff              @@
##             master    #4098       +/-   ##
=============================================
+ Coverage     41.97%   54.28%   +12.31%     
- Complexity    19118    21930     +2812     
=============================================
  Files          1246     1353      +107     
  Lines         71177    84008    +12831     
  Branches       1335     1327        -8     
=============================================
+ Hits          29875    45607    +15732     
+ Misses        41302    38401     -2901
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/Server.php 45.03% <0%> (-47.15%) 12 <0> (ø)
...igration/BuildCalendarSearchIndexBackgroundJob.php 0% <0%> (ø) 7 <7> (?)
apps/dav/lib/CalDAV/CalendarHome.php 0% <0%> (ø) 21 <1> (+1) ⬆️
...pps/dav/lib/Migration/BuildCalendarSearchIndex.php 18.18% <18.18%> (ø) 4 <4> (?)
...s/dav/lib/CalDAV/Search/Xml/Filter/ParamFilter.php 69.23% <69.23%> (ø) 3 <3> (?)
...CalDAV/Search/Xml/Request/CalendarSearchReport.php 78.33% <78.33%> (ø) 26 <26> (?)
.../lib/CalDAV/Search/Xml/Filter/SearchTermFilter.php 80% <80%> (ø) 2 <2> (?)
...s/dav/lib/CalDAV/Search/Xml/Filter/LimitFilter.php 80% <80%> (ø) 3 <3> (?)
.../dav/lib/CalDAV/Search/Xml/Filter/OffsetFilter.php 80% <80%> (ø) 3 <3> (?)
apps/dav/lib/CalDAV/Search/SearchPlugin.php 82.85% <82.85%> (ø) 8 <8> (?)
... and 794 more

@MorrisJobke
Copy link
Member

🚧 🚧 WIP 🚧 🚧

How likely is this to be ready within the next two weeks?

@georgehrke
Copy link
Member Author

How likely is this to be ready within the next two weeks?

Very likely. Will dedicate my weekend to this :)

@georgehrke georgehrke force-pushed the feature/caldav_search branch 3 times, most recently from 136d0cd to 6032a04 Compare April 23, 2017 23:16
@georgehrke
Copy link
Member Author

georgehrke commented Apr 24, 2017 •
edited
Loading

  • unit tests
  • add repair step to fill up calendarobjects_properties
  • integration tests would be cool (need help here though)

@georgehrke
add Nextcloud Search extension to CalDAV
57b543a 
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@georgehrke
remove unused CalendarSearchValidator
e760cda 
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
georgehrke
georgehrke commented Apr 24, 2017
View reviewed changes
apps/dav/appinfo/database.xml Outdated
@@ -671,6 +671,78 @@ CREATE TABLE calendarobjects (
</table>

<table>
<name>*dbprefix*calendarobjects_properties</name>
Copy link
Member Author

Choose a reason for hiding this comment

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

@nickvergessen What's the name's length limit for oracle?

Copy link
Member

Choose a reason for hiding this comment

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

31 characters AFAIK

Copy link
Member

Choose a reason for hiding this comment

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

30,

however when there is a primary key+autoincrement on the table its 30 -6 (including the oc_)

So it is too long here, because oc_calendarobjects_properties_AI_PK is 35 instead of 30.

@georgehrke
add repairstep with backgroundjob to index calendar data
40eec1e 
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@georgehrke georgehrke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 25, 2017
@georgehrke
Copy link
Member Author

This is ready to review and test. Just finishing the tests

@georgehrke
unit test CalDAV Search Plugin
dd424fc 
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@georgehrke
updateProperties: catch exception when reading calendar data
ac3cc52 
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@georgehrke
require at least one param or prop filter element
c76633b 
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@MorrisJobke MorrisJobke requested review from nickvergessen, rullzer and schiessle and removed request for nickvergessen and rullzer April 26, 2017 04:16
nickvergessen
nickvergessen previously requested changes Apr 26, 2017
View reviewed changes
apps/dav/appinfo/database.xml Outdated
@@ -671,6 +671,78 @@ CREATE TABLE calendarobjects (
</table>

<table>
<name>*dbprefix*calendarobjects_properties</name>
Copy link
Member

Choose a reason for hiding this comment

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

30,

however when there is a primary key+autoincrement on the table its 30 -6 (including the oc_)

So it is too long here, because oc_calendarobjects_properties_AI_PK is 35 instead of 30.

@georgehrke
Copy link
Member Author

I will change it to calendarobjects_props.
oc_calendarobjects_props_AI_PK is exactly 30

@georgehrke
Copy link
Member Author

@MorrisJobke @nickvergessen Can we still get this in?

MariusBluem
MariusBluem reviewed Apr 28, 2017
View reviewed changes
apps/dav/appinfo/info.xml
@@ -5,7 +5,7 @@
<description>WebDAV endpoint</description>
<licence>AGPL</licence>
<author>owncloud.org</author>
Copy link
Member

Choose a reason for hiding this comment

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

Can we change this part here? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm. We could. but like the files app the dav app is always enabled and not visible in the apps management anyway

Copy link
Member

Choose a reason for hiding this comment

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

Reminds me of an idea 💡#4580

rullzer
rullzer approved these changes Apr 28, 2017
View reviewed changes
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.

Looks good. LGTM

@georgehrke can you create an issue for the intergration tests.

@georgehrke
unit test custom calendar search
8d00458 
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@georgehrke
rename calendarobjects_properties -> calendarobjects_props
0f8a951 
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@LukasReschke LukasReschke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Apr 28, 2017
@MorrisJobke
Copy link
Member

Tests run locally fine 👍

@MorrisJobke MorrisJobke merged commit 2a77331 into master Apr 29, 2017
@MorrisJobke MorrisJobke deleted the feature/caldav_search branch April 29, 2017 02:38
@MariusBluem MariusBluem mentioned this pull request May 25, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MorrisJobke MorrisJobke MorrisJobke left review comments

@MariusBluem MariusBluem MariusBluem left review comments

@rullzer rullzer rullzer approved these changes

@LukasReschke LukasReschke LukasReschke approved these changes

@nickvergessen nickvergessen nickvergessen left review comments

@schiessle schiessle Awaiting requested review from schiessle

@icewind1991 icewind1991 Awaiting requested review from icewind1991

Assignees

@georgehrke georgehrke

Labels
4. to release Ready to be released and/or waiting for tests to finish
Projects
None yet
Milestone
Nextcloud 12.0
Development

Successfully merging this pull request may close these issues.
8 participants
@georgehrke @mention-bot @codecov-io @MorrisJobke @rullzer @nickvergessen @LukasReschke @MariusBluem