1530213 - Invite attendees doesn't expand mailing lists anymore

Status: RESOLVED FIXED

(Calendar :: E-mail based Scheduling (iTIP/iMIP), defect)

Description

[Magnus Melin [:mkmelin]]

Invite Attendees doesn't expand mailing lists anymore, so it tries to send to the list representation instead, i.e. <testlist>

This broke sometime before start of this year. Forgot to file.

Originally apparently implemented in bug 459021 (for code pointers).

Comment 1

[[:MakeMyDay]]

For which version is this reported? Is this an issue in ESR, too?

Comment 2

[[:MakeMyDay]]

This might be bug 758493 - I need to rebase the patch there to check it.

Comment 3

[Magnus Melin [:mkmelin]]

Not an issue on ESR. I'm seeing it on nightlies, IIRC at least since sometime December.

Comment 4

[Benjamin Flanagin]

Looking at it now

Comment 5

[Magnus Melin [:mkmelin]]

As a pointer, resolvePotentialList is likely what you should be checking - or why that doesn't get called.
https://searchfox.org/comm-central/rev/ee05c0d9006626f95dfbb1b6a79bed84e2b90e98/calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js#950

https://searchfox.org/comm-central/rev/ee05c0d9006626f95dfbb1b6a79bed84e2b90e98/calendar/base/content/dialogs/calendar-event-dialog-attendees.js#889

Maybe the event target is not what it used to be, or something.
anonid is (basically) used only in xbl (anonymous content id).

Comment 6

[Benjamin Flanagin]

Awesome, I was going to widen my search to things not included in bug 459021 as nothing has changed in ...attendees.js in a while. However, I did notice the lack of xml files which are referenced in the earlier bug, though adding them back didn't do anything. Just figured I would try the easiest solution first.

Comment 7

[Magnus Melin [:mkmelin]]

In the de-xbl effort we're doing, the xbl (.xml) are usuallyl converted to custom elements (.js)..

Using the developer tools debugger to debug by setting break points can be handy. Or just adding dump statements in the javascript (outputs like printf into the console). E.g. dump("xxxbenamin hehehhey\n");

Comment 8

[Benjamin Flanagin]

I have a working fix, will upload patch once I've removed the debug code and add a comment or two.

Comment 9

[Benjamin Flanagin]

Attachment: bug-1530213-fix.patch

Let me know if it works, I only removed a few lines and tweaked one or two others.

Comment 10

[Jorg K (CEST = GMT+2)]

The patch is empty. Do a |hg qref| before attaching it.

Comment 11

[Benjamin Flanagin]

Well that's irritating, I checked the patch before exporting it. Should I be following the workflow at the bottom of. https://developer.thunderbird.net/contributing/fixing-a-bug/using-mercurial-queues ?

Comment 12

[Benjamin Flanagin]

Attachment: bug-1530213-fix.patch

Okay, this one has actual data in it.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9068021 [details] [diff] [review]
bug-1530213-fix.patch

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

The anonid="input" used to be https://dxr.mozilla.org/comm-esr60/source/calendar/base/content/dialogs/calendar-event-dialog-attendees.xml#36

So we should instead of removing all the checks, check target.classList.contains("textbox-addressingWidget")

For commit messages, please use a format like so:
Bug 1530213 - make mailing lists used in the invitee dialog expand again.

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +933,4 @@
>          }
>  
>          let roleStatusIcon = listitem.querySelector(".status-icon");
> +	if(roleStatusIcon) {

what's this for? it doesn't look correct

@@ +951,4 @@
>       */
>      resolvePotentialList(input) {
>          let fieldValue = input.value;
> +        if (fieldValue.length > 0) {

while here, we usually only check truthy, like
if (fieldValue) {

@@ +955,5 @@
>              let mailingList = this._resolveListByName(fieldValue);
>              if (mailingList) {
>                  let entries = this._getListEntries(mailingList);
>                  if (entries.length > 0) {
> +                    let currentIndex = this.mMaxAttendees - 2;

wow the old code is pretty here, carving out the number from something like attendeeCol3#1 without even a comment ... 
but the change also seems wrong

Comment 14

[Benjamin Flanagin]

Attachment: bug-1530213-fix2.patch

• Added comments to explain the code changes.
• Changed the description to match the bug.
• Added the check suggested by Magnus Melin

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9068221 [details] [diff] [review]
bug-1530213-fix2.patch

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

When a patch is ready, you should flag it for review. You do that by setting the review ? flag on the attachment, and in the field that appears you select a reviewer by email address.
Patches without flags risk getting no attention.
For WIP patches, use the feedback flag instead.

I can confirm the patch works, but it still needs some adjustment.

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +933,4 @@
>          }
>  
>          let roleStatusIcon = listitem.querySelector(".status-icon");
> +	// An error occurs if "roleStatusIcon" is null checking if it exists fixes the issue.

but it should never be null...
if it is, find out why

@@ +933,5 @@
>          }
>  
>          let roleStatusIcon = listitem.querySelector(".status-icon");
> +	// An error occurs if "roleStatusIcon" is null checking if it exists fixes the issue.
> +	if(roleStatusIcon) {

for code style this would have been: if (roleStatusIcon)

And you'd also indent the part inside the if

@@ +951,4 @@
>       */
>      resolvePotentialList(input) {
>          let fieldValue = input.value;
> +	// input.id is no longer a variable,   

These kind of comments you can leave in the bug, but if you put them in the code that would be confusing to future readers.
Only put comments about current state in the code.

Also, there's some trailing spaces here

@@ +956,5 @@
>              let mailingList = this._resolveListByName(fieldValue);
>              if (mailingList) {
>                  let entries = this._getListEntries(mailingList);
>                  if (entries.length > 0) {
> +		    // sense id is no longer set we have to use the length of the list as our guide. Subtracting by 2 fixes both the offset (have to count by zero) and the actual location of the list item we want to replace.

Please no comments on what used to be, only the current state. You could note down that index 0 is the organizer, then the rest is maybe as clear as it needs to be

@@ +957,5 @@
>              if (mailingList) {
>                  let entries = this._getListEntries(mailingList);
>                  if (entries.length > 0) {
> +		    // sense id is no longer set we have to use the length of the list as our guide. Subtracting by 2 fixes both the offset (have to count by zero) and the actual location of the list item we want to replace.
> +                    let currentIndex = this.mMaxAttendees -2;

nit: space around the -

For changed files, run the linter, like
./mach lint comm/calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js

@@ +962,3 @@
>                      let template = this.querySelector(".addressingWidgetItem");
>                      let currentNode = template.parentNode.childNodes[currentIndex];
>                      this._fillListItemWithEntry(currentNode, entries[0], currentIndex);

I notice _fillListItemWithEntry doesn't actually take a 3rd argument anymore... so fix the callers.

Comment 16

[Benjamin Flanagin]

Gotcha, will fix and resubmit.

Comment 17

[Benjamin Flanagin]

I've cleaned up the code, but haven't figured out why the first item on the list always returns null when setting roleStatusIcon. I'll keep working on it till I understand whats going on, even if the fix isn't forthcoming.

Comment 18

[Benjamin Flanagin]

Attachment: bug-1530213-fix.patch

Code has been cleaned up per your guidance.

roleStatusIcon is still returning null on the first list item (the one that is replaced when the list is expanded). So the code to get around that issue is still in the patch.

I need some guidance on where to look next for the possible culprit. The odd part of the issue is that the first item returns null, but subsequent "cloned" items do not.

I'll keep looking into it.

Comment 19

[Benjamin Flanagin]

Jorg informed me that we don't like tabs. I'll reformat and resubmit.

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9069377 [details] [diff] [review]
bug-1530213-fix.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +933,4 @@
>          }
>  
>          let roleStatusIcon = listitem.querySelector(".status-icon");
> +	if (roleStatusIcon) {

If you look up how status-icon is used, you'll find that is strange. 
If set, its class will at some places be changed to role-icon. But since we're looking up by class, that won't work after such changes. 

I wonder if it wouldn't make sense to always just use the role-icon.
https://searchfox.org/comm-central/rev/e0c2c4a00a839e66b733dff1671cb18f1b217db3/calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js#719-724

It's a rare occasion that the organizer would not be participating, so having that info there seems not needed. (If that's what the data is put there for, I didn't look to verify.)

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 9069377 [details] [diff] [review]
bug-1530213-fix.patch

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

Ok, looking at it some more, let's go with this and file a follow-up for the role-icon, status-icon wrongness.
It looks like they should use the same thing as itip-icon where the role and status are overlayed on the icon. That's how it's showing in the event participants area.
Also need a folloup for the list size overflowing vertically now... (but that's a different bug.)

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 9069377 [details] [diff] [review]
bug-1530213-fix.patch

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

I'm not landing this.

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +936,5 @@
> +	if (roleStatusIcon) {
> +        	roleStatusIcon.removeAttribute("disabled");
> +        	roleStatusIcon.setAttribute("class", "role-icon");
> +        	roleStatusIcon.setAttribute("role", newAttendee.role);
> +	}

This is mis-indented since it's still using tabs.

@@ +955,5 @@
>              let mailingList = this._resolveListByName(fieldValue);
>              if (mailingList) {
>                  let entries = this._getListEntries(mailingList);
>                  if (entries.length > 0) {
> +                    let currentIndex = this.mMaxAttendees - 2; // start currentIndex at the begining of the address list

Can you move this comment onto the line before and really explain what is happening. If you have 10 attendees, you start at 8?

@@ +970,4 @@
>                          currentIndex++;
>                      }
>                      this.mMaxAttendees += entries.length;
> +                    for (let i = currentIndex; i <= this.mMaxAttendees - 1; i++) {

Why did that change? Also, why not use `i < this.mMaxAttendees` as for regular loops.

Comment 23

[Jorg K (CEST = GMT+2)]

Magnus, what's happening with that index business here? Also, don't you want a Calendar peer to look at this?

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 9069377 [details] [diff] [review]
bug-1530213-fix.patch

Geoff, can to take a look at this please.

Comment 25

[Geoff Lankow (:darktrojan)]

Comment on attachment 9069377 [details] [diff] [review]
bug-1530213-fix.patch

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

I'm giving this r- for the reasons Jorg gave, and a comment from me.

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +933,4 @@
>          }
>  
>          let roleStatusIcon = listitem.querySelector(".status-icon");
> +	if (roleStatusIcon) {

You could do querySelector(".role-icon, .status-icon") which would pick up either. I don't think there's any state where the image doesn't have one of those two classes, so the if check is probably unnecessary.

Comment 26

[Magnus Melin [:mkmelin]]

(In reply to Geoff Lankow (:darktrojan) from comment #25)

You could do querySelector(".role-icon, .status-icon") which would pick up
either. I don't think there's any state where the image doesn't have one of
those two classes, so the if check is probably unnecessary.

That's a bit of nonsense though, since in the if clause you set the role-icon class...
But like I wrote earlier, there's a bunch of wrongness going on with that.

Comment 27

[Benjamin Flanagin]

Attachment: 1530213.patch

Sorry for taking so long to get back to this.

I've gotten rid of the offending tabs and added the suggested change by Geoff in comment 25. It fixes the issue with roleStatusIcon returning null so all good there.

I also made the comment more clear per Jorg's request. And changed <= to < in the loop. The loop now looks like this.

for (let i = currentIndex; i < this.mMaxAttendees - 1; i++) {

I believe the - 1 in the loop will make sense with the new comment. It all amounts to the index no longer being supplied by the input, and having to use the mMaxAttendees variable instead.

Comment 28

[Benjamin Flanagin]

Attachment: 1530213.patch

Sorry for taking so long to get back to this.

I've gotten rid of the offending tabs and added the suggested change by Geoff in comment 25. It fixes the issue with roleStatusIcon returning null so all good there.

I also made the comment more clear per Jorg's request. And changed <= to < in the loop. The loop now looks like this.

for (let i = currentIndex; i < this.mMaxAttendees - 1; i++) {

I believe the - 1 in the loop will make sense with the new comment. It all amounts to the index no longer being supplied by the input, and having to use the mMaxAttendees variable instead.

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 9075795 [details] [diff] [review]
1530213.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +953,5 @@
>              let mailingList = this._resolveListByName(fieldValue);
>              if (mailingList) {
>                  let entries = this._getListEntries(mailingList);
>                  if (entries.length > 0) {
> +                     // We set the current index based on the contact list items location in the attendees list

Nit: Full stop missing at end of line.

@@ +956,5 @@
>                  if (entries.length > 0) {
> +                     // We set the current index based on the contact list items location in the attendees list
> +                     // The index isn't given to us through input so we use mMaxAttendees instead
> +                     // subtracting one to count from 0.
> +                     // To replace the contact list entry with the first value in the list we subtract another.

You mean "mailing list"? And "member of the mailing list"?

@@ +971,5 @@
>                          this._fillListItemWithEntry(currentNode, entry, currentIndex);
>                          currentIndex++;
>                      }
>                      this.mMaxAttendees += entries.length;
> +                    for (let i = currentIndex; i < this.mMaxAttendees - 1; i++) {

I'm totally confused now:
Original code:
i <= this.mMaxAttendees;
Previous patch:
currentIndex; i <= this.mMaxAttendees - 1
This patch:
currentIndex; i < this.mMaxAttendees - 1

Comment 30

[Benjamin Flanagin]

Sorry yes mailing list is what I meant. I'll update the comments with proper punctuation and the word changes.

...
This patch:
currentIndex; i < this.mMaxAttendees - 1

It seems that having the - 1 there has no affect on the code even with the < instead of <= so I will remove it.

My initial thought was that by subtracting one from mMaxAttendees it would prevent the loop from running one too many times. Of course that's what removing the "=" would do as well.

I'll do some proofreading and up load an edited patch.

Comment 31

[Benjamin Flanagin]

Attachment: 1530213.patch

Reworded the comment to hopefully make it more clear.

Removed the - 1 from the loop as it was no longer needed.

Comment 32

[Jorg K (CEST = GMT+2)]

Comment on attachment 9076001 [details] [diff] [review]
1530213.patch

Thanks, this looks OK to me now. I'm not a Calendar peer, so formally I can't review this. Geoff has looked at this before, so he'll review it now.

Comment 33

[Benjamin Flanagin]

Gotcha, thanks for looking at it Jorg.

Comment 34

[Geoff Lankow (:darktrojan)]

Comment on attachment 9076001 [details] [diff] [review]
1530213.patch

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

Okay, but please fix up the comment.

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +956,5 @@
>                  if (entries.length > 0) {
> +                     // We set currentIndex based on the mailing list item location using mMaxAttendees with an offset of -2.
> +                     // The first: So that mMaxAttendees starts counting from 0.
> +                     // The second: To replace the mailing list entry with the first member in the mailing list.
> +                    let currentIndex = this.mMaxAttendees - 2;

This comment is indented one space too many, and the first line should be split in two. Plus I think I'd rewrite it to something like this:

"Start replacing rows at the currently active row, which is the second-to-last row that isn't a dummy row. (mMaxAttendees being a count of non-dummy rows.)"

Comment 35

[Jorg K (CEST = GMT+2)]

A bit disappointing that the patch doesn't apply at all:

$ hg -v qpush
applying 1530213.patch
patching file calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
bad hunk #1 @@ -927,53 +927,57 @@ class MozCalendarEventAttendeesList exte
(54 53 57 57)

Looks like you edited it manually and didn't adjust those numbers :-(

EDIT: Yes, it needs to be @@ -927,53 +927,56 @@ class MozCalendarEventAttendeesList exte
since you're adding three lines not four.

Comment 36

[Jorg K (CEST = GMT+2)]

// Start replacing rows at the currently active row, which is the
// second-to-last row that isn't a dummy row. Note that mMaxAttendees
// is a count of non-dummy rows.

Sorry, this is is still not clear, so I had to try it out.

With myself as organiser, another invitee and then the list being replaced on position 3 (counting from 1), I saw a value of 4, so I really have no idea what this is counting. Also, what's a dummy row?

The patch works although it's still not clear to me why we're doing this:

-                    for (let i = currentIndex; i <= this.mMaxAttendees; i++) {
+                    for (let i = currentIndex; i < this.mMaxAttendees; i++) {

Was that wrong before?

I came to land that, but I can't :-(

Comment 37

[Geoff Lankow (:darktrojan)]

It appears that was wrong before.

mMaxAttendees (which does appear to be an odd name) is the count of rows that contain an icon and a space to accept an email address (or the organiser, I guess, which doesn't accept an email address, but has one). All of the other rows are dummy rows and only contain the bottom border. The bottom borders don't actually line up across the window, but that's a story for another bug.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2f0f868999d8
Make invite attendees expand mailing lists. r=darktrojan DONTBUILD

Comment 39

[Jorg K (CEST = GMT+2)]

Landed with more accurate comment as discussed with Geoff on IRC.

Comment 40

[Jorg K (CEST = GMT+2)]

TB 68 beta 5 / Cal 7.0:
https://hg.mozilla.org/releases/comm-beta/rev/be43f86e0509f04130b512f61ddbbe2777d2bcb2

Comment 41

[Magnus Melin [:mkmelin]]

Bug 1565999 and bug 1565992 filed for the problems discovered while reviewing this.