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

Update social avatars in chunks #1745

Merged
merged 5 commits into from
Sep 11, 2020

Conversation

call-me-matt
Copy link
Member

When there are a lot of contacts, the background update of avatars from social networks shall be split into chunks

@codecov
Copy link

codecov bot commented Aug 14, 2020

Codecov Report

Merging #1745 into master will decrease coverage by 3.17%.
The diff coverage is 35.61%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1745      +/-   ##
============================================
- Coverage     40.05%   36.87%   -3.18%     
- Complexity      128      162      +34     
============================================
  Files            17       19       +2     
  Lines           377      499     +122     
============================================
+ Hits            151      184      +33     
- Misses          226      315      +89     
Impacted Files Coverage Δ Complexity Δ
appinfo/app.php 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
lib/AppInfo/Application.php 0.00% <0.00%> (ø) 3.00 <3.00> (+2.00)
lib/Cron/SocialUpdate.php 0.00% <0.00%> (ø) 5.00 <1.00> (+3.00)
lib/Cron/SocialUpdateRegistration.php 0.00% <0.00%> (ø) 4.00 <0.00> (ø)
lib/Dav/PatchPlugin.php 0.00% <0.00%> (ø) 16.00 <16.00> (?)
lib/Listener/LoadContactsFilesActions.php 0.00% <0.00%> (ø) 2.00 <2.00> (?)
lib/Service/SocialApiService.php 87.21% <85.71%> (-1.21%) 47.00 <25.00> (+11.00) ⬇️
lib/Controller/PageController.php 100.00% <100.00%> (ø) 3.00 <0.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 752bb14...c03bd90. Read the comment docs.

@call-me-matt call-me-matt force-pushed the enh/social-chunks branch 3 times, most recently from 13bf720 to 169ae41 Compare August 14, 2020 10:22
Signed-off-by: call-me-matt <nextcloud@matthiasheinisch.de>
@call-me-matt call-me-matt marked this pull request as ready for review August 14, 2020 10:31
@skjnldsv skjnldsv added 2. developing Work in progress enhancement New feature or request labels Aug 21, 2020
@skjnldsv
Copy link
Member

What is the status here? Please update labels :)

@call-me-matt call-me-matt added the 3. to review Waiting for reviews label Aug 21, 2020
Signed-off-by: call-me-matt <nextcloud@matthiasheinisch.de>
@skjnldsv skjnldsv removed the 2. developing Work in progress label Aug 25, 2020
Signed-off-by: call-me-matt <nextcloud@matthiasheinisch.de>
@call-me-matt
Copy link
Member Author

I removed the optimization, so we can merge this.

@skjnldsv
Copy link
Member

skjnldsv commented Sep 5, 2020

@rullzer @ChristophWurst @nickvergessen can I have a backgroundjob expert to do a quick review? 🙏

lib/Service/SocialApiService.php Outdated Show resolved Hide resolved
lib/Cron/SocialUpdate.php Outdated Show resolved Hide resolved
}

// delay to prevent rate limiting issues
sleep($delay);
Copy link
Member

Choose a reason for hiding this comment

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

This feels weird. So you sleep here but execute at most 15seconds?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, this delay is to be compliant to the terms of service of social networks, so I don't want to remove it. And the 15seconds of maximum execution time are to prevent that the update runs forever on huge installations.
How would you propose to tackle this?

Signed-off-by: call-me-matt <nextcloud@matthiasheinisch.de>
$stoppedAtContact = $report[0]['stoppedAt']['contact'];

// make sure the offset contact/address book are still existing
if ($this->social->existsAddressBook($stoppedAtBook, $userId) == false) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ($this->social->existsAddressBook($stoppedAtBook, $userId) == false) {
if ($this->social->existsAddressBook($stoppedAtBook, $userId)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for your review and good comments!
I suppose this should now start with if(!$this ? Or was the negation done on purpose?
(same for the next suggested change)

if ($this->social->existsAddressBook($stoppedAtBook, $userId) == false) {
$stoppedAtBook = null;
}
if ($this->social->existsContact($stoppedAtContact, $stoppedAtBook, $userId) == false) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ($this->social->existsContact($stoppedAtContact, $stoppedAtBook, $userId) == false) {
if ($this->social->existsContact($stoppedAtContact, $stoppedAtBook, $userId)) {

Copy link
Member

@nickvergessen nickvergessen 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, but I didn't execute it

add elegance from nickvergessen

Co-authored-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com>
Signed-off-by: call-me-matt <nextcloud@matthiasheinisch.de>
@nickvergessen
Copy link
Member

Ready to go I guess?

@skjnldsv skjnldsv merged commit 96f05d6 into nextcloud:master Sep 11, 2020
@call-me-matt call-me-matt deleted the enh/social-chunks branch September 11, 2020 11:15
@skjnldsv skjnldsv added this to the 3.4.0 milestone Oct 1, 2020
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.

None yet

3 participants