Fetch avatars from different sources by jakobsack · Pull Request #469 · nextcloud/mail

jakobsack · 2017-08-25T16:13:50Z

This implements parts of #463 and #236
When you open mail/avatars?email=test@example.com you get a JSON response
{ url: $url, source: $source }.
If no image has been found $url is null.

ChristophWurst · 2017-08-28T08:38:45Z

 			return message.get('dateInt') * -1;
+		},
+		initialize: function() {
+			this.on('add', function(message) {


in regards to consistent coding style and testability I'd suggest to move this anonymous callback to a method of this class and just have the wiring in the constructor ;)

ChristophWurst · 2017-08-28T08:39:27Z

+					if (avatar.source != 'none'){
+						message.set('senderImage', avatar.url);
+					}
+				});


please add a .catch(console.error.bind(this)) at the end so that errors are logged to the console

ChristophWurst · 2017-08-28T08:39:41Z

+				});
+
+				Promise.resolve($.ajax(url)).then(function(avatar) {
+					if (avatar.source != 'none'){


use !== instead

ChristophWurst · 2017-08-28T08:41:06Z

+		},
+		initialize: function() {
+			this.on('add', function(message) {
+				var url = OC.generateUrl('apps/mail/avatars?email={email}', {


would be great to have this logic in a service of its own because then we can test this more easily ;)

ChristophWurst · 2017-08-28T08:41:46Z

+
+namespace OCA\Mail\Controller;
+
+use Exception;


ChristophWurst · 2017-08-28T08:42:37Z

+	 */
+	public function show($email) {
+		// close the session to allow parallel downloads
+		$this->session->close();


I don't think this is necessary if you don't annotate the controller with @UseSession. cc @LukasReschke

ChristophWurst · 2017-08-28T08:43:27Z

+	}
+}
+
+__halt_compiler();


ChristophWurst · 2017-08-28T08:45:20Z

+use Gravatar\Gravatar;
+use Mpclarkson\IconScraper\Scraper;
+
+class AvatarService {


I'd really like to have this class tested. In its current form it's a bit difficult to do so. May I suggest to refactor this class into three classes for the individual avatar sources and just combine their results in this class? I can also take care of this, just let me know :)

ChristophWurst · 2017-08-28T08:46:00Z

+			$response = $client->get($gravatarUrl);
+		}
+		catch (\Exception $exception) {
+			$foundGravatar = false;


just return null; here and remove the $foundAvatar stuff ;)

jakobsack · 2017-08-28T12:57:51Z

The avatars are now cached in data/$user/mail/avatars. The images are loaded right from the beginning if possible.

ChristophWurst · 2017-09-05T11:59:20Z

@jakobsack what's your current status? The title and the tags are a bit contradictory :-P

Btw, there seem to be conflicts with the composer deps.

jakobsack · 2017-09-05T12:42:42Z

Sorry, I bricked my laptop last Friday. I'll try to add the last adjustments during this week.

ChristophWurst · 2017-09-05T12:51:19Z

Sorry, I bricked my laptop last Friday. I'll try to add the last adjustments during this week.

Take your time :)

jancborchardt · 2017-10-11T23:52:29Z

@jakobsack any update here or any way we can help? :)

jakobsack · 2017-10-19T18:57:27Z

@ChristophWurst @Gomez Can you test the latest changes please? And tell me why composer won't work on travis? On my machine everything works fine :-/

jakobsack · 2017-10-22T20:55:19Z

I don't know why the MySQL version fails but it seems not to be related to my changes.

ChristophWurst · 2017-11-13T13:41:56Z

Still, some ideas for that are: have a avatar service on the client-side that aggregates avatar loading and de-duplicates the requests. This is especially useful where you have many messages from the same sender and thus the current logic would send multiple requests.

Plus, we should check whether senderImage is already set and do nothing in that case.

done

ChristophWurst · 2017-11-13T13:42:49Z

FYI: setting the yolo-no-review-but-merging-anyway-timeout to 2 weeks 😉

ChristophWurst · 2017-11-13T14:41:53Z

Still, some ideas for that are: have a avatar service on the client-side that aggregates avatar loading and de-duplicates the requests. This is especially useful where you have many messages from the same sender and thus the current logic would send multiple requests.

And we could add a background job that pre-poluates the cache 😉

jancborchardt · 2017-11-14T10:43:04Z

@ChristophWurst will start reviewing now, meanwhile Fyi there are conflicting files. (And the package-lock.json should be in .gitignore maybe?)

ChristophWurst · 2017-11-14T11:13:26Z

meanwhile Fyi there are conflicting files.

Thanks. Will take care of it.

And the package-lock.json should be in .gitignore maybe

Nope, that makes sure everyone installs the exact same version of a dependency and additionally also speeds up the installation of npm packages: https://docs.npmjs.com/files/package-lock.json

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

jancborchardt · 2017-11-14T18:40:46Z

So I did make install-composer-deps, but then on the next it fails like this:

jan@Rechenknecht:~/nextcloud/apps/mail$ make optimize-js 
npm install --production
npm WARN grunt-jscs@1.8.0 requires a peer of grunt@~0.4.2 but none is installed. You must install peer dependencies yourself.

up to date in 2.901s
./node_modules/webpack/bin/webpack.js --config js/webpack.prod.config.js
module.js:515
    throw err;
    ^

Error: Cannot find module 'json-stable-stringify'
    at Function.Module._resolveFilename (module.js:513:15)
    at Function.Module._load (module.js:463:25)
    at Module.require (module.js:556:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/home/jan/nextcloud/apps/mail/node_modules/ajv/lib/compile/util.js:19:20)
    at Module._compile (module.js:612:30)
    at Object.Module._extensions..js (module.js:623:10)
    at Module.load (module.js:531:32)
    at tryModuleLoad (module.js:494:12)
    at Function.Module._load (module.js:486:3)
Makefile:39: recipe for target 'optimize-js' failed
make: *** [optimize-js] Error 1

ChristophWurst · 2017-11-15T14:05:53Z

but then on the next it fails like this:

See #615

jancborchardt · 2017-11-16T15:40:29Z

Doesn’t seem to work for me, even with the fixed optimize-js:

jancborchardt · 2017-11-16T15:57:29Z

After switching folders now it seemed to load. :D

jancborchardt

Weeeeeeee! Greaaat work!

ChristophWurst · 2017-11-16T16:06:14Z

Whoops now I was too happy about the approved review that I forgot to squash the commits 🙈

ChristophWurst · 2017-11-16T16:07:22Z

@jakobsack don't forget to claim your bounties for #463 and #236 😉

Titan-C · 2018-03-09T02:16:31Z

How is this configuration activated? I have nextcloud 13 & contacs 2.1.2. I load my contacts but avatars are not fetched.

ChristophWurst · 2018-03-09T09:25:05Z

Please file a ticket for this bug. Thanks you.

jakobsack requested a review from Gomez August 25, 2017 16:14

ChristophWurst added 3. to review enhancement labels Aug 28, 2017

ChristophWurst added this to the 0.8 milestone Aug 28, 2017

ChristophWurst previously requested changes Aug 28, 2017

View reviewed changes

jakobsack changed the title ~~Add new controller for avatar fetching~~ [WIP] Add new controller for avatar fetching Aug 28, 2017

ChristophWurst added 2. developing and removed 3. to review labels Aug 28, 2017

jakobsack added 3. to review and removed 2. developing labels Aug 28, 2017

jakobsack added 2. developing and removed 3. to review labels Sep 5, 2017

jakobsack force-pushed the avatars branch 2 times, most recently from 5ccd8e5 to 144297a Compare October 19, 2017 18:37

jakobsack added 3. to review and removed 2. developing labels Oct 19, 2017

jakobsack force-pushed the avatars branch from 144297a to 2ebb73f Compare October 22, 2017 20:36

ChristophWurst self-assigned this Oct 23, 2017

jakobsack added 5 commits October 28, 2017 15:01

Add new controller for avatar fetching

b68d8ad

Show gravatar images in message list

97430c7

Introduce caching for avatars

96ea8e1

Try to fix tests

7a6fd92

Fix tiny code style issue

0a49e4f

ChristophWurst requested review from jancborchardt and skjnldsv November 13, 2017 13:42

ChristophWurst added 2 commits November 14, 2017 12:15

Merge branch 'master' of github.com:/nextcloud/mail into avatars

d0b492b

fixup! Cleanup, refactor and test new AvatarService

50216e4

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

Merge branch 'master' of github.com:/nextcloud/mail into avatars

8aba36e

ChristophWurst mentioned this pull request Nov 15, 2017

Fix 'make optimize-js' #615

Merged

jancborchardt closed this in #615 Nov 16, 2017

jancborchardt reopened this Nov 16, 2017

jancborchardt approved these changes Nov 16, 2017

View reviewed changes

ChristophWurst merged commit b11cdf6 into master Nov 16, 2017

ChristophWurst deleted the avatars branch November 16, 2017 16:05

This was referenced Nov 20, 2017

More sources to get avatars from [$15 awarded] #236

Closed

Further avatar sources [$50 awarded] #463

Closed

ChristophWurst modified the milestones: 0.8, 0.7.5 Nov 21, 2017

nextcloud locked and limited conversation to collaborators Mar 9, 2018

Uh oh!

Conversation

jakobsack commented Aug 25, 2017 • edited by ChristophWurst Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobsack commented Aug 28, 2017

Uh oh!

ChristophWurst commented Sep 5, 2017

Uh oh!

jakobsack commented Sep 5, 2017

Uh oh!

ChristophWurst commented Sep 5, 2017

Uh oh!

jancborchardt commented Oct 11, 2017

Uh oh!

jakobsack commented Oct 19, 2017

Uh oh!

jakobsack commented Oct 22, 2017

Uh oh!

ChristophWurst commented Nov 13, 2017

Uh oh!

ChristophWurst commented Nov 13, 2017

Uh oh!

ChristophWurst commented Nov 13, 2017

Uh oh!

jancborchardt commented Nov 14, 2017

Uh oh!

ChristophWurst commented Nov 14, 2017

Uh oh!

jancborchardt commented Nov 14, 2017

Uh oh!

ChristophWurst commented Nov 15, 2017

Uh oh!

jancborchardt commented Nov 16, 2017

Uh oh!

jancborchardt commented Nov 16, 2017

Uh oh!

jancborchardt left a comment

Choose a reason for hiding this comment

Uh oh!

ChristophWurst commented Nov 16, 2017

Uh oh!

ChristophWurst commented Nov 16, 2017

Uh oh!

Titan-C commented Mar 9, 2018

Uh oh!

ChristophWurst commented Mar 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jakobsack commented Aug 25, 2017 •

edited by ChristophWurst

Loading