Implement minimal createMailbox CREATE feature. #43
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since folder management is not entirely trivial given the interaction of namespaces and delimiters, it could make sense to also support some type of slightly higher level API as well. This could entail having listNamespaces and listMailboxes called together and retain their results, which starts to get tricky. So for now I figured I would just implement the low level CREATE binding.
asutherland
added a commit
to asutherland/gaia-email-libs-and-more
that referenced
this pull request
Nov 26, 2014
When we moved to email.js, createFolder was completely broken. I've done the following: - Implemented a createMailbox function in browserbox: emailjs/emailjs-imap-client#43 - Cleaned up our createFolder job. This is reasonably straightforward. The big change is we no longer perform a "LIST" after the fact to see what name transforms might have happened. I added a comment explaining this change. It's probably important to note that the original choice of doing that I think was primarily a cop-out on my part from trying to understand the folder naming rules and having had the goal of createFolder being a comprehensive feature we'd surface to users. - Fixed a testing infrastructure oversight. expect_runOp had explicit support to expect a specific error result for runOp_end. Unfortunately due to an asymmetry with other log types (ex: event) and the "error" argument having a value of false, we were not actually checking the value! For the purposes of minimalism, I updated the log definitions rather than log.js. This required minimal changes to test_disaster_recovery.js and to js/jobmixins.js for downloadBodyReps to make sure the error code was explicitly null rather than undefined. - I cleaned up comments as I went. - mail-fakeservers enhancements that get pulled in please see the commit comment there: mozilla-b2g/mail-fakeservers#28 Testing-wise, we have the following coverage: - The unit tests I added to browserbox above. They're pretty simple, but get us coverage of utf-7 encoding/decoding verification which we don't test in here. - Added a unit test for the path-generating logic that has pretty good coverage (I feel). - Added a fakeserver-using test that makes sure we create the folders and also tests both of the expected exceptional cases: "the folder totally already exists and it's known locally, dude", and "the server ALREADYEXISTS on the server". Not explicitly tested is that if the server just outright fails that we generate the correct error. However, I manually verified we do this during the process of development because of doing this TDD-style.
asutherland
added a commit
to asutherland/gaia-email-libs-and-more
that referenced
this pull request
Nov 27, 2014
When we moved to email.js, createFolder was completely broken. I've done the following: - Implemented a createMailbox function in browserbox: emailjs/emailjs-imap-client#43 - Cleaned up our createFolder job. This is reasonably straightforward. The big change is we no longer perform a "LIST" after the fact to see what name transforms might have happened. I added a comment explaining this change. It's probably important to note that the original choice of doing that I think was primarily a cop-out on my part from trying to understand the folder naming rules and having had the goal of createFolder being a comprehensive feature we'd surface to users. - Fixed a testing infrastructure oversight. expect_runOp had explicit support to expect a specific error result for runOp_end. Unfortunately due to an asymmetry with other log types (ex: event) and the "error" argument having a value of false, we were not actually checking the value! For the purposes of minimalism, I updated the log definitions rather than log.js. This required minimal changes to test_disaster_recovery.js and to js/jobmixins.js for downloadBodyReps to make sure the error code was explicitly null rather than undefined. - I cleaned up comments as I went. - mail-fakeservers enhancements that get pulled in please see the commit comment there: mozilla-b2g/mail-fakeservers#28 Testing-wise, we have the following coverage: - The unit tests I added to browserbox above. They're pretty simple, but get us coverage of utf-7 encoding/decoding verification which we don't test in here. - Added a unit test for the path-generating logic that has pretty good coverage (I feel). - Added a fakeserver-using test that makes sure we create the folders and also tests both of the expected exceptional cases: "the folder totally already exists and it's known locally, dude", and "the server ALREADYEXISTS on the server". Not explicitly tested is that if the server just outright fails that we generate the correct error. However, I manually verified we do this during the process of development because of doing this TDD-style.
|
Thanks! Basically this is exactly what I had on mind. In the future I might just edit the method to accept arrays or some kind of structured representation of the mailbox path, so the user would not have to worry about folder separators and such but right now this is more than good enough. |
andris9
added a commit
that referenced
this pull request
Nov 27, 2014
Implement minimal createMailbox CREATE feature.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since folder management is not entirely trivial given the interaction of
namespaces and delimiters, it could make sense to also support some type
of slightly higher level API as well.
This could entail having listNamespaces and listMailboxes called together
and retain their results, which starts to get tricky. So for now I figured
I would just implement the low level CREATE binding.
(In gaia-email-libs-and-more we've got some existing logic which I've now cleaned up. I've also added a bunch of unit tests in a pending pull request. I can try to upstream that once we reach consensus.)