Skip to content

Handle dovecot sieve folder when subdir of INBOX#728

Merged
ChristophWurst merged 3 commits into
nextcloud:masterfrom
j0s:fix/dovecot-sieve
Jan 26, 2018
Merged

Handle dovecot sieve folder when subdir of INBOX#728
ChristophWurst merged 3 commits into
nextcloud:masterfrom
j0s:fix/dovecot-sieve

Conversation

@j0s
Copy link
Copy Markdown

@j0s j0s commented Jan 22, 2018

Pull request #520 introduced special handling of dovecot's
'dovecot.sieve' folder, so that it isn't queried for a sync
token. A common configuration of dovecot is to put all folders as
subfolders of INBOX. This pull request adds support for
cases when the sieve folder is a subfolder of INBOX.

Pull request nextcloud#520 introduced special handling of dovecot's
'dovecot.sieve' folder, so that it isn't queried for a sync
token. A common configuration of dovecot is to put all folders as
subfolders of INBOX. This pull request adds support for
cases when the sieve folder is a subfolder of INBOX.

Signed-off-by: Johan Sandelin <johan.sandelin@gmail.com>
@ChristophWurst
Copy link
Copy Markdown
Member

Hey, @j0s!

Thanks a lot for your PR. I think the proper solution would be to fix #386 because then the prefix would be removed and thus that wouldn't be an issue, right?

@j0s
Copy link
Copy Markdown
Author

j0s commented Jan 22, 2018

That looks like the proper solution, yes, if the solution involves removing the INBOX prefix from all subfolders. However the fix in #652 (comment) does not solve the problem for me. Unfortunately I'm not comfortable enough with docker to create an image for you to test.

@ChristophWurst
Copy link
Copy Markdown
Member

I'm fine with accepting this temporary fix if

  • You fix the coding style and use camel case variable names and the short array syntax
  • Add a source code comment that this is a hack erm … I mean temporary fix 😉
  • Add a notice to Subfolders and Dovecot #386 so that we don't forge to remove this fix once that ticket has been addressed in a PR

Code style has been fixed, and a comment that this workaround
should be removed once nextcloud#386 has been closed has been added.

Signed-off-by: Johan Sandelin <johan.sandelin@gmail.com>
@j0s j0s mentioned this pull request Jan 23, 2018
@ChristophWurst
Copy link
Copy Markdown
Member

Thanks a lot!

@ChristophWurst
Copy link
Copy Markdown
Member

ChristophWurst commented Jan 24, 2018

@ChristophWurst
Copy link
Copy Markdown
Member

@j0s FYI your email address used for the commits here is not associated with your account. You might want to add it to your account, then GitHub will correctly show you as author 😉

@ChristophWurst
Copy link
Copy Markdown
Member

First-time contributor

Congrats on your first contribution btw 🙌 Looking forward to more contributions ✨ 🚀

Copy link
Copy Markdown
Member

@ChristophWurst ChristophWurst 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!

@ChristophWurst ChristophWurst merged commit 0961f1d into nextcloud:master Jan 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants