support lists of contacts with list inside it, at least when sending messages to the parent list

RESOLVED FIXED in Thunderbird 51.0

Status

Thunderbird
Message Compose Window
--
enhancement
RESOLVED FIXED
11 months ago
6 months ago

People

(Reporter: jerome.ploquin, Assigned: jerome.ploquin)

Tracking

45 Branch
Thunderbird 51.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

11 months ago
Created attachment 8772353 [details] [diff] [review]
cm2_tb45.1.1_patch_72_rev72.patch

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; Trident/7.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; .NET4.0C; .NET4.0E; rv:11.0) like Gecko

Steps to reproduce:

1/ Create a list named l0 with a contact
2/ Create a liste named l1 and put l0 in l1
3/ Send a mail to l1


Actual results:

The mail is not sent


Expected results:

The mail should have been sent (or building a list of list must be forbidden). The attached file is a patch that allows the usage of lists of lists.

Comment 1

11 months ago
Comment on attachment 8772353 [details] [diff] [review]
cm2_tb45.1.1_patch_72_rev72.patch

Jerome is our friend from the French government. They have a whole group of people working on TB. He agreed to send some patches.

Jerome, you need to mark the attachment as patch and then request review from the appropriate person. If you don't know, ask me, Aceman or Magnus (mkmelin).

I think Aceman does address book stuff.
Attachment #8772353 - Attachment is patch: true
Attachment #8772353 - Flags: review?(acelists)

Comment 2

11 months ago
Comment on attachment 8772353 [details] [diff] [review]
cm2_tb45.1.1_patch_72_rev72.patch

Review of attachment 8772353 [details] [diff] [review]:
-----------------------------------------------------------------

Please change to commit message to English.

At https://dxr.mozilla.org/comm-central/source/mailnews/addrbook/public/nsIAbDirectory.idl#182 the comment claims we can only add maillists to the toplevel directories. How comes you could create a maillist inside a maillist? It probably isn't enforced enough. Becase the addressbook UI is confused with such objects. It does not display a tree if there is maillist inside maillist (and then you could have the same maillist in multiple maillists). Also it displays and edits a maillist in maillist as a single contact.

So how do you maintain such lists? Does this work with the users, are they aware how to not corrupt such maillists?

::: comm-esr45/mailnews/compose/src/nsMsgCompose.cpp
@@ +4811,5 @@
> +struct nsMsgRecipientComparator
> +{
> +  bool Equals(const nsMsgRecipient &recipient,
> +              const nsMsgRecipient &recipientToFind) const {
> +

Surplus blank line.

@@ +4813,5 @@
> +  bool Equals(const nsMsgRecipient &recipient,
> +              const nsMsgRecipient &recipientToFind) const {
> +
> +    if (!recipient.mEmail.Equals(recipientToFind.mEmail,
> +                                  nsCaseInsensitiveStringComparator()))

Space before { .

@@ +4860,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = existingCard->GetPrimaryEmail(newRecipient.mEmail);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    if (newRecipient.mName.IsEmpty() && newRecipient.mEmail.IsEmpty()){

Space before { .

@@ +4867,5 @@
> +
> +    // First check if it's a mailing list
> +    size_t index = allMailListArray.IndexOf(newRecipient, 0, nsMsgMailListComparator());
> +    if (index != allMailListArray.NoIndex &&
> +        allMailListArray[index].mDirectory)

Can you use allMailListArray.Contains(newRecipient, nsMsgMailListComparator()) here?

@@ +4869,5 @@
> +    size_t index = allMailListArray.IndexOf(newRecipient, 0, nsMsgMailListComparator());
> +    if (index != allMailListArray.NoIndex &&
> +        allMailListArray[index].mDirectory)
> +    {
> +      //check if maillist processed

Space after // and start with capital letter and end with a dot if it is a sentence.

@@ +4871,5 @@
> +        allMailListArray[index].mDirectory)
> +    {
> +      //check if maillist processed
> +      size_t index2 = mailListProcessed.IndexOf(newRecipient, 0, nsMsgMailListComparator());
> +      if (mailListProcessed.NoIndex != index2) {

Can you use mailListProcessed.Contains(newRecipient, nsMsgMailListComparator()) here?

@@ +4885,5 @@
> +      rv = ResolveMailList(directory2,
> +                            allDirectoriesArray,
> +                            allMailListArray,
> +                            mailListProcessed,
> +                            aListMembers);

One space off in indentation.

@@ +4893,5 @@
> +    }
> +
> +    //check if recipient is in aListMembers
> +    index = aListMembers.IndexOf(newRecipient, 0, nsMsgRecipientComparator());
> +    if (allMailListArray.NoIndex != index) {

Again, .Contains() ?

@@ +4960,5 @@
>        stillNeedToSearch = false;
>        for (uint32_t i = 0; i < MAX_OF_RECIPIENT_ARRAY; i ++)
>        {
> +        mailListProcessed.Clear();
> +        

whitespace

@@ +4968,5 @@
>            nsMsgRecipient &recipient = recipientsList[i][j];
>            if (!recipient.mDirectory)
>            {
>              // First check if it's a mailing list
> +            size_t index = mailListArray.IndexOf(recipient, 0, nsMsgMailListComparator());

Can you use index_type instead of size_t at all places?

@@ +4974,5 @@
>                  mailListArray[index].mDirectory)
>              {
> +              //check mailList Processed
> +              size_t index2 = mailListProcessed.IndexOf(recipient, 0, nsMsgMailListComparator());
> +              if (mailListProcessed.NoIndex != index2) {

.Contains()

@@ +4993,5 @@
> +                                   mailListArray,
> +                                   mailListProcessed,
> +                                   members);
> +              NS_ENSURE_SUCCESS(rv, rv);
> +              

whitespace

@@ +5003,5 @@
> +              for (uint32_t c = 0; c < members.Length(); c++)
> +              {
> +                nsMsgRecipient &member = members[c];
> +                index2 = recipientsList[i].IndexOf(member, 0, nsMsgRecipientComparator());
> +                if (recipientsList[i].NoIndex == index2) {

.Contains()

Updated

11 months ago
Assignee: nobody → jerome.ploquin
Severity: normal → enhancement
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Summary: lists of contacts with list inside → support lists of contacts with list inside it, at least when sending messages to the parent list

Comment 3

10 months ago
I noticed that the patch uses Windows line endings (CR/LF). Mozilla source code is generally with Unix/Mac line endings (LF), so you'd have to change that or the patch won't apply.

Comment 4

10 months ago
Created attachment 8790049 [details] [diff] [review]
cm2_tb45.1.1_patch_72_rev72.patch (line endings replaced, removed comm-esr45, refreshed)

Also, the patch needs to be applicable to comm-central, not comm-esr45.
It didn't apply, but I've fixed it.

Comment 5

10 months ago
I've tried the patch and it works.

I think there is a misunderstanding here. This bug is not about creating lists in lists.
This bug is about adding the name of a list as an address to another list.

So I have a list L-0 that contains jk@jk.com.
I have a list L-1 at contains |L-0 <L-0>|.

In current trunk you can create such a setup, but apparently the sending then doesn't work (I haven't tried). With the patch it works.

I could imagine that this is a useful feature. Anyway, as comment #0 says: Make is work or disallow it.

(In reply to :aceman from comment #2)
> How come you could create a maillist inside a maillist? It probably isn't enforced enough.
Does my comment above answer that?

===

In general about the patch:
There are a few trailing spaces and comments should be:
// This is a comment starting with upper-case and ending with a full stop.

Comment 6

10 months ago
Created attachment 8790058 [details] [diff] [review]
cm2_tb45.1.1_patch_72_rev72.patch (now formally correct)

OK, I fixed the nits from Aceman's preliminary review:
- Fixed commit message.
- Fixed comments.
- Removed trailing spaces and other white-space issues.
- Used .Contains() where possible. Not possible in two cases
  where index is used later.
Previously I already had
- replaced line endings
- removed comm-esr45
- refreshed the patch to current trunk.

Usually the patch author needs to do all this, but I don't want to discourage Jerome with those details. However, I would appreciate if further patches - and I hope to see many coming(!!) - matched our coding standards.

I think size_t is OK to use, I can't find any references to index_type in all of C-C.

There is one issue left:
ResolveMailList() should get a description.

As far as I can see, the patch works. I even tried recursive lists sending e-mail to L-1 (containing L-0), where L-0 not only includes an e-mail address but also L-1.

So let's get this reviewed.

I think this function is well worth having since in a larger enterprise, you can then structure your mailing lists.
Attachment #8772353 - Attachment is obsolete: true
Attachment #8790049 - Attachment is obsolete: true
Attachment #8772353 - Flags: review?(acelists)
Attachment #8790058 - Flags: review?(acelists)

Comment 7

10 months ago
Magnus mentioned that we need a good test for this. It's easy this time since we can just enhance
https://dxr.mozilla.org/comm-central/source/mailnews/compose/test/unit/test_expandMailingLists.js.

This is really a cheap test since the data is pre-canned and not constructed during the test.
(Assignee)

Comment 8

9 months ago
Thank you so much!
This funcion is very useful for large hierarchical organizations like ours.

We have noticed the rules that must be folllowed when producing a patch, and we'll try to do our best for the nexts.

Comment 9

9 months ago
(In reply to jerome.ploquin from comment #8)
> Thank you so much!
Can you give us a description for nsMsgCompose::ResolveMailList().
I can see that it resolves mailing list but a few words from the author would be good especially re. solving recursive inclusion.

> This function is very useful for large hierarchical organizations like ours.
Yes, no doubt, also for large enterprises.

> We have noticed the rules that must be followed when producing a patch, and
> we'll try to do our best for the next.
Keep them coming. If this is number 72, where are the other 71? ;-)

Comment 10

9 months ago
I'll expand the test in test_expandMailingLists.js. I've put listexpansion.mab into my profile, so I'll just add some mailing lists to it and then add more cases to the test.

Comment 11

9 months ago
OK, added:
1) family (list) = parents (list) + kids (list).
2) parents (list) = homer + marge + parents (list recursion).
3) kids (list) = older-kids (list) + maggie.
4) older-kids (list) = bart + lisa.
5) bad-kids (list) = older-kids + bad-younger-kids (list)
6) bad-younger-kids (list) = maggie + bad-kids (list recursion).
That should do it. The test will expand those six lists and check that the result is right.

Comment 12

9 months ago
With the patch applied, the existing test
mozilla/mach xpcshell-test mailnews/compose/test/unit/test_expandMailingLists.js
still passes, so that's a bonus ;-)
I'll add the new cases later.

Comment 13

9 months ago
Created attachment 8790498 [details] [diff] [review]
Test cases (v1a).

Test cases as per previous comment.
Attachment #8790498 - Flags: review?(acelists)

Comment 14

9 months ago
Created attachment 8790596 [details] [diff] [review]
cm2_tb45.1.1_patch_72_rev72.patch (v3-JK).

Added description for ResolveMailList and fixed some whitespace issues. Use interdiff to check it.
Attachment #8790058 - Attachment is obsolete: true
Attachment #8790058 - Flags: review?(acelists)
Attachment #8790596 - Flags: review?(acelists)

Comment 15

9 months ago
Comment on attachment 8790596 [details] [diff] [review]
cm2_tb45.1.1_patch_72_rev72.patch (v3-JK).

Review of attachment 8790596 [details] [diff] [review]:
-----------------------------------------------------------------

Logic looks OK, but I'd like Aceman to check it, too.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +4837,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = existingCard->GetPrimaryEmail(newRecipient.mEmail);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    if (newRecipient.mName.IsEmpty() && newRecipient.mEmail.IsEmpty()) {

Does this make sense? Can you send e-mail if the person doesn't have an e-mail address? I know, that was in the original code. Or perhaps you want to get the error upon checking the expanded addresses before send so you can correct this entry.

@@ -4962,5 @@
> -          NS_ENSURE_SUCCESS(rv, rv);
> -          rv = existingCard->GetPrimaryEmail(newRecipient.mEmail);
> -          NS_ENSURE_SUCCESS(rv, rv);
> -
> -          if (newRecipient.mName.IsEmpty() && newRecipient.mEmail.IsEmpty())

Here it was.
Attachment #8790596 - Flags: review+
(Assignee)

Comment 16

9 months ago
(In reply to Jorg K (GMT+2, PTO during summer) from comment #9)
> (In reply to jerome.ploquin from comment #8)
> > Thank you so much!
> Can you give us a description for nsMsgCompose::ResolveMailList().
> I can see that it resolves mailing list but a few words from the author
> would be good especially re. solving recursive inclusion.
I saw that you already did it: thank you, we should have done it more quickly
> 
> > This function is very useful for large hierarchical organizations like ours.
> Yes, no doubt, also for large enterprises.
> 
> > We have noticed the rules that must be followed when producing a patch, and
> > we'll try to do our best for the next.
> Keep them coming. If this is number 72, where are the other 71? ;-)
Some of them are coming! we have to verify that our patchs are corrects regarding of the tb patch rules.

Comment 17

9 months ago
Comment on attachment 8790498 [details] [diff] [review]
Test cases (v1a).

Review of attachment 8790498 [details] [diff] [review]:
-----------------------------------------------------------------

OK, nice samples:) So this also checks if we cope with recursive lists. I hope we do not promote creating those very much, just that we can cope with them in this particular place (expanding lists).

Please fix the 'ist' while in that file:
* @param aCheckTo - the expected To addresses (after possible ist population)
Attachment #8790498 - Flags: review?(acelists) → review+

Comment 18

9 months ago
Thanks, I'll land it with the typo fix after you've reviewed the code changes.

Comment 19

9 months ago
Comment on attachment 8790596 [details] [diff] [review]
cm2_tb45.1.1_patch_72_rev72.patch (v3-JK).

Review of attachment 8790596 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is working for me, I have played with it and the recipients seem to be resolved properly.
Thanks.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +4688,5 @@
>  }
>  
> +nsresult nsMsgCompose::GetABDirAndMailLists(const nsACString& aDirUri,
> +                                            nsCOMArray<nsIAbDirectory> &aDirArray,
> +                                            nsTArray<nsMsgMailList> &aMailListArray)

Please add documentation comment block for this function when you are updating it, similarly to ResolveMailList().

@@ +4885,2 @@
>  nsresult
>  nsMsgCompose::LookupAddressBook(RecipientsArray &recipientsList)

Please add documentation comment block for this function when you are updating it, similarly to ResolveMailList(). There s already some stub in nsMsgCompose.h .

@@ +4976,5 @@
> +              for (uint32_t c = 0; c < members.Length(); c++)
> +              {
> +                nsMsgRecipient &member = members[c];
> +                if (!recipientsList[i].Contains(member, nsMsgRecipientComparator())) {
> +                  recipientsList[i].InsertElementAt(j+pos, member);

We use spaces around operators, like "j + pos".

@@ +4985,1 @@
>                continue;

I think I would like "else" block here (as in if (mailist){ } else {single address}), but you would need to reindent the following "single address block". So leave it as is for now.
Attachment #8790596 - Flags: review?(acelists) → review+

Comment 20

9 months ago
(In reply to Jorg K (GMT+2, PTO during summer) from comment #5)
> I think there is a misunderstanding here. This bug is not about creating
> lists in lists.
> This bug is about adding the name of a list as an address to another list.
> 
> So I have a list L-0 that contains jk@jk.com.
> I have a list L-1 at contains |L-0 <L-0>|.
> 
> In current trunk you can create such a setup, but apparently the sending
> then doesn't work (I haven't tried). With the patch it works.
> 
> I could imagine that this is a useful feature. Anyway, as comment #0 says:
> Make is work or disallow it.
> 
> (In reply to :aceman from comment #2)
> > How come you could create a maillist inside a maillist? It probably isn't enforced enough.
> Does my comment above answer that?

Yes, but in your example when you add L-0 <L-0> to L-1 a new card is created with Display name=L-0 and Email=L-0. So now you have a maillist and a normal card with the same name. I can imagine that can cause problems, when look up the cards just by the name (a string).

Those bogus L-0 <L-0> cards are shown in the AB manager, but are not offered in e.g. compose autocomplete (maybe because they do not have a valid email). So it appears to work, but can be fragile.

So the ML creating UI still does not cope with sublists yet. So we still need to leave comments in the code that say lists in lists are NOT supported officially.

Comment 21

9 months ago
Created attachment 8791631 [details] [diff] [review]
cm2_tb45.1.1_patch_72_rev72.patch (v4-JK).

I don't know how I got the job of (pseudo-)documenting previously undocumented functions :-(

Please check whether you like it. Use interdiff. Also, I removed the 'continue' and did an else branch.
Attachment #8790596 - Attachment is obsolete: true
Attachment #8791631 - Flags: review?(acelists)

Comment 22

9 months ago
OK, typo: Lookup *an* array.

Comment 23

9 months ago
(In reply to Jorg K (GMT+2, PTO during summer) from comment #21)
> Created attachment 8791631 [details] [diff] [review]
> cm2_tb45.1.1_patch_72_rev72.patch (v4-JK).
> 
> I don't know how I got the job of (pseudo-)documenting previously
> undocumented functions :-(

I also do it all the time. It is because when you are working in the function/changing it, you learn what it is doing and what the args are. Then you are the best person to document it right away.
Yes, of course those functions should not have been allowed into the tree without documentation. But those were the wild times :)

Comment 24

9 months ago
Comment on attachment 8791631 [details] [diff] [review]
cm2_tb45.1.1_patch_72_rev72.patch (v4-JK).

Review of attachment 8791631 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +4892,5 @@
> +
> +/**
> + * Lookup and array of recipients which can be individual addresses
> + * or mailing lists. The result is returned by manipulating the
> + * recipient array passed in.

OK, just please also mention the maillists are replaced by the cards they contain and also that duplicates are removed (I think).
Attachment #8791631 - Flags: review?(acelists) → review+

Comment 25

9 months ago
Created attachment 8791645 [details] [diff] [review]
cm2_tb45.1.1_patch_72_rev72.patch (v5-JK).

You didn't notice that the description was 100% wrong ;-)
That function looks up To, Cc and Bcc, and returns the recipients. Nothing is passed in. Also, duplicates are not removed, however, recipients are not added more than once if they appear in multiple lists.

I removed the horrible if (NS_SUCCEEDED(rv)) which wasted a whole level of indentation in the entire function. Interdiff will look horrible.
Attachment #8791631 - Attachment is obsolete: true
Attachment #8791645 - Flags: review?(acelists)

Comment 26

9 months ago
Comment on attachment 8791645 [details] [diff] [review]
cm2_tb45.1.1_patch_72_rev72.patch (v5-JK).

(In reply to Jorg K (GMT+2, PTO during summer) from comment #25)
> I removed the horrible if (NS_SUCCEEDED(rv)) which wasted a whole level of
> indentation in the entire function. Interdiff will look horrible.

Would you believe I wanted to say that when thinking about the continue->else part? :)
Attachment #8791645 - Flags: review?(acelists) → review+

Comment 27

9 months ago
https://hg.mozilla.org/comm-central/rev/4e8144c48cff
https://hg.mozilla.org/comm-central/rev/78408d7daf8f (test with typo fixed).

(In reply to :aceman from comment #26)
> Would you believe I wanted to say that when thinking about the
> continue->else part? :)
Great minds think alike ;-)
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0

Updated

6 months ago
Duplicate of this bug: 542947
You need to log in before you can comment on or make changes to this bug.