Refactor server-side folder management implementation #271

ChristophWurst · 2017-01-04T12:21:49Z

I finally found the time to study the server-side implementation and refactor it into a testable, extensible form. This is needed for the planned features like threading. Before I start with those I want the code to be tested almost 100% because testing IMAP manually is a major PITA. This PR shows that moving the existing code a bit around can lead to a much better code quality. Actually, I could even remove some of the code in place because it's just not needed or can be done more efficiently.

better OOP architecture
dependency injection everywhere
abstraction where it made sense
SRP applied as much as possible
added a bunch of tests for the new code
REST API compatible to old implementation
new code is not optimized
old code was not removed

TODO:

100% code coverage (actually only a few code paths missing)
~~check why unified inbox flagged folder is not shown any more~~ well, we never had that feature 🙈
file ticket about the js bug that occurs when accounts are added
re-enable search mailbox (starred messages filter)

jancborchardt · 2017-01-09T11:24:39Z

Aaawesome! :) Let us know as soon as it’s reviewable! cc @nextcloud/mail

ChristophWurst · 2017-01-14T15:32:32Z

I have to do some more refactoring before I can finish this one unfortunately, otherwise I'd have to add those ugly unified inbox hacks to the new shiny code too 😢 Should be easy though 😉

ChristophWurst · 2017-01-27T11:45:29Z

@nextcloud/mail this should be ready for a review 🚀

Let me know if something breaks 😉

ChristophWurst · 2017-01-27T14:50:11Z

Open todo (though that shouldn't break anything when added back): caching.

* better OOP architecture * dependency injection everywhere * abstraction where it made sense * SRP applied as much as possible * added a bunch of tests for the new code * REST API compatible to old implementation * new code is not optimized * old code was not removed Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

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

…p?id=50688 Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

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

ChristophWurst · 2017-02-09T19:47:59Z

Please help with testing/reviewing this PR @nextcloud/mail @Quix0r

skjnldsv · 2017-02-10T04:51:35Z

That is a long pr! Care to told us how to review? :p

ChristophWurst · 2017-02-10T07:19:35Z

That is a long pr! Care to told us how to review? :p

Yup, sorry 'bout that. Well, just look at the diff 😉 I pretty much re-wrote the old implementation and added a bunch of tests. Unfortunately, I cannot tell you how this could be reviewed easily 😟

skjnldsv · 2017-02-10T14:23:04Z

Sooo, read the js and process it into your head? 😆

ChristophWurst · 2017-02-12T19:45:11Z

Sooo, read the js and process it into your head? 😆

Sure you can do that hehe

Most of the changes were made to the server implementation though.

So I basically rewrote the code that handles folder management, which is mainly only about listing folders of an IMAP account. All we do after we got the list is translation, sorting, guessing the folder role and building the folder tree from the source list.

To have the most flexibility, maintainability, testability and readability I tried to split the code into logical parts:

a high level service to use in the controller -> a new folder manager
a class that translates IMAP into the OOP world -> a folder mapper
a class to handle translation -> a translator class

The rest of the rather big diff is caused by tests because I almost hit 100% coverage on the new classes (only one little, uncritical if-statement is not covered).

Thus you can start at the controller, look what it calls (spoiler: a single method on the manager) and review the delegated functionality in there. Everything should be self-explanatory from the class/method names, or at least that was my intention 😉

Let me know if there's anything else you need to know to understand the changes :-)

Quix0r · 2017-02-13T08:42:39Z

Hmm, /js/models/account.js has lost some coverage: https://coveralls.io/builds/10080949/source?filename=js%2Fmodels%2Faccount.js

jancborchardt · 2017-02-22T16:10:23Z

Been using it today and can’t find any issues so far. :) Any blockers or things to add back in this specific PR?

ChristophWurst · 2017-02-22T16:44:09Z

@jancborchardt cool, thanks for testing! I think this is ready for merging 😉

ChristophWurst · 2017-02-22T16:45:08Z

About the js code coverage: it's not really reliable. It goes up and down. We're not at 33% coverage anyway, the number seems to be wrong :-|

jancborchardt

Couldn’t find any breakage so far, let’s get it in :)

jancborchardt · 2017-02-22T16:47:49Z

Perfect! Looking forward to the other unified folders. ;) What can I test next?

skjnldsv · 2017-02-22T17:55:12Z

Damn, sorry @ChristophWurst I forgot this one! 😢
Thanks @jancborchardt

Quix0r · 2017-02-23T09:34:59Z

@ChristophWurst TODO 3 is still open in your list!

ChristophWurst · 2017-02-23T09:44:59Z

@Quix0r will file an issue right away 🚀

Quix0r

Already got merged but maybe still good to consider that I have found and maybe then reflect this in next PR.

Quix0r · 2017-02-23T09:37:38Z

js/models/account.js

+				return undefined;
+			}
+			return _.find(this.folders, function(folder) {
+				// TODO: handle special folders in subfolder properly


Maybe use @TODO so documentation tools can pick it up? It is very common though.

makes sense, will use it next time.

Then maybe apply this globally, e.g. search for // TODO and replace it with // @TODO. And same with multi-line comments.

Quix0r · 2017-02-23T09:38:30Z

lib/Contracts/IMailManager.php

+
+interface IMailManager {
+
+	public function getFolders(Account $account);


Methods declared in interfaces are always public, no need to have the then redundant public statement around.

Seems like good practice to always be specific. :)

Quix0r · 2017-02-23T09:39:23Z

lib/Controller/FoldersController.php

 	 */
-	public function __construct($appName, IRequest $request, AccountService $accountService, $UserId) {
+	public function __construct($appName, IRequest $request,


Really split this into more than one line? If that is the coding-convention here, okay.

Quix0r · 2017-02-23T09:43:01Z

tests/Controller/FoldersControllerTest.php

-		$account = $this->getMockBuilder('OCA\Mail\Account')
-			->disableOriginalConstructor()
-			->getMock();
+	public function testIndex() {


Please don't kill doc tags, they produce nice documentation (e.g. generated by doxygen).

What do you mean by doc tags?

/** * Some documented method, does foo things and need bar for its purpose. * You have to enter a better description than this * * @param string $bar Some bar string * @return void */ public function doFoo (string $bar) { // Do foo things with bar [...] }

That is a so called doc-tag. :-) It will produce nice documentation in such tools (see Doxygen, it can process more than just C/C++). And PHP 7 supports native type-hints. 👍

Quix0r · 2017-02-23T09:44:51Z

tests/FolderTest.php

+<?php
+
+/**
+ * @author Christoph Wurst <christoph@winzerhof-wurst.at>


Have you (the team) already made a decision for @copyright tag? Maybe give it to FSFE so they can handle any law issues for you. Maybe on the developer team (in general) to express that people develop code not for themselves but for the project?

cc @schiessle

What do you think is best for this project?

Quix0r · 2017-02-23T09:45:33Z

tests/FolderTest.php

+		$this->assertFalse($this->folder->isSearchable());
+	}
+
+	public function testJsonSerialize() {


Lots of code is missing documentation (what does it do, what is the method foo()and bar() for?)

That's true, many segments of our code are either only documented poorly or not documented at all. We can work on that ofc :-)

Quix0r · 2017-02-23T09:46:19Z

tests/data/folders_german.json

@@ -1,81 +0,0 @@
-{


Got rid of an old file? Good. 👍

lock · 2018-11-21T08:44:26Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and questions.

ChristophWurst added 2. developing technical debt labels Jan 4, 2017

ChristophWurst mentioned this pull request Jan 9, 2017

Inbox Folders recognized as Subfolders instead of Folders in the sidebar #150

Open

ChristophWurst mentioned this pull request Jan 19, 2017

Client side unified inbox folder #292

Merged

ChristophWurst force-pushed the refactor/server-side-folder-management branch from 07f3998 to ccf8ab9 Compare January 26, 2017 20:10

ChristophWurst added 3. to review and removed 2. developing labels Jan 27, 2017

ChristophWurst requested review from Gomez, nickvergessen, jancborchardt and skjnldsv January 27, 2017 11:45

ChristophWurst added 2. developing and removed 3. to review labels Jan 31, 2017

ChristophWurst added 12 commits February 9, 2017 20:36

create a search folder automatically

d4d5792

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

use SearchFolder instead of SearchMailbox

1524004

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

increase test coverage

3782323

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

group tests to allow excluding them

dd71170

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

add even more tests :-)

8c97264

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

use server's test case

2d834e2

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

remove dead code and test folder sorting

f481b99

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

return the sorted array

d0d746c

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

do not pass mocked objects to fix php bug https://bugs.php.net/bug.ph…

6f83ddc

…p?id=50688 Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

fix comment

b74212b

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

re-enable IMAP caching

9dcf840

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

ChristophWurst force-pushed the refactor/server-side-folder-management branch from 428d933 to 9dcf840 Compare February 9, 2017 19:46

ChristophWurst added 3. to review and removed 2. developing labels Feb 9, 2017

nickvergessen removed their request for review February 10, 2017 14:40

This was referenced Feb 15, 2017

Dovecot + prefixes [$15 awarded] #185

Closed

Unified folders for other special folders (Favorites, Drafts, Sent, Trash) #313

Closed

jancborchardt approved these changes Feb 22, 2017

View reviewed changes

jancborchardt merged commit 03573e2 into master Feb 22, 2017

jancborchardt deleted the refactor/server-side-folder-management branch February 22, 2017 16:47

ChristophWurst mentioned this pull request Feb 23, 2017

JS error when adding a second account #320

Closed

Quix0r reviewed Feb 23, 2017

View reviewed changes

reubenlillie mentioned this pull request Jun 7, 2017

Subfolders and Dovecot #386

Closed

phsc84 mentioned this pull request Jul 3, 2017

add new mailfolders [$50 awarded] #199

Closed

ChristophWurst mentioned this pull request Aug 2, 2017

Mail app won't accept address with "." in it #155

Closed

lock bot locked and limited conversation to collaborators Nov 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor server-side folder management implementation #271

Refactor server-side folder management implementation #271

ChristophWurst commented Jan 4, 2017 •

edited

Loading

jancborchardt commented Jan 9, 2017

ChristophWurst commented Jan 14, 2017

ChristophWurst commented Jan 27, 2017

ChristophWurst commented Jan 27, 2017

ChristophWurst commented Feb 9, 2017

skjnldsv commented Feb 10, 2017

ChristophWurst commented Feb 10, 2017

skjnldsv commented Feb 10, 2017

ChristophWurst commented Feb 12, 2017 •

edited

Loading

Quix0r commented Feb 13, 2017

jancborchardt commented Feb 22, 2017

ChristophWurst commented Feb 22, 2017

ChristophWurst commented Feb 22, 2017

jancborchardt left a comment

jancborchardt commented Feb 22, 2017

skjnldsv commented Feb 22, 2017

Quix0r commented Feb 23, 2017

ChristophWurst commented Feb 23, 2017

Quix0r left a comment

Quix0r Feb 23, 2017

ChristophWurst Feb 23, 2017

Quix0r Feb 23, 2017

Quix0r Feb 23, 2017

jancborchardt Mar 26, 2017

Quix0r Feb 23, 2017

Quix0r Feb 23, 2017

ChristophWurst Feb 23, 2017

Quix0r Mar 27, 2017 •

edited

Loading

Quix0r Feb 23, 2017

ChristophWurst Feb 23, 2017

Quix0r Feb 23, 2017

ChristophWurst Feb 23, 2017

Quix0r Feb 23, 2017

lock bot commented Nov 21, 2018


		interface IMailManager {

		public function getFolders(Account $account);

Refactor server-side folder management implementation #271

Refactor server-side folder management implementation #271

Conversation

ChristophWurst commented Jan 4, 2017 • edited Loading

jancborchardt commented Jan 9, 2017

ChristophWurst commented Jan 14, 2017

ChristophWurst commented Jan 27, 2017

ChristophWurst commented Jan 27, 2017

ChristophWurst commented Feb 9, 2017

skjnldsv commented Feb 10, 2017

ChristophWurst commented Feb 10, 2017

skjnldsv commented Feb 10, 2017

ChristophWurst commented Feb 12, 2017 • edited Loading

Quix0r commented Feb 13, 2017

jancborchardt commented Feb 22, 2017

ChristophWurst commented Feb 22, 2017

ChristophWurst commented Feb 22, 2017

jancborchardt left a comment

Choose a reason for hiding this comment

jancborchardt commented Feb 22, 2017

skjnldsv commented Feb 22, 2017

Quix0r commented Feb 23, 2017

ChristophWurst commented Feb 23, 2017

Quix0r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quix0r Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lock bot commented Nov 21, 2018

ChristophWurst commented Jan 4, 2017 •

edited

Loading

ChristophWurst commented Feb 12, 2017 •

edited

Loading

Quix0r Mar 27, 2017 •

edited

Loading