Skip to content

Make ContactsStore a public API - #6637

Merged
rullzer merged 3 commits into
masterfrom
contactsstore_public_api
Dec 11, 2017
Merged

Make ContactsStore a public API#6637
rullzer merged 3 commits into
masterfrom
contactsstore_public_api

Conversation

@LEDfan

@LEDfanLEDfan commented Sep 25, 2017

Copy link
Copy Markdown
Member

This makes the ContactsStore a public API which can be used by apps. As I described here: #5585 (comment) this is needed for e.g. the Chat app to know which users are allowed to Chat with other users.

@LEDfanLEDfan added the 3. to review Waiting for reviews label Sep 25, 2017
@mention-bot

mention-bot commented Sep 25, 2017

Copy link
Copy Markdown

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

LEDfan
LEDfan commented Sep 25, 2017
View reviewed changes
Comment threadapps/dav/appinfo/app.php
if (!is_null($user)) {
$app->setupContactsProvider($cm, $user->getUID());
} else {
$app->setupSystemContactsProvider($cm);

@LEDfanLEDfanSep 25, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This change is needed when there is no users, e.g on the CLI using the occ command.

ChristophWurst
ChristophWurst requested changes Sep 25, 2017
View reviewed changes

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good so far, just some minor documentation/code style issues

Comment threadlib/public/Contacts/ContactsMenu/IContactsStore.php

use OCP\IUser;

interface IContactsStore {

@ChristophWurstChristophWurstSep 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

missing @since annotations on both the interface and its methods

Comment threadlib/public/Contacts/ContactsMenu/IContactsStore.php Outdated
*/
public function findOne(IUser $user, $shareType, $shareWith);

} No newline at end of file

@ChristophWurstChristophWurstSep 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

new line missing

@rullzerrullzer added this to the Nextcloud 13 milestone Sep 27, 2017
@codecov

codecovBot commented Oct 1, 2017•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #6637 into master will decrease coverage by <.01%.
The diff coverage is 28.57%.

@@ Coverage Diff @@## master #6637 +/- ##
============================================
- Coverage 51.09% 51.09% -0.01% - Complexity 24865 24867 +2 
============================================
Files 1596 1596 Lines 94608 94622 +14 Branches 1367 1367 ============================================
+ Hits 48340 48344 +4 - Misses 46268 46278 +10
Impacted FilesCoverage ΔComplexity Δ
...ib/private/Contacts/ContactsMenu/ContactsStore.php86% <ø> (ø)41 <0> (ø)⬇️
apps/dav/lib/AppInfo/Application.php18.36% <0%> (-0.79%)15 <1> (+1)
apps/dav/appinfo/app.php26.66% <0%> (-1.91%)0 <0> (ø)
apps/dav/lib/CardDAV/ContactsManager.php85% <100%> (+1.66%)5 <1> (+1)⬆️
lib/private/Server.php81.5% <28.57%> (-0.43%)133 <0> (ø)
apps/files_trashbin/lib/Trashbin.php72.28% <0%> (-0.25%)136% <0%> (ø)
apps/files_trashbin/lib/Expiration.php91.93% <0%> (+1.61%)29% <0%> (ø)⬇️

@LEDfan

LEDfan commented Oct 1, 2017

Copy link
Copy Markdown
MemberAuthor

@ChristophWurst done

@LEDfanLEDfan mentioned this pull request Oct 1, 2017
Merged
14 tasks
@LEDfan

LEDfan commented Dec 3, 2017

Copy link
Copy Markdown
MemberAuthor

This is still tagged for NC 13, does this mean it will be merged before 13? I forgot about the featue-freeze to bump this in time...
Thanks!

@MorrisJobke

MorrisJobke commented Dec 8, 2017

Copy link
Copy Markdown
Member

This is still tagged for NC 13, does this mean it will be merged before 13? I forgot about the featue-freeze to bump this in time...
Thanks!

We plan to do the beta2 soon (at least middle of next week, looking at the other open PRs). I would like to have feedback from @rullzer or @ChristophWurst on this one here.

@MorrisJobkeMorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 8, 2017
@MorrisJobke

MorrisJobke commented Dec 8, 2017

Copy link
Copy Markdown
Member

And please fix the conflicts ;)

@MorrisJobkeMorrisJobke mentioned this pull request Dec 8, 2017
Merged
28 tasks
@LEDfan
LEDfanforce-pushed the contactsstore_public_api branch from 8dbc639 to 3b2e916CompareDecember 9, 2017 08:45
@LEDfan

LEDfan commented Dec 9, 2017

Copy link
Copy Markdown
MemberAuthor

@MorrisJobke I fixed the conflicts.

@LEDfanLEDfan added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 9, 2017
@MorrisJobke

MorrisJobke commented Dec 10, 2017

Copy link
Copy Markdown
Member

@LEDfan Because you added a new PHP class could you dump the autoloader again:

The autoloaders are not up to date
Please run: bash build/autoloaderchecker.sh
And commit the result

ChristophWurst
ChristophWurst requested changes Dec 10, 2017
View reviewed changes
Comment threadlib/private/Server.php Outdated
);
});

$this->registerService(\OCP\Contacts\ContactsMenu\IContactsStore::class, function(Server $c) {

@ChristophWurstChristophWurstDec 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Register it as alias with $this->registerAlias(\OCP\Contacts\ContactsMenu\IContactsStore::class, \OC\Contacts\ContactsMenu\ContactsStore); and the DI container will resolve the constructor args automatically. This also prevents the contacts store being instantiated twice.

@LEDfan

LEDfan commented Dec 10, 2017

Copy link
Copy Markdown
MemberAuthor

@MorrisJobke@ChristophWurst thanks, I updated the PR 😄

ChristophWurst
ChristophWurst approved these changes Dec 10, 2017
View reviewed changes

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Changes look good

@blizzz

blizzz commented Dec 11, 2017

Copy link
Copy Markdown
Member

more conflicts @LEDfan

@MorrisJobke
MorrisJobkeforce-pushed the contactsstore_public_api branch from 9bb7f5b to adf7d11CompareDecember 11, 2017 16:19
@MorrisJobke

MorrisJobke commented Dec 11, 2017

Copy link
Copy Markdown
Member

Fixed the conflicts in the server container. Nothing crucial.

@MorrisJobkeMorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 11, 2017
@MorrisJobke

MorrisJobke commented Dec 11, 2017

Copy link
Copy Markdown
Member

Ah ... and conflicts again in the same file ... let me rebase a last time 🙈

@LEDfan@MorrisJobke
Make ContactsStore a public API
f6ef779
Signed-off-by: Tobia De Koninck <tobia@ledfan.be>
@LEDfan@MorrisJobke
Add@SInCEannotation and add missing new line
78049af
Signed-off-by: Tobia De Koninck <tobia@ledfan.be>
@LEDfan@MorrisJobke
Register ContactsStore using alias + run autoloaderchecker.sh
cecfc28
Signed-off-by: Tobia De Koninck <tobia@ledfan.be>
@MorrisJobke
MorrisJobkeforce-pushed the contactsstore_public_api branch from adf7d11 to cecfc28CompareDecember 11, 2017 17:19
@rullzer
rullzer merged commit 179be8d into masterDec 11, 2017
@rullzer
rullzer deleted the contactsstore_public_api branch December 11, 2017 19:04
@ChristophWurstChristophWurst added the pending documentation This pull request needs an associated documentation update label Oct 17, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChristophWurstChristophWurstChristophWurst approved these changes
@blizzzblizzzblizzz approved these changes
@rullzerrullzerAwaiting requested review from rullzer

Assignees

No one assigned

Labels

4. to releaseReady to be released and/or waiting for tests to finishpending documentationThis pull request needs an associated documentation update

Projects

None yet

Milestone

Nextcloud 13

Development

Successfully merging this pull request may close these issues.

7 participants

@LEDfan@mention-bot@MorrisJobke@blizzz@ChristophWurst@rullzer@tobiasKaminsky