Closed Bug 532522 Opened 15 years ago Closed 13 years ago

gloda indexes deleted folders, that are subfolders of trash

Categories

(MailNews Core :: Database, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 10.0

People

(Reporter: rkent, Assigned: protz)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gloda key])

Attachments

(1 file, 1 obsolete file)

I have many folders that I previously deleted, that are sitting as subfolders of trash. Gloda should not index them, but does.
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [gloda key]
Attached patch Patch v1 (obsolete) — Splinter Review
I guess you'll want to have tests for that, so just asking for feedback. If not, feel free to r+ :-).
Assignee: nobody → jonathan.protzenko
Status: NEW → ASSIGNED
Attachment #551965 - Flags: feedback?(bugmail)
Comment on attachment 551965 [details] [diff] [review]
Patch v1

This seems like one of the right places to change the logic, but please use nsIMsgFolder's isSpecialFolder helper which already has the ability to check ancestors:
http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgFolder.idl#412

Unfortunately, I believe covering the case rkent mentions requires a bit more legwork.  Specifically, deleting a folder by moving it to the trash will likely either trigger folderMoveCopyCompleted or folderRenamed notifications (they confuse me).  These will need to make sure to update the folder indexing priority, and potentially need to trigger deletion logic on the messages inside if it constitutes a priority change.

And yes, please, I would like a test, as this seems eminently able to be regressed.  You will note I have helpfully created stub test functions for you in base_index_messages.js (test_folder_deletion_single/test_folder_deletion_nested).  Some might say I prophetically saw the need for these tests, others (more correctly) that I should have written them long ago...  Note that you'll need to add them to the list of tests at the bottom of the file since they are not listed.
Attachment #551965 - Flags: feedback?(bugmail) → feedback+
So the tests were painful for that one. Some remarks:
- I added the new test at the bottom of the list, because otherwise, test_pending_commit_tracker_flushes_correctly would complain message headers from the old folder (that was moved) being invalid, and emit an error; the case is perfectly documented in the source, and understood, except it uses ._error instead of ._warn, so well...
- I just wrote one test, as the nested case is a strict superset of the simple case,
- I had to disable one part of the test unless on local folder injection mode, because the point was precisely to test injecting into these newly moved folders, and since they were not managed by make_sets_in_folder, the test would fail because there was no fake folder associated to them, and we cannot inject directly into imap folders.

Also, the documentation pretends that make_sets_in_folders returns a nsIMsgLocalMailFolder which is BALD-FACED LIE! :-D I had to write a wrapper around that to make sure I always give nsIMsgFolders to the msgCopyService. That's pretty much all. The test is pretty intensive, so I believe everything is right...
Attachment #551965 - Attachment is obsolete: true
Attachment #552721 - Flags: review?(bugmail)
Comment on attachment 552721 [details] [diff] [review]
Patch v2, with tests

async_driver already uses do_execute_soon under the hood, so you don't need to have it do a timeout.  (That may not have always been true, but it's been true a long time now.)

Your folder moving logic seems like it would do great in messageInjection.js, please move it there.

Please remove your pretty color dump you left in.

Otherwise, things look fantastic!  Thanks!
Attachment #552721 - Flags: review?(bugmail) → review+
http://hg.mozilla.org/comm-central/rev/5273b72d2e58
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 10.0
You need to log in before you can comment on or make changes to this bug.