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

allow for regular background updates of avatars from social networks #1722

Merged
merged 1 commit into from
Aug 11, 2020

Conversation

call-me-matt
Copy link
Member

implements #1594

@call-me-matt call-me-matt self-assigned this Jul 31, 2020
@call-me-matt call-me-matt force-pushed the enh/social-updater branch 2 times, most recently from 8cf1b21 to 64187bd Compare July 31, 2020 13:23
@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #1722 into master will decrease coverage by 4.58%.
The diff coverage is 24.07%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1722      +/-   ##
============================================
- Coverage     35.35%   30.76%   -4.59%     
- Complexity      101      128      +27     
============================================
  Files            15       17       +2     
  Lines           280      377      +97     
============================================
+ Hits             99      116      +17     
- Misses          181      261      +80     
Impacted Files Coverage Δ Complexity Δ
appinfo/routes.php 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
lib/Controller/SocialApiController.php 0.00% <0.00%> (ø) 9.00 <4.00> (+4.00)
lib/Cron/SocialUpdate.php 0.00% <0.00%> (ø) 2.00 <2.00> (?)
lib/Cron/SocialUpdateRegistration.php 0.00% <0.00%> (ø) 4.00 <4.00> (?)
lib/Settings/AdminSettings.php 0.00% <ø> (ø) 5.00 <0.00> (ø)
lib/Service/SocialApiService.php 51.57% <13.63%> (-33.33%) 36.00 <16.00> (+16.00) ⬇️
lib/Controller/PageController.php 100.00% <100.00%> (ø) 3.00 <0.00> (+1.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b78f4d6...973c288. Read the comment docs.

README.md Outdated Show resolved Hide resolved
*/
protected function registerAddressbooks($user, IManager $manager) {
$cm = new ContactsManager($this->davBackend, $this->l10n);
$cm->setupContactsProvider($manager, $user, $this->urlGen);
Copy link
Member Author

@call-me-matt call-me-matt Jul 31, 2020

Choose a reason for hiding this comment

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

Is there a way to limit this to active address books owned by a user? i. e. return address books which are

  • not system address book
  • not shared address book from other user
  • not deactivated

Maybe we can enhance the IManager or IAddressBook?

Copy link
Member

Choose a reason for hiding this comment

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

cc @georgehrke what do you think? Does it make sense to add filters to the IAddressBook interface?


// get contacts in that addressbook
$contacts = $addressBook->search('', ['UID'], ['types' => true]);
// TODO: filter for contacts with social profiles
Copy link
Member Author

Choose a reason for hiding this comment

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

Could be optimized with $addressBook->search('', ['X-SOCIALPROFILE'], ['types' => true]);, if X-SOCIALPROFILE was indexed

Copy link
Member

Choose a reason for hiding this comment

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

Could be optimized with $addressBook->search('', ['X-SOCIALPROFILE'], ['types' => true]);, if X-SOCIALPROFILE was indexed

Mind to add this? :)

Copy link
Member

Choose a reason for hiding this comment

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

if it's not indexed (for older version) does it still works? Is the searched value just ignored?
If so it's ok I guess :)

Copy link
Member Author

@call-me-matt call-me-matt Aug 2, 2020

Choose a reason for hiding this comment

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

Right, I have included this and added a fallback for older server versions.
But concerning the indexer, I could not find out how to add the X-SOCIALPROFILE. Simply adding it to the linked array did not have any effect. Edit: worked after running cron to update the index.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pull request created: nextcloud/server#22085

Copy link
Member Author

@call-me-matt call-me-matt Aug 3, 2020

Choose a reason for hiding this comment

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

Turns out it was not cron, but me refreshing the profiles and triggering a re-index by that - it only works for new or modified contacts! We'll need a background job to re-index all contacts when a user activates this function. @skjnldsv or is there a better way / place to do that? We could, of course, also just ignore this and activate the optimization in a later release - hoping that contacts have been refreshed between the extension of the indexed fields and the moment we start to filter for that.

@call-me-matt call-me-matt force-pushed the enh/social-updater branch 2 times, most recently from df55f34 to 73691e4 Compare July 31, 2020 23:07
@skjnldsv skjnldsv added 2. developing Work in progress enhancement New feature or request labels Aug 1, 2020
@call-me-matt call-me-matt linked an issue Aug 2, 2020 that may be closed by this pull request
call-me-matt added a commit to nextcloud/server that referenced this pull request Aug 2, 2020
required in order to optimize regular background updates of contact avatars from social networks (see nextcloud/contacts#1722 (comment))
@call-me-matt call-me-matt force-pushed the enh/social-updater branch 5 times, most recently from f75d37b to 4f3a2a3 Compare August 4, 2020 21:46
@call-me-matt call-me-matt force-pushed the enh/social-updater branch 3 times, most recently from 4c32f71 to e0bf1fc Compare August 4, 2020 22:19
Signed-off-by: call-me-matt <nextcloud@matthiasheinisch.de>
@call-me-matt call-me-matt marked this pull request as ready for review August 4, 2020 22:39
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 10, 2020
@call-me-matt call-me-matt merged commit 752bb14 into nextcloud:master Aug 11, 2020
@MorrisJobke
Copy link
Member

Bildschirmfoto 2020-09-15 um 12 36 26

😢

@call-me-matt
Copy link
Member Author

Ups, I'm sorry. That was an accident.

@skjnldsv skjnldsv added this to the 3.4.0 milestone Oct 1, 2020
tcitworld added a commit to nextcloud/server that referenced this pull request Mar 21, 2023
…ddressBook

This allows to just UPDATE the card row instead of deleting it and reinsert it. It's very similar to #30120 for calendars.

As we need the addressbookid exposed, this introduces OCA\DAV\CardDAV\Card that extends Sabre's.

I chose specifically NOT to auto-inject LoggerInterface in Addressbook like in #30120 because the chain of DI is huge just for ONE simple call and it would break an existing dirty call (OCA\Contacts calling OCA\DAV) of ContactsManager in Contacts: nextcloud/contacts#1722 (in SocialApiService), but this is debatable.

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
tcitworld added a commit to nextcloud/server that referenced this pull request Mar 21, 2023
…ddressBook

This allows to just UPDATE the card row instead of deleting it and reinsert it. It's very similar to #30120 for calendars.

As we need the addressbookid exposed, this introduces OCA\DAV\CardDAV\Card that extends Sabre's.

I chose specifically NOT to auto-inject LoggerInterface in Addressbook like in #30120 because the chain of DI is huge just for ONE simple call and it would break an existing dirty call (OCA\Contacts calling OCA\DAV) of ContactsManager in Contacts: nextcloud/contacts#1722 (in SocialApiService), but this is debatable.

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
tcitworld added a commit to nextcloud/server that referenced this pull request Mar 21, 2023
…ddressBook

This allows to just UPDATE the card row instead of deleting it and reinsert it. It's very similar to #30120 for calendars.

As we need the addressbookid exposed, this introduces OCA\DAV\CardDAV\Card that extends Sabre's.

I chose specifically NOT to auto-inject LoggerInterface in Addressbook like in #30120 because the chain of DI is huge just for ONE simple call and it would break an existing dirty call (OCA\Contacts calling OCA\DAV) of ContactsManager in Contacts: nextcloud/contacts#1722 (in SocialApiService), but this is debatable.

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
nextcloud-command pushed a commit to nextcloud/server that referenced this pull request Apr 24, 2023
…ddressBook

This allows to just UPDATE the card row instead of deleting it and reinsert it. It's very similar to #30120 for calendars.

As we need the addressbookid exposed, this introduces OCA\DAV\CardDAV\Card that extends Sabre's.

I chose specifically NOT to auto-inject LoggerInterface in Addressbook like in #30120 because the chain of DI is huge just for ONE simple call and it would break an existing dirty call (OCA\Contacts calling OCA\DAV) of ContactsManager in Contacts: nextcloud/contacts#1722 (in SocialApiService), but this is debatable.

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
tcitworld added a commit to nextcloud/server that referenced this pull request Apr 26, 2023
…ddressBook

This allows to just UPDATE the card row instead of deleting it and reinsert it. It's very similar to #30120 for calendars.

As we need the addressbookid exposed, this introduces OCA\DAV\CardDAV\Card that extends Sabre's.

I chose specifically NOT to auto-inject LoggerInterface in Addressbook like in #30120 because the chain of DI is huge just for ONE simple call and it would break an existing dirty call (OCA\Contacts calling OCA\DAV) of ContactsManager in Contacts: nextcloud/contacts#1722 (in SocialApiService), but this is debatable.

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
tcitworld added a commit to nextcloud/server that referenced this pull request Apr 26, 2023
…ddressBook

This allows to just UPDATE the card row instead of deleting it and reinsert it. It's very similar to #30120 for calendars.

As we need the addressbookid exposed, this introduces OCA\DAV\CardDAV\Card that extends Sabre's.

I chose specifically NOT to auto-inject LoggerInterface in Addressbook like in #30120 because the chain of DI is huge just for ONE simple call and it would break an existing dirty call (OCA\Contacts calling OCA\DAV) of ContactsManager in Contacts: nextcloud/contacts#1722 (in SocialApiService), but this is debatable.

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
tcitworld added a commit to nextcloud/server that referenced this pull request Apr 27, 2023
…ddressBook

This allows to just UPDATE the card row instead of deleting it and reinsert it. It's very similar to #30120 for calendars.

As we need the addressbookid exposed, this introduces OCA\DAV\CardDAV\Card that extends Sabre's.

I chose specifically NOT to auto-inject LoggerInterface in Addressbook like in #30120 because the chain of DI is huge just for ONE simple call and it would break an existing dirty call (OCA\Contacts calling OCA\DAV) of ContactsManager in Contacts: nextcloud/contacts#1722 (in SocialApiService), but this is debatable.

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
tcitworld added a commit to nextcloud/server that referenced this pull request May 16, 2023
…ddressBook

This allows to just UPDATE the card row instead of deleting it and reinsert it. It's very similar to #30120 for calendars.

As we need the addressbookid exposed, this introduces OCA\DAV\CardDAV\Card that extends Sabre's.

I chose specifically NOT to auto-inject LoggerInterface in Addressbook like in #30120 because the chain of DI is huge just for ONE simple call and it would break an existing dirty call (OCA\Contacts calling OCA\DAV) of ContactsManager in Contacts: nextcloud/contacts#1722 (in SocialApiService), but this is debatable.

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Design improvements on "Update avatar from social media" feature
3 participants